adjust GitHub actions and tox config #506

terencehonles · 2021-04-02T04:07:00Z

Changes to GitHub actions:

The service definition was duplicated on squash
Merge lint and test workflow files into ci.yml
Update for Django 3.2 and only test unreleased library versions with Python 3.9 to limit permutations
Move redis sentinel startup to a script to more easily use in local development

Changes to README:

Fix the GitHub badge definition

Changes to tests and testing environment:

Move tox config to setup.cfg
Remove runtest-*.py wrappers in favor of using runtest.py --settings=<settings>
Move test settings into their own directory
Fix unix socket tests

x-ref: ymyzk/tox-gh-actions#60 will remove the need for lint-dj31-redislatest

WisdomPill

I think you forgot something, the linters are not checked in these tests.
Probably the option in matrix section is missing.
Furthermore I am not a fan of tox and prefer the django approach to separate the linters so that it is easier to see the different job failures. Here is the django way of linting.

So I would say to leave the lint action and separate the listing jobs, I know it takes longer because the installation has to be done from scratch before executing the linter check, it is just personal taste and cleanliness.

That said, I am open to discuss different options.

terencehonles · 2021-04-02T07:08:39Z

It's defined for a specific run as seen in the tox config https://github.com/jazzband/django-redis/pull/506/checks?check_run_id=2251646864#step:8:140

I mainly made the change because the lint workflow was using tox too.

terencehonles · 2021-04-02T07:11:37Z

It would probably be best to eventually configure a check runner which can add inline annotations to the PRs and not require users to parse the log output.

WisdomPill · 2021-04-02T14:00:48Z

It would probably be best to eventually configure a check runner which can add inline annotations to the PRs and not require users to parse the log output.

could you elaborate? I don't understand.

What do you think about my proposal?
Have you seen how linters work in django's repo?

terencehonles · 2021-04-02T17:37:21Z

It would probably be best to eventually configure a check runner which can add inline annotations to the PRs and not require users to parse the log output.

could you elaborate? I don't understand.

These are status "checks" https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/about-status-checks which are different than "statuses" which is just a pass/fail from a build action.

What do you think about my proposal?
Have you seen how linters work in django's repo?

You are saying to split them out individually? Yes I'm aware of that way of doing that. We don't need a separate workflow to make that happen though.

Furthermore I am not a fan of tox

Is there a reason you don't like tox? It is convenient to have a simple command run all the tests locally. I do understand there is more output that you have to parse, but that was why I suggested in the long term it would be good to surface the issues in the PR rather in the logs.

terencehonles · 2021-04-02T18:35:25Z

I realized one issue you may have had with the current tox setup is that all lint steps are combined. I could split out each of the lint tools into their own env which may make it easier to see failures (and it will allow them all to run regardless if the other failed).

Each of the tox envs are grouped by tox-gh-actions, and can be expanded (I thought they expanded by default if the section failed, but not so sure about that now).

WisdomPill · 2021-04-03T11:20:05Z

DISCAIMER: this is just an opinion, I am not here to fight any battles or impose my views

I don't like tox because it is not a straightforward tool in my opinion, I find it useful because you can run different combinations of tools (in this case python, django and redis). But I think it is overdoing it because GitHub actions already have matrix support. Sure you can't do that locally, but do you want to run tests on every version possible locally every time? Furthermore by just looking at the workflow you don't understand what is it doing. For the same reason, for linting, I prefer to have separate jobs in separate actions to keep things simple. I am not a big fan of merging everything together with one tool, in this case lint workflows and tests. Also adding different split linters inside tox it seems too much for me. Having one tool to do everything is a simplification on one side and a complication on another. As a matter of fact what you are proposing is much more complicated than just do it the "django" way, or I might say the "django's repo" way, I find it more elegant. Finally I don't mind proceeding with what you have proposed here, as I said in the disclaimer I am not here to fight any battles, it is not worth it for this kind of things.

terencehonles · 2021-04-03T21:25:09Z

I think it is overdoing it because GitHub actions already have matrix support.

While that's true tox makes installing the dependencies easier. Otherwise you'd need to write your own shell script to install potential different requirements.txt files. Also I hadn't seen tox-gh-actions before, but it's pretty cool too have tox "automatically" pick the right envs based on the matrix run.

Sure you can't do that locally, but do you want to run tests on every version possible locally every time?

Maybe yes, and maybe no. It definitely helps for testing against multiple versions of Python and although Python 2 support has been dropped, for the time that it was supported it was nice to easily have both run (I had both installed on my system). Since I don't have 3.6 on my system anymore there's not too many runs that happen by default anymore.

I'm definitely on the fence about tox running everything by default, but I encourage you to get familiar with the -l and -e flags. Knowing those is definitely been helpful to me. Usually I'll run tox the default way to see if anything breaks. Sometimes if a library hasn't been updated in awhile I use -l to see what might run and if they've even specified py38. Once there's been a failure or I know what I might want to run I use the -e flag to target exactly what I want.

I know tox can be cumbersome, but it can also be really useful if set up correctly.

One thing I noticed is we probably should have a way to detect if redis is available and also have a simple way to start it up so people can run the tests locally since that's a gap that's not "trivial" to fix now that the tests expect redis and a sentinel. We may also want to migrate the tests to use pytest and pytest-django since pytest has command line flags that are pretty nice (--pdb, --lf, etc)

Furthermore by just looking at the workflow you don't understand what is it doing. For the same reason, for linting, I prefer to have separate jobs in separate actions to keep things simple. I am not a big fan of merging everything together with one tool, in this case lint workflows and tests.

I'll definitely buy that, but how things are organized is debatable and depending on the size of things splitting out may not help.

Also adding different split linters inside tox it seems too much for me. Having one tool to do everything is a simplification on one side and a complication on another.

You may be misunderstanding what I'm suggesting (or maybe not). I don't think it would be complicating the config more than it already is, which I'll agree we could split out of tox.

As a matter of fact what you are proposing is much more complicated than just do it the "django" way, or I might say the "django's repo" way, I find it more elegant. Finally I don't mind proceeding with what you have proposed here, as I said in the disclaimer I am not here to fight any battles, it is not worth it for this kind of things.

Part of my change to merge the workflow runs was to make sure the badge on the README was specified correctly and pointed to the actual state of things. Since this code doesn't change much are we ok merging this in and continuing to update things?

Or would you prefer the lint run to be separate but specified in the same workflow file? I don't mind holding off on that part of the change and can make the lint check remain tox -e lint on it's own run. I'm not suggesting increasing the scope of this change to having more runners at this point, but I think we can follow up soon.

Just a small reminder that the main reason I'd like to merge this in because the duplicate redis service definition is keeping the tests from running.

WisdomPill · 2021-04-04T07:40:06Z

I understand what you mean, I prefer to do things little by little, so here is my proposal and reason to proceed this way.

Let's make only the fix to the squash now, and tidy tox a little. Let's keep lint workflow so if that fails on the email you already get the workflow that has failed (lint or test).

After that I propose to have a separate issue to split linters in tox.

I am also in favor of your change to put tox config in setup.cfg find it an elegant solution.

Also it would be nice to close this issue asap so we could proceed to fix codecov and add coverage badge and checks in #510.

Last but not least I had in mind to change a little bit the linting settings, in favor of black compatibility tips, you can consult them here

WisdomPill · 2021-04-04T07:45:57Z

And I forgot to mention, for redis health check and sentinel I think we could create a directory with utility scripts.

By the way that is what I do instead of using tox, in my projects I have check and run linters scripts that look like this.

check-linters.sh

#!/usr/bin/env zsh

echo "Checking black"
black --target-version py38 --check --diff .

echo "Checking flake8"
flake8 --exclude .venv,.mypy_cache

echo "Checking isort"
isort --check-only --diff .

echo "Checking mypy"
mypy lib

run-linters.sh

#!/usr/bin/env zsh

echo "Running black"
black --target-version py38 .

echo "Running flake8"
flake8 --exclude .venv,.mypy_cache

echo "Running isort"
isort .

and the lint.yml would look like this, also I forgot to mention that I use pipenv

name: Lint

on:
  - push
  - pull_request

jobs:
  black:
    runs-on: ubuntu-latest

    steps:
      - uses: actions/checkout@v2

      - name: Set up Python 3.8
        uses: actions/setup-python@v2
        with:
          python-version: 3.8

      - name: Get python version
        run: echo "PYTHON_VERSION=$(python3 --version | awk '{print $2}')" >> $GITHUB_ENV

      - name: Install pipenv
        run: pip install pipenv

      - name: Cache pipenv
        uses: actions/cache@v2
        with:
          path: ~/.local/share/virtualenvs
          key: lint-pipenv-${{ env.PYTHON_VERSION }}-${{ hashFiles('Pipfile.lock') }}
          restore-keys: |
            lint-pipenv-${{ env.PYTHON_VERSION }}

      - name: Install dependencies
        run: pipenv install --dev --python 3.8

      - name: Run black
        run: pipenv run black --target-version py38 --check --diff .

  flake8:
    runs-on: ubuntu-latest

    steps:
      - uses: actions/checkout@v2

      - name: Set up Python 3.8
        uses: actions/setup-python@v2
        with:
          python-version: 3.8

      - name: Get python version
        run: echo "PYTHON_VERSION=$(python3 --version | awk '{print $2}')" >> $GITHUB_ENV

      - name: Install pipenv
        run: pip install pipenv

      - name: Cache pipenv
        uses: actions/cache@v2
        with:
          path: ~/.local/share/virtualenvs
          key: lint-pipenv-${{ env.PYTHON_VERSION }}-${{ hashFiles('Pipfile.lock') }}
          restore-keys: |
            lint-pipenv-${{ env.PYTHON_VERSION }}

      - name: Install dependencies
        run: pipenv install --dev --python 3.8

      - name: Run flake8
        run: pipenv run flake8

  isort:
    runs-on: ubuntu-latest

    steps:
      - uses: actions/checkout@v2

      - name: Set up Python 3.8
        uses: actions/setup-python@v2
        with:
          python-version: 3.8

      - name: Get python version
        run: echo "PYTHON_VERSION=$(python3 --version | awk '{print $2}')" >> $GITHUB_ENV

      - name: Install pipenv
        run: pip install pipenv

      - name: Cache pipenv
        uses: actions/cache@v2
        with:
          path: ~/.local/share/virtualenvs
          key: lint-pipenv-${{ env.PYTHON_VERSION }}-${{ hashFiles('Pipfile.lock') }}
          restore-keys: |
            lint-pipenv-${{ env.PYTHON_VERSION }}

      - name: Install dependencies
        run: pipenv install --dev --python 3.8

      - name: Run isort
        run: pipenv run isort --check-only --diff .

  mypy:
    runs-on: ubuntu-latest

    steps:
      - uses: actions/checkout@v2

      - name: Set up Python 3.8
        uses: actions/setup-python@v2
        with:
          python-version: 3.8

      - name: Get python version
        run: echo "PYTHON_VERSION=$(python3 --version | awk '{print $2}')" >> $GITHUB_ENV

      - name: Install pipenv
        run: pip install pipenv

      - name: Cache pipenv
        uses: actions/cache@v2
        with:
          path: ~/.local/share/virtualenvs
          key: lint-pipenv-${{ env.PYTHON_VERSION }}-${{ hashFiles('Pipfile.lock') }}
          restore-keys: |
            lint-pipenv-${{ env.PYTHON_VERSION }}

      - name: Install dependencies
        run: pipenv install --dev --python 3.8

      - name: Run mypy
        run: pipenv run mypy lib

I am not propose to go this way, I just wanted to show how I work, in my projects we usually do not use matrix at all.
I might also be a little bit old fashioned that I like shell scripts to manage stuff, my bad 😄.

WisdomPill · 2021-04-04T14:54:07Z

.github/workflows/test.yml

 jobs:
  build:
-    name: build (Python ${{ matrix.python-version }}, Django ${{ matrix.django-version }}, Redis.py ${{ matrix.redis-version }})
+    name: Python ${{ matrix.python-version }}, Django ${{ matrix.django-version }}, Redis.py ${{ matrix.redis-version }}


I would actually not change this or maybe call it test

The title already includes "test" because that's the name of the workflow. You can see what this looks like on this pr

I changed this to test because I added back the lint check at your request.

WisdomPill · 2021-04-04T14:55:39Z

Okay, I had a look at tox and it does not seem that difficult to learn, I would suggest to add another environment called lintenv and split the lint tools so that they can run in different jobs. Another thing is that we need to discuss on another issue or pr is on how to use pytest and enable coverage reports

WisdomPill

Could you separate the lint steps?

You could also leave a step that executes the others for development, and also why there are two lint environments?

terencehonles · 2021-04-04T19:07:01Z

Could you separate the lint steps?

They are separate. If you're asking for separate workflow files, no. The badge on the README should point to the workflow run and that was the point of me merging the two. There's not really a benefit to separate the workflows other than to have more files. The build definitions are rather short and the definition is only important to the maintainers not people submitting PRs.

If you're asking about the different lint tools let's do that separately. I'm just trying to keep this "compatible" with the previous env setup / your requests to have them separate.

You could also leave a step that executes the others for development

Not sure what this means

and also why there are two lint environments?

I left this in because it doesn't hurt and we're still working towards the future ci setup.

Just a fyi I'm not going to be near my computer for the rest of the day. I just wanted to push at least a compromise with the lint step so we could get this merged in. (Not sure if you were aiming to do that today or later in the week is fine)

WisdomPill · 2021-04-04T19:10:06Z

No I mean, inside tox, so you could have different checks showing up.

I mean these two [testenv:{lint,lint-dj31-redislatest}]

WisdomPill · 2021-04-04T19:24:05Z

And I see requirements.txt is not used, can you confirm that?

terencehonles · 2021-04-05T16:36:19Z

And I see requirements.txt is not used, can you confirm that?

That looks to be correct. It was no longer referenced in 2af9ab9 when tox was used on the CI.

No I mean, inside tox, so you could have different checks showing up.

I mean these two [testenv:{lint,lint-dj31-redislatest}]

I'm not sure what this comment was pertaining to. I believe I had responded to the original question:

and also why there are two lint environments?

I left this in because it doesn't hurt and we're still working towards the future ci setup.

You may have interpreted:

Not sure what this means

and also why there are two lint environments?

But I was following quotes, not preceeding them, so my question was:

You could also leave a step that executes the others for development

Not sure what this means

WisdomPill · 2021-04-06T10:15:17Z

I meant having

[lintenv:isort]
...

[lintenv:flake8]
...

[lintenv:black]
...

it is nice to have the runs that have failed in the email and pr page rather than looking at logs.
Like django official repo is doing, have a look here

On that manner, I think [testenv:{lint,lint-dj31-redislatest}] should be removed.

About the test changes that you have made,

WisdomPill · 2021-04-06T10:19:33Z

tests/README.txt

+.. code-block:: bash

-Install the development requirements using the requirements.txt file:
+  # start redis and a sentinel


A little disclaimer that the setup it is done with docker would be nice

WisdomPill · 2021-04-06T10:21:53Z

tests/runtests.py

+SOCK_SETTINGS = "settings.sqlite_usock"

 if __name__ == "__main__":
+    if any(SOCK_SETTINGS in i for i in sys.argv) or SOCK_SETTINGS in os.environ.get(


Any ideas on how to enable tests with unix socket?

They work locally now, we probably need to move the CI to use the start redis script which can pass the arguments needed to configure redis. Alternatively we could probably use socat to pipe the tests to redis...

terencehonles · 2021-04-06T20:24:31Z

I meant having
[lintenv:isort]
...

[lintenv:flake8]
...

[lintenv:black]
...
it is nice to have the runs that have failed in the email and pr page rather than looking at logs.
Like django official repo is doing, have a look here

On that manner, I think [testenv:{lint,lint-dj31-redislatest}] should be removed.

About the test changes that you have made,

That is what I meant by:

I realized one issue you may have had with the current tox setup is that all lint steps are combined. I could split out each of the lint tools into their own env.

But you had replied:

As a matter of fact what you are proposing is much more complicated than just do it the "django" way, or I might say the "django's repo" way, I find it more elegant.

So I figured we could get this PR in and then either pull out tox for the lint steps or split it as you're suggesting now.

terencehonles · 2021-04-06T21:43:35Z

I backed out the env lint-dj31-redislatest since that was the main "regression" in this PR, and will merge it (I squashed to a smaller set of logical changes). We should put future tox-gh-actions, tox env refactoring, and pytest changes in their own PRs.

WisdomPill · 2021-04-07T05:02:24Z

I understand you are in hurry, but you could at least for other people to review and merge your changes.

Why did you remove django 3.0 from matrix?

terencehonles · 2021-04-07T06:29:25Z

Why did you remove django 3.0 from matrix?

Django 3.2 was released and 3.0 hit end of extended support (https://www.djangoproject.com/download/)

terencehonles requested a review from WisdomPill April 2, 2021 04:08

WisdomPill reviewed Apr 2, 2021

View reviewed changes

terencehonles force-pushed the fix-misc-action-config branch 2 times, most recently from 4b68659 to 75405d7 Compare April 2, 2021 18:32

WisdomPill mentioned this pull request Apr 4, 2021

Fix codecov #510

Closed

WisdomPill reviewed Apr 4, 2021

View reviewed changes

terencehonles force-pushed the fix-misc-action-config branch from d9e2c55 to ca7df53 Compare April 4, 2021 18:44

WisdomPill requested changes Apr 4, 2021

View reviewed changes

terencehonles changed the title ~~fix squash error and other miscellaneous GitHub actions config~~ adjust GitHub actions and tox config Apr 5, 2021

WisdomPill requested changes Apr 6, 2021

View reviewed changes

terencehonles force-pushed the fix-misc-action-config branch from 8341bad to dd9167e Compare April 6, 2021 20:58

terencehonles added 3 commits April 6, 2021 14:20

fix squash error and other miscellaneous GitHub actions config

f156a73

add scripts to manage redis for local testing and fix unixsocket tests

d1c383f

deduplicate test runners and organize test settings

0791b02

add Django 3.2 and drop Django 3.0 from support list and test matrix

6308e85

terencehonles force-pushed the fix-misc-action-config branch from dd9167e to 6308e85 Compare April 6, 2021 21:23

terencehonles merged commit 2c9b586 into jazzband:master Apr 6, 2021

terencehonles mentioned this pull request Apr 6, 2021

fix stray markdown in README #511

Merged

terencehonles deleted the fix-misc-action-config branch April 6, 2021 22:01

terencehonles mentioned this pull request Apr 7, 2021

Add unix socket tests to CI #515

Closed

Uh oh!

adjust GitHub actions and tox config #506

adjust GitHub actions and tox config #506

Uh oh!

Conversation

terencehonles commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WisdomPill left a comment

Choose a reason for hiding this comment

Uh oh!

terencehonles commented Apr 2, 2021

Uh oh!

terencehonles commented Apr 2, 2021

Uh oh!

WisdomPill commented Apr 2, 2021

Uh oh!

terencehonles commented Apr 2, 2021

Uh oh!

terencehonles commented Apr 2, 2021

Uh oh!

WisdomPill commented Apr 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DISCAIMER: this is just an opinion, I am not here to fight any battles or impose my views

Uh oh!

terencehonles commented Apr 3, 2021

Uh oh!

WisdomPill commented Apr 4, 2021

Uh oh!

WisdomPill commented Apr 4, 2021

Uh oh!

WisdomPill Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

terencehonles Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

terencehonles Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

WisdomPill commented Apr 4, 2021

Uh oh!

WisdomPill left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terencehonles commented Apr 4, 2021

Uh oh!

WisdomPill commented Apr 4, 2021

Uh oh!

WisdomPill commented Apr 4, 2021

Uh oh!

terencehonles commented Apr 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WisdomPill commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WisdomPill Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

WisdomPill Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

terencehonles Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

terencehonles commented Apr 6, 2021

Uh oh!

terencehonles commented Apr 6, 2021

Uh oh!

WisdomPill commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terencehonles commented Apr 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

terencehonles commented Apr 2, 2021 •

edited

Loading

WisdomPill commented Apr 3, 2021 •

edited

Loading

WisdomPill left a comment •

edited

Loading

terencehonles commented Apr 5, 2021 •

edited

Loading

WisdomPill commented Apr 6, 2021 •

edited

Loading

WisdomPill commented Apr 7, 2021 •

edited

Loading