Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial workflow automation: nox and automated pip-compile #258

Merged
merged 7 commits into from
Aug 21, 2023

Conversation

gotmax23
Copy link
Collaborator

@gotmax23 gotmax23 commented Aug 8, 2023

Description

This PR has two parts. There's a noxfile to run python linters and a workflow to automatically file PRs to update locked dependencies.

noxfile

The noxfile has targets to lint/format the noxfile itself and the issue labeler
script using standard python linters. The linters can be run with nox -e lint. I'd like to apply these linters to other files in a separate PR. I will add a docs session to replace the current Makefile-based approach in a future PR.

New requirements

There's new requirements files in the test directory for the static, formatters, and typing nox sessions. These sessions can be run together with nox -e lint.

nox workflow

There's a workflow to run linters with nox in CI.

noxfile pip-compile

There's also a nox session to run pip-compile to update requirements files. It can be run with nox -e pip-compile. This applies to the docs requirements and the requirements for the linters.

pip-compile workflow

This workflow runs on a scheduled basis to update requirements lockfiles and submit PRs if there are any changes. It uses nox -e pip-compile.


Please Rebase and merge. The commits have been split up in a very intentional way.

Relates: #54

@gotmax23 gotmax23 added the doc builds Relates to building the documentation label Aug 8, 2023
@github-actions github-actions bot added the needs_triage Needs a first human triage before being processed. label Aug 8, 2023
@oraNod oraNod removed the needs_triage Needs a first human triage before being processed. label Aug 9, 2023
.github/workflows/nox.yml Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
.github/dependabot.yml Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
.github/workflows/nox.yml Outdated Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
@gotmax23 gotmax23 force-pushed the nox2 branch 2 times, most recently from e4e3ebb to 612f048 Compare August 9, 2023 15:53
@oraNod
Copy link
Contributor

oraNod commented Aug 9, 2023

@gotmax23 Hey just chiming in here a bit to say that I've been looking at this today and playing around. I want to get more familiar before I start commenting and leave a review but so far I think it's really awesome.

@gotmax23
Copy link
Collaborator Author

gotmax23 commented Aug 9, 2023

@gotmax23 Hey just chiming in here a bit to say that I've been looking at this today and playing around. I want to get more familiar before I start commenting and leave a review but so far I think it's really awesome.

Cool, thanks! Feel free to ask any questions.

noxfile.py Outdated Show resolved Hide resolved
.pip-tools.toml Outdated Show resolved Hide resolved
.github/dependabot.yml Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
.github/workflows/pip-compile.yml Outdated Show resolved Hide resolved
.github/workflows/pip-compile.yml Outdated Show resolved Hide resolved
.github/workflows/pip-compile.yml Outdated Show resolved Hide resolved
.github/workflows/pip-compile.yml Outdated Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
@gotmax23 gotmax23 force-pushed the nox2 branch 2 times, most recently from 48ecacc to def1d1f Compare August 9, 2023 17:59
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@samccann
Copy link
Contributor

@gotmax23 please describe what this PR is doing. The description to merge and rebase is ... not so helpful to someone like me ;-)

@gotmax23
Copy link
Collaborator Author

I force pushed a modified ci: add workflow to refresh pinned dependencies commit that splits out the pip-compile update workflows as follows:

  • .github/workflows/reusable-pip-compile.yml - reusable logic
  • .github/workflows/pip-compile-docs.yml - a workflow that uses the reusable workflow to update the docs build dependencies
  • .github/workflows/pip-compile-dev.yml - a workflow that uses the reusable workflow to update the dependencies for linters, formatters, and other dev tools we may add in the future

Both pip-compile-dev and pip-compile-docs can be triggered manually. They both run weekly, but this can be changed.

Comment on lines +55 to +58
permissions:
pull-requests: write
contents: write
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could move this to the job level to be stricter?

Suggested change
permissions:
pull-requests: write
contents: write
permissions: {}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? If the parent job doesn't set permissions, GHA will fail. It doesn't automatically inherit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more of a defensive style. Also, that “parent” only exists through the reusable path, not workflow_dispatch.

nox ${{ inputs.nox-args }}
- name: Push new dependency versions and create a PR
env:
GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you see this concern? #258 (comment)

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good enough for the first iteration and later on, the existing concerns can be addressed via follow-ups.

Copy link
Contributor

@oraNod oraNod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think it's split out nicely enough so probably no need for separate PRs as I suggested in an earlier comment. We can adjust the schedule.interval frequency later on too if weekly is gets noisy or leads to issues with broken builds from fast changing requirements.

@gotmax23 Another one for follow up but I suppose we should really start using Fedora in our workflows instead of Ubuntu, wouldn't you say?

@gotmax23 gotmax23 added the no_backport This PR should not be backported. devel only. label Aug 21, 2023
@gotmax23
Copy link
Collaborator Author

Marking as no_backport. I'll handle manually.

gotmax23 and others added 7 commits August 21, 2023 18:25
Currently, this only lints the noxfile itself and the issue labeler
script.
We can expand the noxfile to build docs and lint other new files in
hacking.

Relates: ansible#54
This workflow updates pinned dependencies and files a PR if necessary.
For now, it only applies to the devel branch.

Co-authored-by: Don Naro <[email protected]>
This allows running the tests on an adhoc basis.
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
@gotmax23
Copy link
Collaborator Author

I adjusted this PR to account for the new requirements-relaxed file.

@gotmax23
Copy link
Collaborator Author

LGTM. I think it's split out nicely enough so probably no need for separate PRs as I suggested in an earlier comment. We can adjust the schedule.interval frequency later on too if weekly is gets noisy or leads to issues with broken builds from fast changing requirements.

The current PR submits separate PRs for docs and linters. I think this makes most sense. I explained my view in #258 (comment).

@gotmax23 Another one for follow up but I suppose we should really start using Fedora in our workflows instead of Ubuntu, wouldn't you say?

As a Fedora developer, I strongly favor it, but I don't think using Ubuntu poses any direct problems per se. If we end up needing to test against very new Python versions, running in a Fedora container in GHA is a good choice, as it gets them earlier than GHA's Ubuntu images.

@gotmax23
Copy link
Collaborator Author

Okay, this should be ready to merge. The GHA will trigger and submit new PRs (assuming everything works properly) after I merge this. Thank you all for your valuable feedback!

@gotmax23 gotmax23 merged commit c074d63 into ansible:devel Aug 21, 2023
6 checks passed
@webknjaz
Copy link
Member

If we end up needing to test against very new Python versions, running in a Fedora container in GHA is a good choice, as it gets them earlier than GHA's Ubuntu images.

Ubuntu way is to just use deadsnakes: https://github.com/marketplace/actions/deadsnakes

@gotmax23
Copy link
Collaborator Author

If we end up needing to test against very new Python versions, running in a Fedora container in GHA is a good choice, as it gets them earlier than GHA's Ubuntu images.

Ubuntu way is to just use deadsnakes: https://github.com/marketplace/actions/deadsnakes

Fedora has Cpython 2.7, 3.6-3.12, and Pypy 3.9 and 3.10 in the official repositories. The packages go through the same QA/testing requirements as the rest of the Fedora packages and don't require enabling any extra repositories. We (the Fedora Python SIG) package all these versions to allow Python developers to test across Python versions and achieve our goal of making Fedora the ideal distro for Python developers. Upstream Cpython changed their release schedule to accommodate Fedora, so we could get the latest Python version in every odd Fedora release and provide value to the ecosystem by submitting patches upstream to fix compat issues. We really should advertise this more. Fedora loves Python.

@webknjaz
Copy link
Member

We really should advertise this more. Fedora loves Python.

Miro actually gave a lightning talk about this during EuroPython :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc builds Relates to building the documentation no_backport This PR should not be backported. devel only.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants