workflows/pr: preparation for required status checks#417957
workflows/pr: preparation for required status checks#417957wolfgangwalther merged 5 commits intoNixOS:masterfrom
Conversation
nix-owners
bot
requested review from
LeSuisse,
Mic92,
azuwis,
infinisil,
mweinelt,
philiptaron,
risicle and
zowoq
github-actions
bot
added
10.rebuild-darwin: 0
Another way of dealing with that could be to run a one-time CI job that goes back a certain amount of time and manually creates a status check with the same name for all PRs which had passed all jobs. We'd probably want to limit this, because after a certain time X, a PR can be very well considered out of date and CI should run again. But this would allow for a smoother transition. |
|
Applied the wording improvements locally, but will only push once we bikeshedded the name enough - every push here is heavy with > 40 jobs running. |
wolfgangwalther
force-pushed
the
ci-required-status-check
branch
from
f68c81b to
3aabef1
Compare
Some naming improvements after we introduced the PR / Push workflows and small refactors.
This is only a refactor at this stage, but split into a separate commit for better review. It's the base for the next two commits.
This fixes a problem where each workflow would get their own merge commit. This happens frequently when the target branch is merged into a the same time, different workflows in the same run will run get-merge-commit at different times and thus have different merge commits. Since the jobs don't really depend on each other, this doesn't cause practical problems, yet. But it has already led to strange CI failures in a still unmerged PR, which can be prevented from happening with this clean approach. And yes, this saves a few API calls on every run.
This allows *not* depending on those two jobs with the required status checks in the next commit, which wouldn't really make sense. If labeling or pinging maintainers fails for obscure reasons or because the GitHub API is down, a PR might still pass all other tests and be merge-eligible.
This job serves as a target for the "Required Status Checks" branch protection rule.
wolfgangwalther
force-pushed
the
ci-required-status-check
branch
from
3aabef1 to
caf4ced
Compare
MattSturgeon
left a comment
MattSturgeon
left a comment
There was a problem hiding this comment.
Went through each commit again. Everything seems clear, and I don't see any obvious issues.
I'd say let's test this out 🚀
| # This job's only purpose is to serve as a target for the "Required Status Checks" branch ruleset. | ||
| # It "needs" all the jobs that should block merging a PR. | ||
| # If they pass, it is skipped — which counts as "success" for purposes of the branch ruleset. | ||
| # However, if any of them fail, this job will also fail — thus blocking the branch ruleset. | ||
| no-pr-failures: | ||
| # Modify this list to add or remove jobs from required status checks. | ||
| needs: | ||
| - check | ||
| - lint | ||
| - eval | ||
| - build | ||
| # WARNING: | ||
| # Do NOT change the name of this job, otherwise the rule will not catch it anymore. | ||
| # This would prevent all PRs from merging. | ||
| name: no PR failures | ||
| if: ${{ failure() }} | ||
| runs-on: ubuntu-24.04-arm | ||
| steps: | ||
| - run: exit 1 |
There was a problem hiding this comment.
I assume, before merging, we should confirm in NixOS/org#130 that we are definitely planning to enable required status checks at some point?
I can't see why we wouldn't want to... But if the proposal was blocked for any reason, then we wouldn't need to merge this job.
Alternatively, we could merge it optimistically so that we can see it working in production.
Once we've proven the CI is working, it will be easier to argue in favour of requiring it.
With this approach, we can still remove the job later if the proposal is blocked or we change our minds (for whatever reason).
There was a problem hiding this comment.
Alternatively, we could merge it optimistically
I'd say let's do that, yes. It's not going to do any harm if unused - especially with the new naming that doesn't say anything about "required" anymore.
github-actions
bot
added
the
12.approvals: 1
|
Successfully created backport PR for |
|
Successfully created backport PR for |
github-actions
bot
added
the
8.has: port to stable
3 participants
This adds a
PR / required to mergejob, which:Once merged, this job can be used for the "Required Status Checks" branch protection rule. Once we enable that, merging PRs will only be possible when CI succeeds.
Before adding this job, I restructed the PR workflow slightly: Labels and Reviewers were previously part of Eval, but that would make them required as well. By moving them up into the PR workflow, they can be optional for the
requiredjob. They don't really need to block a merge.While doing that, I also fixed the merge commit inconsistencies described in #406825 (comment).
Roadmap to Required Status Checks:
I'm not sure if we need to wait and if yes, how long, before enabling the checks. Every PR that ran its CI the last time before the merge of this PR, will be blocked from merging until CI is triggered again.
Note: Any discussion around if and how to implement the branch protection rules is better placed in NixOS/org#130.
Things done
Add a 👍 reaction to pull requests you find important.