Unify GitHub Actions branch-protection run logic#15794
Conversation
jakelishman
added
the
type: qa
jakelishman
added
the
Changelog: None
|
One or more of the following people are relevant to this code:
|
jakelishman
force-pushed
the
versionless-ci
branch
16 times, most recently
from
7f94c7c to
7261d64
Compare
Currently, whenever we need to bump the runner image or Python versions used in the pull-request and merge-queue Actions workflows, we then have to spend a lot of time updating the branch-protection rules in the GitHub web interface (which is fiddly and error prone), and then update every active PR to ensure it triggers the new rules and can move into the merge queue. Neither of these two things ought to be necessary: * branch-protection rules _can_ be configured in code. While this does technically relax the number of people who can modify the final branch-protection permissions (by nature of approving reviews to change the `branch-protection.yml` file), in practice nothing has actually changed, because it was always possible to simply stub out the required rules. * if a PR is not _already_ in the merge queue while we are changing the branch-protection rules, but has already passed the (previous) set of protection rules, it will still have to pass the new rules on entry to the merge queue. This commit centralises all the actual branch-protection rules into a source-control-tracked in-code configuration. The separate `on-merge-queue.yml` and `on-pull-request.yml` files are unnecessary because they were necessarily largely the same already in order for the branch protection to function correctly, so were duplicating the same configuration. In addition, we provide a single in-code "job" that does nothing but depend on all other branch-protection rules. This lets us specify only that job in the branch-protection rules in the GitHub web interface, vastly simplifying the opaque web-only configuration, and making it no longer vary based on the names of individual subjobs. We also previously spread the configuration of runner-image and Python-version selection into many places across many files, making it rather hard to update when the Python versions changed. Here, we centralise it (at least for the `branch-protection.yml` file) into a single place, which is easier to update. This _can_ be done with variables in the GitHub web interface, but we want to keep them in code to keep the same system of source-control history as other parts of the CI configuration. Other files are not updated to use a centralised configuration as part of this PR; we could modify the system to have a shared `config.yml` job that does nothing but populate a list of outputs with suitable values to achieve this.
jakelishman
force-pushed
the
versionless-ci
branch
from
7261d64 to
dde7359
Compare
|
Ok, I think I got this to the place I wanted it to be in. |
mtreinish
left a comment
mtreinish
left a comment
There was a problem hiding this comment.
I think this LGTM, I still find a bit unfortunate that the only way to get a grouping like this is to create a dummy job that depends on all the jobs we actually care about. I can't think of a better solution given the limits of the github platform. I've just left a couple of inline comments.
| outputs: | ||
| python-old: '3.10' | ||
| python-new: '3.13' | ||
| runner-linux-x86_64: 'ubuntu-latest' | ||
| runner-linux-arm64: 'ubuntu-24.04-arm' | ||
| runner-macos-arm64: 'macos-15' | ||
| runner-windows-x86_64: 'windows-latest' |
There was a problem hiding this comment.
I guess maybe for a follow up something we could add is a user input trigger on this to enable manually running a job with different options if we wanted. But definitely not in scope for this PR and probably not even in this rule. But the comment about the web interface made met think about it.
There was a problem hiding this comment.
oh that's an interesting idea. I'll have to think about it, but there might be a way. The next step after this one is probably to move this "job" into its own file so other files can depend on it too, and maybe part of that will make it easier.
| name: Integration Neko | ||
| needs: config | ||
| runs-on: ${{ needs.config.outputs.runner-linux-x86_64 }} | ||
| steps: | ||
| - uses: Qiskit/qiskit-neko@main | ||
| with: | ||
| test_selection: "terra" |
There was a problem hiding this comment.
I guess you didn't feel like this needed it's own file anymore since it's just calling a single action?
There was a problem hiding this comment.
I mean, it already is its own file, it's just that the file is in a different repo haha. The old neko.yml file had one extra code line, and if that was necessary I'd have left it in its own file, but it was just an awkward trick left over from the Qiskit 0.x -> 1.0 transition.
| push: | ||
| branches: [main, "stable/*"] |
There was a problem hiding this comment.
Do we have a post merge job anywhere anymore? I think it's still valuable to have a quick smoke test job after the merge is complete. It's unlikely to ever catch anything, it's just a sanity check that github did the merge for us correctly.
There was a problem hiding this comment.
Coveralls still has a push trigger which involves running the tests, which I think would be enough?
Trying to protect against GitHub failing to do the ff-only merge queue and branch protection properly feels like a bit of a fool's errand - if we can't trust the basic git operations, I'm not sure we can trust the CI either.
There was a problem hiding this comment.
I mean I trust nothing they provide as a general rule. But as long as we're running something that will fail if it doesn't compile or pass all the tests I'm happy.
mtreinish
left a comment
mtreinish
left a comment
There was a problem hiding this comment.
My open questions have all been answered, so lets move forward with this now
When we updated the branch-protection rules in Qiskitgh-15794[^1], we set the "finalize" job to require all the previous runs. However, if a previous run fails, then this job does not report another failure status, just a "skipped". "Skipped" counts as a success state for evaluating branch-protection rules, so the previous form was not properly protecting the branch. [^1]: 913091b: Unify GitHub Actions branch-protection run logic
When we updated the branch-protection rules in Qiskitgh-15794[^1], we set the "finalize" job to require all the previous runs. However, if a previous run fails, then this job does not report another failure status, just a "skipped". "Skipped" counts as a success state for evaluating branch-protection rules, so the previous form was not properly protecting the branch. [^1]: 913091b: Unify GitHub Actions branch-protection run logic
When we updated the branch-protection rules in gh-15794[^1], we set the "finalize" job to require all the previous runs. However, if a previous run fails, then this job does not report another failure status, just a "skipped". "Skipped" counts as a success state for evaluating branch-protection rules, so the previous form was not properly protecting the branch. [^1]: 913091b: Unify GitHub Actions branch-protection run logic
3 participants
Currently, whenever we need to bump the runner image or Python versions used in the pull-request and merge-queue Actions workflows, we then have to spend a lot of time updating the branch-protection rules in the GitHub web interface (which is fiddly and error prone), and then update every active PR to ensure it triggers the new rules and can move into the merge queue.
Neither of these two things ought to be necessary:
branch-protection rules can be configured in code. While this does technically relax the number of people who can modify the final branch-protection permissions (by nature of approving reviews to change the
branch-protection.ymlfile), in practice nothing has actually changed, because it was always possible to simply stub out the required rules.if a PR is not already in the merge queue while we are changing the branch-protection rules, but has already passed the (previous) set of protection rules, it will still have to pass the new rules on entry to the merge queue.
This commit centralises all the actual branch-protection rules into a source-control-tracked in-code configuration. The separate
on-merge-queue.ymlandon-pull-request.ymlfiles are unnecessary because they were necessarily largely the same already in order for the branch protection to function correctly, so were duplicating the same configuration. In addition, we provide a single in-code "job" that does nothing but depend on all other branch-protection rules. This lets us specify only that job in the branch-protection rules in the GitHub web interface, vastly simplifying the opaque web-only configuration, and making it no longer vary based on the names of individual subjobs.We also previously spread the configuration of runner-image and Python-version selection into many places across many files, making it rather hard to update when the Python versions changed. Here, we centralise it (at least for the
branch-protection.ymlfile) into a single place, which is easier to update. This can be done with variables in the GitHub web interface, but we want to keep them in code to keep the same system of source-control history as other parts of the CI configuration. Other files are not updated to use a centralised configuration as part of this PR; we could modify the system to have a sharedconfig.ymljob that does nothing but populate a list of outputs with suitable values to achieve this.Summary
Details and comments
We're about to have to change the branch-protection rules and deal with all the pain of that in order to merge #15224, so we might as well use the same time to make it so we don't have to go through the same pain again in the future.