Introduce a gate/check GHA job

[webknjaz]

This adds a GHA job that reliably determines if all the required dependencies have succeeded or not.

It also allows to reduce the list of required branch protection CI statuses to just one — check. This reduces the maintenance burden by a lot and have been battle-tested across a small bunch of projects in its action form and in-house implementations of other people.

This action is now in use in aiohttp (and other aio-libs projects), CherryPy, some of the Ansible repositories, pip-tools, all of the jaraco's projects (like setuptools, importlib_metadata), some of the hynek's projects (like attrs, structlog), some PyCQA, PyCA, PyPA and pytest projects, a few AWS Labs projects. Admittedly, I maintain a few of these but it seems to address some of the pain folks have: jaraco/skeleton#55 (comment).

I saw a few attempts to reduce the maintenance burden for GHA and figured
this might be useful for CPython too, which is why I'm submitting this patch.
Looking forward to hearing what you think!

The story behind this is explained in more detail at https://github.com/marketplace/actions/alls-green#why.

[webknjaz]

This is mandatory because otherwise it'll get a skipped status in some cases and the branch protection sees that as skipped == success which is undesired.

[webknjaz]

This setting tells the action that if the listed jobs (comma-separated) got skipped, that's okay. But the jobs are allowed to be skipped based on the same condition they've got set. If the first job requests test runs, this list will compute as empty and none of the jobs will be allowed to be skipped (meaning that skips would turn into failures).

[hugovk]

Do I understand this right?

• If check-docs starts, it must pass; if they skip, that's fine.

• If check_generated_files or any of the other jobs in its group start, they must all pass; if they all skip, that's fine.

• If test_hypothesis starts, it must pass; if they skip, that's fine.

[webknjaz]

@hugovk almost: this is paired with the setting above ☝️ — essentially, if something is allowed to be skipped, and it is skipped, the action will treat it as non-failure.
But if something is allowed to be skipped and is not skipped, whether its failure is to be treated as such is controlled by the allowed-failures input.

Also, not they are not grouped inside the action, the templating here just produces a comma-separated list of things and the action doesn't know it was in some group externally. So yes, such a relation kinda exists, but I would say that it's indirect.

And since there's no “groups”, if any of the jobs that are allowed to be skipped run, each individual job is evaluated separately from the “group”, based on the fact of failures being allowed or not.

N.B. Whether a job is allowed to fail is also contributed by the continue-on-error setting, which is mostly useful for controlling individual matrix jobs. If a job was allowed to fail prior to this one, the action will not “see” if it was red and treat it as green.

[webknjaz]

• If test_hypothesis starts, it must pass; if they skip, that's fine.

With the context I added earlier, seeing that test_hypothesis is listed in allowed-failures unconditionally, it will never cause failures. This reflects the current branch protection configuration as seen in the Checks widget in PRs.

[hugovk]

Thanks for the PR!

Some points to consider:

1. This changes the security levels: a small group of 6 org owners + 8 repo admins can change the required checks via repo settings. With this PR, any of the ~90 active core devs can merge changes to this file. Is that okay? I think it's probably fine, we're not deploying or anything from here, but let's make sure.

2. Whilst this does reduce maintenance burden in administering which checks are required or not, is that something that needs changing often?
   For projects that test a big matrix of Python versions, it's definitely a plus: no need to add/remove version-specific checks via the interface once or twice a year. Maybe less so here?

3. Has this ~14 vs. ~90 been a bottleneck in the past?

[webknjaz]

@hugovk these are good points and I haven't considered some of them.

1. I should probably document the security considerations in README. It seems like this might be solvable by requiring reviews from code owners, for example. This should limit the scope of the CI updates made flying under the radar. It also occurred to me that the repository settings updates happen unnoticed, with no traceability for the larger public — only the admins would be able to see this in the security audit log and only if they specifically search for such changes. Whereas with the branch-protection-as-a-code, it's all there, verifiable via https://github.com/python/cpython/commits/main/.github/workflows/build.yml.
2. I've been offering similar PRs to many projects with matrixes of various sizes, and I've seen the maintainers forgetting to add new matrix factors. After all, it's a mental overhead having to memorize to perform some extra action items far from the place where you're editing the source code, especially since they aren't even in the code.
3. That's not something I can observe/verify 🤷‍♂️ — maybe @ezio-melotti knows?

[ambv]

I don't know if this is worth it for CPython as a number of jobs we require are outside of GitHub Actions (CLA, Issue Number, and News). So this check will increase complexity but won't actually solve the problem of bringing all required checks in one place.

[CAM-Gerlach]

@hugovk , as you've worked a lot of CI stuff lately across the various Python-org repos, do you have further feedback on this?

1. I should probably document the security considerations in README. It seems like this might be solvable by requiring reviews from code owners, for example.

Just to note, that would be a pretty huge change in terms of workflow for this repo, since CODEOWNERS is used here for much more than just who is required to review certain changes. I'm not sure that itself is a dealbreaker for this proposal, but certainly something to keep in mind.

[zware]

Agreed with @ambv: this looks like significant added complexity for not much actual gain in this project. It looks like a nice idea for some other setups, but doesn't feel like a great fit for us here. It's also possible I'm just missing the point, though :)

[brettcannon]

1. I should probably document the security considerations in README. It seems like this might be solvable by requiring reviews from code owners, for example.

Just to note, that would be a pretty huge change in terms of workflow for this repo, since CODEOWNERS is used here for much more than just who is required to review certain changes.

FYI the SC has explicitly stated individuals cannot act as owners on any one piece of code (i.e. we all own the code), so requiring CODEOWNERS approval is a non-starter IMO.

[ambv]

Alright, so I'm closing this based on the latest feedback. Thanks for putting this together, @webknjaz, and sorry it wasn't a good match for us at this time!

[ambv]

Given that our requirements changed, with Auto-merge enabled, and more checks existing now, I would like to reconsider this PR for inclusion.

@webknjaz, would you be so kind as to adapt this to the current state of main?

[webknjaz]

@ambv so I just realized that the docs are its own workflow. The only way this is gonna work is wiring that in as a reusable workflow.

[webknjaz]

A quick summary of our in-person interaction. Łukasz and I agreed to implement the following:

• rename the check job ID and name
• convert the docs workflow into a reusable one
• integrate that into the main build workflow, making use of the dynamic change detection that's already present there
• move the "allowed failures" declarations to the check

@ambv do we need to do the same to the build_msi workflow?

[webknjaz]

@ambv@hugovk I think this is ready.

[webknjaz]

@ambv@hugovk let's see if this is ready now...

[hugovk]

Thanks! Let's give @ambv some time to check before merging.

[webknjaz]

@hugovk IIRC Łukasz and I talked about merging this PR but not changing the branch protection settings for a while so the check name would start being reported in the Checks widget on old PR updates, otherwise they'll get blocked until updated.

[hugovk]

Thanks again!

[bedevere-bot]

GH-107114 is a backport of this pull request to the 3.12 branch.

[bedevere-bot]

GH-107115 is a backport of this pull request to the 3.11 branch.

[webknjaz]

UPD: this has been backported to 3.12 and 3.11 and the branch protection changed accordingly.