-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add mergify to nixpkgs #39
Comments
If mergify is ever deciding to shutdown on us, we can switch to https://github.com/rust-lang/bors for merge queues. At the moment this seems unlikely because it has a reasonable business model and supporting opensource projects is cheaper marketing compared to online advertisement. Bors is still in development at the time by the rust community and the infra team currently has other issues to solve. |
Another interesting optimization is that we could use merge queues to already compute output paths before the commits are merged into master. Currently we wait in pull request actions for evaluation in master. This is ok in most cases as the merge base is usually started a bit earlier than the github actions on a pull request, but with merge queues we could close this gap and also run actions less often as multiple pull requests are included in a single merge using batching. |
Does mergify offer any features (useful to nixpkgs) that GitHub's native merge queue does not? We've been using mergify on nixvim for a while, and while generally it's nice, it can be a little clunky at times; in ways that I suspect github's built-in solution may handle better. EDIT:
|
would it be possible to create a "required checks are green" check ( |
@MattSturgeon the squash/rebase commands are nice and you can trigger "queue" commands based on label's i.e. have a backport be automatically merged. The latter one doesn't work github doesn't out-of-the box, because you need a github app token for that to be able to trigger github actions ci. |
We have some checks that can soft-fail i.e. cherry-picks that are not the same as the original pull request: https://github.com/NixOS/nixpkgs/blob/master/.github/workflows/check-cherry-picks.yml. We wouldn't be able to merge in this case. |
I'm fine with this as an experiment. We could enable this if others from @NixOS/org also agree, thoughts? |
NixOS/nixpkgs#364540 is a good example of something that would've been prevented with this |
@winterqt You 👎'd, can you give a reason? |
Ideally also an alternative. |
Can we get this going? We can just run this as a beta test and always disable this if things go downhill. |
If we enable it for testing while allowing manual merges through GitHub UI, how will it deal with those? I would expect them to invalidate the merge queue. |
Mergify handles that gracefully. Manual merges would still be able to break tests that have been tested inside the merge queue correctly, but this is not different from the status quo. |
Let's get this going if there aren't any blockers. |
How does it handle that, though? If it needs to re-run the CI job on the merge queue (this is what marge-bot does IIRC), it would probably never be able to merge anything since stuff is merged to Nixpkgs all the time. Or does it just ignore the state of target branch when merging the queue? That would indeed not be any different from status quo but I could not find any docs. Another question I had was about who will be able to trigger merges but it looks like it is able to map GitHub permissions sensibly. |
I enabled the app so we can find out in practice. Ping here if you find any issues that require a revert. |
Please annonnce this somewhere. I only came across this by chance. Ideally also add a small explanation/guide for those of us who haven't used this before. |
Are you sure this works ? |
The setup needs a bit more work before it's considered ready to be used more widely (or reverted if we find irreconciliable issues). I closed the issue because the purpose of the org repo is to handle permission requests. There should probably be another meta ticket on nixpkgs to track the state of the feature overall. |
Looks like it is extremely spammy. Sorry @adrianvp Edit: Looking at NixOS/nixpkgs#366326 (comment), it does not handle moving base branch as I was afraid – the queue will be reset on every push to master. |
Note that mergify was disabled again because of the spam |
What was the decision making process for enabling this? Do we have defined authorities and responsibilities? People with enough foresight to prevent the inevitable? I find it remarkable how we somehow spent half a year doing this voting dance and steering committee stuff, just to then ignore it and continue with YOLOing impactful decisions as if nothing had happened. |
Lines 29 to 37 in d5a00a3
The actual discussion and process is all in this thread. |
I find stuff like this to be a lot less critical than the SC. Its job is to make decisions that affect the community as a whole and are often controversial. I generally don't think we need to have a huge decision process around trying things that are just clear improvements and disadvantage nobody. "Let's try mergify and see if we like it" simply is not controversial. This was enabled, it had major flaws, it was noticed and quickly disabled again. There wasn't any real damage to speak of other than a bit of PR spam and a bit of confusion. I personally don't find it to be all that critical. Once the major flaw is fixed, we can try again and perhaps actually test it then. |
It would be great if you could actually provide reasoning instead of post emojis, because this is nothing we can work with or improve on. This discussion culture is frustrating. It's always easy if something goes wrong to just write "told you though" after the fact, even what went wrong here is probably not what you would have brought up anyway. |
Here is a quick retrospective ImpactIn terms of impact on this experiment, I had a quick look and here are some numbers: Mergify created opened 59 PRs on the repo. It burned additional CI minutes. To get an idea, I pulled some number of GitHub actions minutes spent per day. The UI is quite bad so getting those 4 numbers already took me 20min. We don't have enough data to know how it compares to normal variations.
Things that went wellWe found out about mergify's behaviour quickly. The impact was within the bounds I had envisioned. nixpkgs is so unique that it would have been hard to find out about this issue with a syntetic setup. Things that went not so wellOnce enabled, I was expecting @Mic92 to take over. Also didn't expect project members to be so enthusiastic and start using the feature :) That was a communication issue on my part. The next day I was busy, but should have keep a closer look at what Mergify was doing. I also see some uncertainty on how the decision process is happening. Let's do a retrospective to improve this and clarify the decision-making process of the org team. I'd like to get to a place where we can take quick decisions, while keeping the amount of uncertainty as low as possible. @NixOS/org https://crab.fit/happy-halloween-crab-120024 |
@Mic92 It seems you're confusing @piegamesde for me for some reason? I didn't have time to write up my objections, but I was going to. The emoji reaction was a quick way of trying to slow things down. I did mention in our internal Matrix room that I was going to write up proper objections so we could discuss, but @zimbatm unfortunately didn't see that message before adding it anyways. |
Yes sorry. |
So can we, enable GitHub's own merge queue feature? Doesn't seem like nearly as noisy as Mergify. It's tightly integrated with GitHub. |
Skimming through the docs, it seems to me that:
Footnotes
|
Well, we could already start modifying github actions to add merge queue events, it doesn't require any org permissions and we can test and review those changes just like any other pull request that changes CI. |
They aren't in GitHub's impl. However attempting to combine both queued and unqueued merges was the reason why Mergify's queue kept getting "reset" anyway. So if we use either queue implementation, it'd have to be used exclusively for practical reasons. |
We already deployed mergify in nixos-hardware: NixOS/nixos-hardware#1249 and in nix https://github.com/NixOS/nix/blob/master/.mergify.yml
This pull requests adds support for it in nixpkgs. Mergify adds support for declarative merge queues to nixpkgs. Merge queues make sure that pull request are tested against the latest base branch. The advantage over github's own merge queues is that, the configuration is part of the repository and therefore does not require a repository admin to keep "Required Checks" in sync with recently added CI checks.
Another advantage mergify has it's that it is a github app. There are many cases where github won't run checks on automated created pull requests. With mergify this is not an issue. If we are happy with the workflow introduced mergify, we can proceed adding it to
CONTRIBUTING.md
.We have a similar feature in merge-bot. However it doesn't work very reliable. With mergify, we can later make
merge-bot
just add a label remove the complicated code and database integration we have just now.The text was updated successfully, but these errors were encountered: