Skip to content
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

Closed
Mic92 opened this issue Nov 30, 2024 · 32 comments
Closed

Add mergify to nixpkgs #39

Mic92 opened this issue Nov 30, 2024 · 32 comments

Comments

@Mic92
Copy link
Member

Mic92 commented Nov 30, 2024

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.

@Mic92
Copy link
Member Author

Mic92 commented Nov 30, 2024

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.

@Mic92
Copy link
Member Author

Mic92 commented Dec 1, 2024

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.

@MattSturgeon
Copy link

MattSturgeon commented Dec 2, 2024

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:

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.

@sersorrel
Copy link

would it be possible to create a "required checks are green" check (on: check_run or on: status, maybe), that is the only one actually monitored by the github "Required Checks" setting? then the configuration for "required" checks would effectively be part of the repository.

@Mic92
Copy link
Member Author

Mic92 commented Dec 3, 2024

@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.

@Mic92
Copy link
Member Author

Mic92 commented Dec 3, 2024

would it be possible to create a "required checks are green" check (on: check_run or on: status, maybe), that is the only one actually monitored by the github "Required Checks" setting? then the configuration for "required" checks would effectively be part of the repository.

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.

@infinisil
Copy link
Member

I'm fine with this as an experiment. We could enable this if others from @NixOS/org also agree, thoughts?

@infinisil
Copy link
Member

NixOS/nixpkgs#364540 is a good example of something that would've been prevented with this

@infinisil
Copy link
Member

@winterqt You 👎'd, can you give a reason?

@Mic92
Copy link
Member Author

Mic92 commented Dec 13, 2024

@winterqt You 👎'd, can you give a reason?

Ideally also an alternative.

@JohnRTitor
Copy link

Can we get this going? We can just run this as a beta test and always disable this if things go downhill.

@jtojnar
Copy link
Member

jtojnar commented Dec 16, 2024

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.

@Mic92
Copy link
Member Author

Mic92 commented Dec 16, 2024

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.

@zimbatm
Copy link
Member

zimbatm commented Dec 16, 2024

Let's get this going if there aren't any blockers.

@jtojnar
Copy link
Member

jtojnar commented Dec 16, 2024

Mergify handles that gracefully.

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.

@zimbatm
Copy link
Member

zimbatm commented Dec 16, 2024

I enabled the app so we can find out in practice. Ping here if you find any issues that require a revert.

@zimbatm zimbatm closed this as completed Dec 16, 2024
@Atemu
Copy link
Member

Atemu commented Dec 17, 2024

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.

@GaetanLepage
Copy link

Are you sure this works ?
NixOS/nixpkgs#365782 (comment)
It seems to be taking a very long time.

@zimbatm
Copy link
Member

zimbatm commented Dec 17, 2024

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.

@jtojnar
Copy link
Member

jtojnar commented Dec 18, 2024

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.

@infinisil
Copy link
Member

Note that mergify was disabled again because of the spam

@piegamesde
Copy link
Member

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.

@MattSturgeon
Copy link

MattSturgeon commented Dec 18, 2024

What was the decision making process for enabling this? Do we have defined authorities and responsibilities?

Org owners need approval from at least one other org owner to take care of implementing
higher-impact changes that are _not controversial_, such as:
- Administer unmaintained repos, such as:
- Performing maintenance.
- Giving commit access to trusted people that offer maintenance.
- Archiving if appropriate.
- Create GitHub apps to unblock automation.
- Changes to the structure and CI of the [org repository](https://github.com/NixOS/org).
- Content updates to the [GitHub organisation documentation](./github.md).

The actual discussion and process is all in this thread.

@Atemu
Copy link
Member

Atemu commented Dec 18, 2024

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.

@Mic92
Copy link
Member Author

Mic92 commented Dec 19, 2024

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.

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.

@zimbatm
Copy link
Member

zimbatm commented Dec 19, 2024

Here is a quick retrospective

Impact

In 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.

  • 12/14/2024 - 45,164
  • 12/15/2024 - 46,624
  • 12/16/2024 - 51,545
  • 12/17/2024 - 62,987

Things that went well

We 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 well

Once 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

@winterqt
Copy link
Member

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.

@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.

@Mic92
Copy link
Member Author

Mic92 commented Dec 19, 2024

@Mic92 It seems you're confusing @piegamesde for me for some reason?

Yes sorry.

@JohnRTitor
Copy link

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.

@jtojnar
Copy link
Member

jtojnar commented Jan 8, 2025

Skimming through the docs, it seems to me that:

  • Regular merges might not be compatible with merge queues. I would want to at least prepare an announcement but probably gather a feedback before enabling it.
  • Will probably need GitHub actions modified. 1
  • Since GitHub merge queues require Org owners to set up and configure, I would like to see some plan which checkboxes to check beforehand. 2 And test those on a different repo first.

Footnotes

  1. Last time I tried the GH queues on one of my projects, still in beta, it automatically started merging all PRs even before the CI finished so perhaps I missed that I need to modify the actions. Or maybe it was auto-merges.

  2. Someone mentioned https://github.com/twelvelabs/gh-repo-config. That would be helpful but not sure if this is secure.

@Mic92
Copy link
Member Author

Mic92 commented Jan 8, 2025

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.

@MattSturgeon
Copy link

MattSturgeon commented Jan 8, 2025

Regular merges might not be compatible with merge queues.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests