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

🎨🧪 Wire linters into the main CI workflow #1652

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

webknjaz
Copy link
Contributor

This patch makes the quality workflow definition into reusable. It doesn't need to track the same triggers and conditions as the main one. Additionally, the alls-green check can now take into account the outcome of linting making it possible to only have one job in the branch protection.

@webknjaz webknjaz marked this pull request as draft July 12, 2023 00:25
@webknjaz webknjaz force-pushed the maintenance/gha-reusable-quality branch from 61a4226 to 4fe3333 Compare July 12, 2023 00:27
@webknjaz webknjaz marked this pull request as ready for review July 12, 2023 00:27
@nedbat
Copy link
Owner

nedbat commented Jul 12, 2023

I'm missing what problem this solves? Conceptually, tests and linting are separate to me, so deserve separate workflows.

@webknjaz
Copy link
Contributor Author

Yes, I understand that the linters may be conceptually different. Though, they still remain in a separate workflow definition file but show up on the same screen with other jobs. As in this graph: https://github.com/nedbat/coveragepy/actions/runs/5526202109?pr=1652. The checks still show up in the same widget at the bottom of PRs too.

But wiring that into the unified check job helps prevent forgetting to change branch protection, which also tends to be set by linters. And being able to do so also opens up some possibilities, like file-dependent job runs that can still make use of branch protection (this is the problem I was solving for CPython recently, for example).

I see that 4250899 dropped mypy from the branch protection and maybe re-added and re-removed it. But it's hard to say since these changes aren't traceable. With everything wired in, it'd be possible to see what jobs and under which condition influence the final branch protection check.

Of course, it's your call, but I've been rolling out this approach of modular workflow definitions in several places recently, and it seems to work very well.

@webknjaz webknjaz force-pushed the maintenance/gha-reusable-quality branch from 4fe3333 to 259c135 Compare July 12, 2023 01:47
@webknjaz webknjaz force-pushed the maintenance/gha-reusable-quality branch from 259c135 to d579f73 Compare August 7, 2023 12:37
@nedbat
Copy link
Owner

nedbat commented Aug 8, 2023

I guess I don't understand the reason for making it reusable, just to use it from one workflow? Why not embed it directly in the workflow? I must be missing something.

@webknjaz
Copy link
Contributor Author

just to use it from one workflow?

Yes, a single workflow runtime is viewable as a single entity on the UI.

Why not embed it directly in the workflow?

It's possible, but it's usually nice to have separate job definitions in dedicated files. Having them in their own files lets us avoid having an oversized main workflow definition that ties them together. At the same time, this allows to consolidate the triggers in the main workflow and not have to keep them in sync through many files.
Think of Python — we put things into separate modules to structure and group stuff. But it is also a layer separation mechanism. Low-level implementation may be separate from the top-level control flow.

https://learn.scientific-python.org/development/guides/gha-basic/#reusable-workflows

I must be missing something.

I think the idea here is that yes, it's nice to have modularized definitions for semantically different jobs. And at the same time, it's hard to maintain huge YAML files. So this concept is a good middle ground in my opinion. FWIW, after I used it in a few repos, I liked it a lot and plan to use in my other projects.

This patch makes the quality workflow definition into reusable. It
doesn't need to track the same triggers and conditions as the main
one. Additionally, the `alls-green` check can now take into account
the outcome of linting making it possible to only have one job in
the branch protection.
@webknjaz webknjaz force-pushed the maintenance/gha-reusable-quality branch from d579f73 to 94f2975 Compare August 29, 2023 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants