rulesets/nixpkgs: add merge queue for lints on master #149
Conversation
Allowing any number of `/` needs the `**/*` syntax.
|
The "check references" job fails, but this seems unrelated to this PR. |
| "ref_name": { | ||
| "exclude": [], | ||
| "include": [ | ||
| "~DEFAULT_BRANCH" |
There was a problem hiding this comment.
Just for the record: we will never* be able to add wildcard branches to the include/exclude lists for this ruleset, as GitHub currently doesn't support requiring Merge Queues on a ruleset with wildcard patterns.
All listed branches must either be the default branch, or a non-wildcard pattern.
This means we can't add refs/heads/release-* and would instead have to add (e.g.) refs/heads/release-25.05.
*At least, until GitHub adds support.
There was a problem hiding this comment.
Yeah, that might not be too bad, though. In the current release process, the release team needs to push directly to the release branch anyway, so the merge queue would require them to do that differently. There could be a step in the release process to have the merge queue enabled for the new branch, explicitly.
| "bypass_actors": [ | ||
| { | ||
| "actor_id": 13362067, | ||
| "actor_type": "Team", | ||
| "bypass_mode": "always" | ||
| } | ||
| ] |
There was a problem hiding this comment.
We're not including all of the other checks, yet, because any change included here means it can't be bypassed anymore. We'll need to investigate whether we might still need to be able to do this later on, for now I added the CI team as by-passers as a safety net
This seems reasonable for now. Anyone bypassing the Merge Queue will a) reset the queue, affecting other PRs and b) almost certainly break CI on master.
Have you validated how this interacts with the other ruleset's bypass? E.g. can a committer bypass pr.yml and enqueue a PR?
I assumed that bypassing any rule meant all rules were ignored, even if they come from separate rulesets. I.e. I assume to merge a failing PR, you must also bypass the queue and you cannot enqueue a failing PR? 🤔
So I theorize this may accidentally prevent normal committers from bypassing anything being merged into master.
There was a problem hiding this comment.
Have you validated how this interacts with the other ruleset's bypass? E.g. can a committer bypass
pr.ymland enqueue a PR?
I have not explicitly tested this, no, but ...
I assumed that bypassing any rule meant all rules were ignored, even if they come from separate rulesets 🤔
... my experience with separate rulesets so far has been, that the bypassers only explicitly bypass the rules they are configured for. This is the big advantage compared to classic branch protection rules. I tested it with other rules a while ago, ...
So I theorize this may accidentally prevent normal committers from bypassing anything being merged into master.
... thus I don't expect this to happen. Committers should be able to bypass the required checks to hit the button, thus adding the PR to the queue. They won't be able to bypass the merge queue checks, so they will effectively never be able to bypass the lints. On master, that is.
There was a problem hiding this comment.
Hm. I'm trying to test this in a test org+repo: MattSturgeonOrg/TestRepo#1 (an org is needed to enable Merge Queues)
I have a ruleset that requires a status check (that'll never run), but can be bypassed by users with write: Bypassable.json
I have a second ruleset that requires merge queue: Queue.json
I have a "merge when ready" button, but no "bypass" tickbox:
"Merge when ready" simply enables Auto Merge on the PR:
I also don't get any "bypass" checkbox if I manually fail the required status check:
gh api /repos/MattSturgeonOrg/TestRepo/statuses/155a82d8f0c68c4cd5832899b5685916e7fbf2e2 \
-f 'state=failure' \
-f 'context=foo' \
-f 'description=sent from gh api CLI'There was a problem hiding this comment.
Ah, OK, I see what you mean now. I assume bypassing is still possible via CLI / API merge or so - but it seems like the UI just doesn't handle this case at all.
The UI effectively turns this into a "can't be bypassed".
What if you're allowed to bypass the queue? Does another option appear for you somehow?
There was a problem hiding this comment.
What if you're allowed to bypass the queue? Does another option appear for you somehow?
Yeah, that allows merging directly into the base branch:
There was a problem hiding this comment.
I haven't thought through the tradeoffs fully. With mechanisms like merge queues we won't be able to prove who merged what from the git history, as that will be done by GitHub only (a single point).
If I understand correctly, the "I'll sign my merge commits" approach is mainly to prevent other committers from impersonating you - that's what you are protected against. You can say: Nope, this merge commit is not mine, because I always sign my commits.
Now.. this is only a problem, if pushing directly to the target branch is possible in the first place. If you can't push directly to the target branch, but let GitHub actually create those merge commits, then nobody can impersonate you anyway. At least I can't imagine how, because to impersonate you, I'd need to connect your email address to my account first.
Thus, I think, the case of "CLI merges are disallowed because we use merge queues", but "CLI merges are still possible by bypassing" is the worst case for you:
- You would not be able to consistently sign your commits anymore without restarting merge queues on every commit,
- But others are not stopped from sneaking in a commit pretending to be yours in a direct push, restarting the queue a single time, thus likely not being noticed.
If we strictly disallow CLI merges, this should make signing the merges not required anymore.
Does that make sense or what am I missing?
There's one aspect that I don't quite understand even right now, yet: We do have a requirement for pull requests to the default branch already. Thus, you can't merge arbitrary commits, they need to be in a pull request. When you now merge a PR via CLI, this merge will show up in the PR as well - with your name as the merger.
Now.. if you fake a merge commit from somebody else - will this attribute the merge in the PR to this person as well? I kind of doubt that this will happen. So I assume that in practice it's already impossible to fake a merge commit in another committers name right now, at least when targeting the default branch. (To require PRs and / or enable merge queues for other branches, we'd need to do NixOS/nixpkgs#424345 (comment))
Edit: I now see that this scenario seems to be precisely what you mentioned with "BTW, GitHub has a long history of commonly misattributing who merged which PR [...]". So this might need to be tested explicitly...
There was a problem hiding this comment.
Not really. I just see a difference between GitHub promising that it checked stuff vs. having a permanent auditable cryptographic proof.
There was a problem hiding this comment.
I sympathise with your position, but at a certain point we have to accept that we are using GitHub (warts and all) and may need to occasionally compromise by doing things their way. At least, if we want things like Merge Queues, which I believe most of us do.
If I understand correctly, the "I'll sign my merge commits" approach is mainly to prevent other committers from impersonating you - that's what you are protected against. You can say: Nope, this merge commit is not mine, because I always sign my commits.
I would be very surprised if a bad merge being mis-atributed to you caused any serious damage to your reputation. Maybe I'm naive, but in that scenario you should be able to explain the known issue, reference this conversation, and prior examples of other mis-atributed merges.
Anecdotally, I've never knowingly seen a mis-atributed merge commit created by GitHub; I'd be interested in seeing the examples you mentioned.
My personal view is that even if GitHub does do weird things on occasion, and we miss out on the ability to sign PR merge commits, the benefits of having a Merge Queue are still worth it.
There was a problem hiding this comment.
OK, with this input in mind, I changed the bypassers for the merge queue to all committers. Once we introduce the merge queue, everyone who merges via git's CLI needs to be careful to only do this on non-merge-queue branches and not on master. For merges into master, the regular merge queue button in the PR must be used - otherwise the merge queue will be restarted on each CLI merge.
Note that this will be reported when pushing, too:
remote: Bypassed rule violations for refs/heads/master:
remote:
remote: - Changes must be made through the merge queue
remote:
It doesn't cause an error, though, so it might also just be missed.
There was a problem hiding this comment.
I would be very surprised if a bad merge being mis-atributed to you caused any serious damage to your reputation.
To clarify, that wasn't the point at all. And I didn't mean to argue for or against the queues, as I haven't thought about it much. Just presenting explanations around the question sent at me.
wolfgangwalther
force-pushed
the
merge-queue-master
branch
from
be01b3f to
2bffe93
Compare
|
This currently has conflicts. I will resolve those, once the following two issues are resolved:
|
This requires NixOS/nixpkgs#431146 and enables a very basic merge queue for the master branch in nixpkgs. At this stage, this only runs the "lint" group of checks: treefmt, nixpkgs-vet and the parse check. We should do this right now, because the recent nixfmt 1.0.0 update can easily cause nixfmt regressions after merging a seemingly OK PR - where nixfmt ran before the update, but the would need to run again afterwards.
We're not including all of the other checks, yet, because any change included here means it can't be bypassed anymore. We'll need to investigate whether we might still need to be able to do this later on, for now I added the CI team as by-passers as a safety net
This includes the commit from #146, to avoid merge conflicts. Also, we should probably wait on that to be merged anyway, because it currently causes dependabot to not do updates. This means we're not running Nix 2.30 in GHA, yet, which means we are still subject to random nixpkgs-vet failures. These go away after rerunning the job, but would still kick the PR out of the queue at first. So we might just as well wait for this update to be in.
cc @NixOS/nixpkgs-ci