Introduce a gate/check GHA job by webknjaz · Pull Request #97533 · python/cpython · GitHub

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.

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.

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).

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.

@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.

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.

Thanks for the PR!

Some points to consider:

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.
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?
Has this ~14 vs. ~90 been a bottleneck in the past?

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

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.
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.
That's not something I can observe/verify 🤷‍♂️ — maybe @ezio-melotti knows?

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.

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

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.

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 :)

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.

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!

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?

@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.

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?

@ambv @hugovk I think this is ready.

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

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, conda, coveragepy, Open edX, Towncrier some of the Ansible repositories, pip-tools, all of the jaraco's projects (like `setuptools`, `importlib_metadata`), some of 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 figured, this might be useful for CPython too, which is why I'm submitting this patch. The story behind this is explained in more detail at https://github.com/marketplace/actions/alls-green#why. Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>

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

@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.

Thanks again!

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com> (cherry picked from commit e7cd557) Co-authored-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua>

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

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

(cherry picked from commit e7cd557)

This fixes an incorrect conflict resolution problem that happened in 0cdc3a5 while backporting PR python#97533 as PR python#107115 (merged prematurely). This problem caused GitHub Actions CI/CD to crash while attempting to load the workflow file definition, preventing the jobs that are defined in `.github/workflows/build.yml` from actually starting.

This fixes an incorrect conflict resolution problem that happened in 0cdc3a5 while backporting PR #97533 as PR #107115 (merged prematurely). This problem caused GitHub Actions CI/CD to crash while attempting to load the workflow file definition, preventing the jobs that are defined in `.github/workflows/build.yml` from actually starting.

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

bedevere-bot added the awaiting review label Sep 24, 2022

webknjaz commented Sep 24, 2022

View reviewed changes

Comment thread .github/workflows/build.yml Outdated

webknjaz commented Sep 24, 2022

View reviewed changes

webknjaz force-pushed the maintenance/gha-check branch from 1e1bf94 to 75801c3 Compare October 14, 2022 20:43

webknjaz requested a review from ezio-melotti as a code owner October 14, 2022 20:43

webknjaz force-pushed the maintenance/gha-check branch from 75801c3 to e441450 Compare October 14, 2022 20:49

ezio-melotti added skip issue skip news labels Nov 11, 2022

ezio-melotti requested a review from hugovk November 11, 2022 00:45

webknjaz force-pushed the maintenance/gha-check branch 2 times, most recently from 46d97b5 to b436563 Compare January 6, 2023 19:19

ambv reviewed Mar 28, 2023

View reviewed changes

Comment thread .github/workflows/build.yml Outdated

ambv closed this Mar 28, 2023

ambv reopened this Apr 26, 2023

webknjaz force-pushed the maintenance/gha-check branch from 2e9f6a1 to 0abb403 Compare April 26, 2023 20:38

webknjaz force-pushed the maintenance/gha-check branch from 0abb403 to 9ac328e Compare April 26, 2023 21:13

ambv reviewed Apr 26, 2023

View reviewed changes

Comment thread .github/workflows/build.yml Outdated

webknjaz force-pushed the maintenance/gha-check branch 2 times, most recently from 46004d0 to 71f3a64 Compare April 26, 2023 23:41

webknjaz requested a review from AA-Turner May 28, 2023 23:51

webknjaz force-pushed the maintenance/gha-check branch from 136fda2 to 79f84d7 Compare June 26, 2023 16:46

webknjaz force-pushed the maintenance/gha-check branch from 79f84d7 to d363de3 Compare June 26, 2023 17:22

AlexWaygood removed their request for review June 26, 2023 17:23

hugovk reviewed Jun 26, 2023

View reviewed changes

Comment thread .github/workflows/build.yml Outdated

hugovk reviewed Jun 26, 2023

View reviewed changes

Comment thread .github/workflows/build.yml Outdated

webknjaz requested a review from hugovk June 26, 2023 19:34

webknjaz force-pushed the maintenance/gha-check branch from bfa9d60 to 0fa6164 Compare June 26, 2023 19:34

hugovk approved these changes Jun 30, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jun 30, 2023

hugovk merged commit e7cd557 into python:main Jul 6, 2023

bedevere-bot removed the awaiting merge label Jul 6, 2023

ambv pushed a commit that referenced this pull request Jul 23, 2023

[3.12] Introduce a gate/check GHA job (GH-97533 ) (#107114 )

18c68ec

(cherry picked from commit e7cd557)

ambv pushed a commit that referenced this pull request Jul 23, 2023

[3.11] Introduce a gate/check GHA job (GH-97533 ) (#107115 )

0cdc3a5

(cherry picked from commit e7cd557)

webknjaz mentioned this pull request Jul 23, 2023

[3.11] 🔥 Drop hypothesis job dep @ GHA #107128

Merged

hugovk mentioned this pull request Jan 17, 2024

Should we make the Docs CI mandatory on the Python main branch? python/core-workflow#424

Closed

webknjaz mentioned this pull request Apr 24, 2025

Please support something like "allow-failure" for a given job actions/runner#2347

Open

+                  - name: Decide whether the needed jobs succeeded or failed
+                    uses: re-actors/alls-green@198badcb65a1a44528f27d5da555c4be9f12eac6
+                    with:
+                      allowed-skips: >-

                     run: xvfb-run make buildbottest TESTOPTS="-j4 -uall,-cpu"
+                check:  # This job does nothing and is only used for the branch protection
+                  if: always()

Introduce a gate/check GHA job by webknjaz · Pull Request #97533 · python/cpython · GitHub

Uh oh!

Uh oh!

Introduce a gate/check GHA job - #97533

Introduce a gate/check GHA job#97533hugovk merged 1 commit intopython:mainpython/cpython:mainfrom webknjaz:maintenance/gha-checkwebknjaz/cpython:maintenance/gha-checkCopy head branch name to clipboard

Conversation

webknjaz commented Sep 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

webknjaz Sep 24, 2022 • 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!

webknjaz Sep 24, 2022

Choose a reason for hiding this comment

Uh oh!

hugovk Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

webknjaz Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

webknjaz Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

hugovk commented Nov 11, 2022

Uh oh!

webknjaz commented Dec 2, 2022

Uh oh!

Uh oh!

ambv commented Mar 28, 2023

Uh oh!

CAM-Gerlach commented Mar 28, 2023

Uh oh!

zware commented Mar 28, 2023

Uh oh!

brettcannon commented Mar 28, 2023

Uh oh!

ambv commented Mar 28, 2023

Uh oh!

ambv commented Apr 26, 2023

Uh oh!

webknjaz commented Apr 26, 2023

Uh oh!

Uh oh!

webknjaz commented Apr 26, 2023

Uh oh!

webknjaz commented May 29, 2023

Uh oh!

webknjaz commented Jun 26, 2023

Uh oh!

Uh oh!

Uh oh!

hugovk left a comment

Choose a reason for hiding this comment

Uh oh!

webknjaz commented Jun 30, 2023

Uh oh!

hugovk commented Jul 6, 2023

Uh oh!

bedevere-bot commented Jul 23, 2023

Uh oh!

bedevere-bot commented Jul 23, 2023

Uh oh!

webknjaz commented Jul 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Introduce a gate/check GHA job#97533
hugovk merged 1 commit into
python:mainfrom
webknjaz:maintenance/gha-check

webknjaz commented Sep 24, 2022 •
edited
Loading

webknjaz Sep 24, 2022 •
edited
Loading