Allow commiters to merge any PR after CI is green

[Atemu]

I sometimes have the case where I approve a PR but would like to wait for CI checks to be green before actually merging. What happens then is that I leave the tab open and return to it some time later but that can sometimes be weeks or even months when it should have been merged after an hour or so.

Would this be in scope for the merge bot?

[Mic92]

Would be a nice feature indeed. This or merge queues.

[JohnRTitor]

+1 for the merge queue, I believe we have a similar thing in Nixos-hardware already.

I try to "approve PRs" before merging, so I could get back after CI passes green. But sometimes PRs fall through the cracks despite being approved by a commiter.

[felschr]

A while ago I attempted to implement this functionality in nixpkgs using GitHub actions: https://github.com/NixOS/nixpkgs/pull/261800

Unfortunately I got stuck, but maybe this can give some insights for a new implementation in nixpkgs-merge-bot.

[Scriptkiddi]

So apparently our docs need to be better. Currently if you use the merge bot it waits for all checks to pass and then merges the pr.

It only skips Darwin tests because the ci for that is sometimes broken

[Atemu]

Oh, and it works on any PR?

[Mic92]

I think, it does by-name checks. But we can probably remove this restrictions for committers.

[JohnRTitor]

It does by-name checks and checks whether the invoking person is listed as the maintainer of the package. But currently if CI is not green, it errors out, providing no merge queue functionality.

[Atemu]

It does by-name checks and checks whether the invoking person is listed as the maintainer of the package.

Disabling those two checks if the invoker is a committer is basically the core of this feature request.

But currently if CI is not green, it errors out, providing no merge queue functionality.

That would/should be expected behaviour.

[Mic92]

It probably needs a proper database at this point. On the other side, can we not just add mergify to nixpkgs` as well?

[Scriptkiddi]

@JohnRTitor It provides an error message, that it will try again later. Or is this not working for some reason?

[JohnRTitor]

@JohnRTitor It provides an error message, that it will try again later. Or is this not working for some reason?

It does not suppress by name checks for commiters I am afraid. https://github.com/NixOS/nixpkgs/pull/333074#issuecomment-2284277610

[patka-123]

Perhaps I'm not understanding something, but this seems like an existing github feature that can be enabled.

[alyssais]

The existing GitHub feature would not work for us, because it only looks at required checks, and most (all?) CI checks in Nixpkgs are not required. It would be non-trivial to make them required, because sometimes there's a legitimate need for them to be bypassed — we'd have to see if we could make permissions more widely available to be able to do that than they currently are. (I think it's currently restricted to org owners, which would absolutely not be enough since there's not a lot of overlap between org owners and the most active Nixpkgs committers.)

[pbsds]

Several issues tied to ofborg CI should also be addressed for this auto-merge feature to be a QoL boost:

• Don't merge if ofborg eval has not been run. This happens from time to time, or is at least slow to trigger, especially on automatic backport PRs
• If the darwin builders don't return within a week we can assume it may have dropped the queue.
• Don't wait around for queued darwin builds that are not meta.available
• Common darwin ofborg failures like out-of-disk, socket read/write error, or inability to build the deps of passthru'd nixosTests should also be detected and ignored.

[alyssais]

Don't wait around for queued darwin builds that are not meta.available

Some of the common failures happen on Linux too.

[Scriptkiddi]

@pbsds

• We wait for ofboarg eval and checks. We do not wait on anything darwin related as those fail to often.

[Scriptkiddi]

So we got a little bit closer to the goal with the Committer PR Merge Strategy. It allows to merge every pull request from a committer by the merge bot, from a maintainer of the package. To full support this feature set here we need a Committer merge strategy that only validates if a person is a committer, than this issue can be closed

[wolfgangwalther]

Since nixpkgs-merge-bot currently doesn't check ofborg results, this feature is the same as the already enabled "auto-merge" in GitHub directly.

Imho, it's questionable whether the merge bot should support this - or whether we should try to create better required status checks in Nixpkgs directly. For example, we could create a second category of "required status checks" that is triggered by OfBorg results.

[Scriptkiddi]

the merge bot checks ofborg results

[wolfgangwalther]

the merge bot checks ofborg results

Not right now:

https://github.com/NixOS/nixpkgs-merge-bot/blob/fd38895757a0eff0a66efdeb363be3f9f8f41753/nixpkgs_merge_bot/commands/merge.py#L42-L48

These were disabled in #142, which.. you approved and merged ;)

[wolfgangwalther]

We now have a merge queue in Nixpkgs for the master branch and support auto-merge on the others. I don't think we should spent time and effort on this for merge-bot, instead, if we want to include ofborg in these things, we should work on making (parts of) ofborg a required status check in Nixpkgs.