Support GitHub actions #558
Conversation
|
@AdamDonna - Is there anything I can help with this PR? |
|
@j-antunes Yes. If there's an templated solution for this that would be amazing, or if you have experience getting github actions working feel free to add to this PR or create a competing solution 👍 |
|
@AdamDonna - Just wanted to touch base with you. I started by adding the linting CI to the project. I ran the unit tests locally, and they all passed. I also noticed we are missing a |
|
Great work. Did you get pre commit to work with those rules? Moving to pyproject allows us to copy and modify config from other projects and it looks like the direction other jazzband projects are moving. I support it based that. Interesting that we don't have a requirements file. I wonder how that was managed historically 🤔 |
|
Yes, pre-commit also worked. However, I did not change anything on it. Worth revisiting it. Sounds good about pyproject I think previously they just installed the packages that they needed. If you look at Then on the CI/CD it looks like they used somr python-django pre-built containers. So those must already have the necessary packages. |
j-antunes
force-pushed
the
support-github-actions
branch
2 times, most recently
from
8c67dff to
01a0f0a
Compare
j-antunes
force-pushed
the
support-github-actions
branch
from
4cbc294 to
3fe25ee
Compare
for more information, see https://pre-commit.ci
|
@AdamDonna - I just pushed a new CI/CD to run unit tests. It now runs the file We are missing running coverage for the unit tests, and running tox. I think it would make sense to run them separate. I think these should only trigger when merging to main. |
| - name: Set up Python environment | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: "3.10" |
There was a problem hiding this comment.
Can this be configured as a matrix so we can test against many python versions?
| python-version: "3.10" | ||
| - name: Install dependencies | ||
| run: | | ||
| pip install django==3.2 |
There was a problem hiding this comment.
Can this be configured as a matrix so we can cover different django version, or add additional steps for each django version?
There was a problem hiding this comment.
Yeah, that would be the next step. To run the different versions of python and django.
I think how I would like to implement this. Would be a sort of multi step. When someone pushs an initial PR it runs the unittests for the oldest version first. If that passes and someone does an initial review of the code. It would trigger running the matrix.
I think this makes sense, to save resources. There is no point in running the test matrix for something that is not yet finished.
|
I'm strongly in favour of running coverage should run on all changes as a part of each push. It helps to identify paths which aren't tested before they're merged, and prompts contributors to add more tests. |
| flake8-lint: | ||
| runs-on: ubuntu-latest | ||
| name: flake8 Lint | ||
|
|
||
| steps: | ||
| - name: Check out source repository | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Set up Python environment | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: "3.10" | ||
|
|
||
| - name: Install flake8 | ||
| run: pip install flake8 | ||
|
|
||
| - name: Run flake8 | ||
| run: flake8 . |
There was a problem hiding this comment.
I would not add flake8 since ruff is a thing these days.
Besides, adding linting sounds like a different PR than just getting GHA running?
There was a problem hiding this comment.
Oh, not to mention the repo already uses pre-commit, so you can just use pre-commit/action :)
There was a problem hiding this comment.
I'm a bit confused. Are you suggesting to keep using pre-commit instead of ruff?
There was a problem hiding this comment.
No – it's just that this PR would be adding additional linting configuration beyond that already implemented in #557; instead of doing lints "manually", one can just run pre-commit, so lints are being configured in one place.
#560 switches the black + isort + flake8 configuration to ruff (and adds the GHA step that runs pre-commit).
There was a problem hiding this comment.
There's a bit more to it. Jazzband enforce pre-commit as a CI step eg:// https://results.pre-commit.ci/run/github/1360247/1701345236.N6ZJwVBqR5qkswtYyt4tsw
So we must have some sort of pre-commit tooling. Running the precommit tooling against file changes in the diff is the most consistent way of enforcing linting.
Switching to Ruff looks fine. It looks easy to back out of in the unlikely event there's issues with it
| @@ -0,0 +1,21 @@ | |||
| name: CI-UnitTests | |||
There was a problem hiding this comment.
I'm in strong favour of this file being renamed to test which feels like a standard
|
I'm not convinced that we should be changing the linting rules to get Ruff working here. That looks like a concession to get the tool working, rather than getting the tool to work for our configuration. I think that will need some reinvestigation before we move commit to it |
|
What would be the impact of changing the linter? I agree with you, that it would be better not to change it, but what I'm thinking is that the project as been out of date for quite some time, that an update/upgrade of this time would make sense. |
TBH, my #560 PR is the one changing the linter - flake8 had been technically in use in this repo before, but not enforced. |
|
@akx - Yes I agree. Let's close this one. |
No description provided.