rulesets/nixpkgs: add merge queue for lints on master #149
Conversation
|
The "check references" job fails, but this seems unrelated to this PR. |
MattSturgeon
left a comment
MattSturgeon
left a comment
There was a problem hiding this comment.
Excited to get merge queues soon 👀
| "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.
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:
|
wolfgangwalther
force-pushed
the
merge-queue-master
branch
from
2bffe93 to
181e0f0
Compare
|
This will enable the I briefly wondered whether this means we should take nixpkgs-vet out of the merge queue for now or make it From my perspective, this PR is ready to go: Let's enable the merge queue for master @NixOS/org! |
| "max_entries_to_merge": 5, | ||
| "min_entries_to_merge_wait_minutes": 5, | ||
| "grouping_strategy": "ALLGREEN", | ||
| "check_response_timeout_minutes": 60 |
There was a problem hiding this comment.
Maybe check if these numbers make sense for Nixpkgs, or try them out and adjust as necessary (can do it more ad-hoc, commit later)
There was a problem hiding this comment.
Yeah, no experience, so left the default here. We should certainly look into this once we have a bit more experience.
|
Not merging while GitHub seems to be dying here in CH, but as soon as it's stable, let's do it. |
|
The merge itself also won't do anything. This needs to be imported manually and be re-exported manually before merge. |
|
I know! :) |
|
This broke: NixOS/nixpkgs#441598 (comment) |
|
nvm |
Strange: that initial attempt didn't trigger the Looks like it's starting to work, I see three things have now been enqueued and each has a workflow run. And as I finish typing this comment: it looks like everything went smoothly with the first few workflow runs! 🎉 |
|
Yes, I derped by copying the rule edit from this commit wrong and didn't notice till it was too late. |
|
Please run the export script, too, to get the latest state of the rulesets back into the repo. |
|
Already done: #160 |
|
Maybe this doesn't fit well with technical discussion, and I don't want to distract anyone who's monitoring here for post-merge issues, but: I'd like to say a big thank you to @wolfgangwalther for all the effort they've put into getting this across the line. Also thanks to @winterqt for applying the changes to the ruleset and monitoring for issues. And finally, thanks to everyone else for their reviews, feedback, etc. Hopefully this continues to go smoothly, but if not I have every faith we will identify whatever issues exist and iron them out. |
|
A few notes from looking at things:
|
8 participants
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