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

Allow commiters to merge any PR after CI is green #95

Open
Atemu opened this issue Jun 6, 2024 · 16 comments
Open

Allow commiters to merge any PR after CI is green #95

Atemu opened this issue Jun 6, 2024 · 16 comments

Comments

@Atemu
Copy link
Member

Atemu commented Jun 6, 2024

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
Copy link
Member

Mic92 commented Jun 7, 2024

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

@JohnRTitor
Copy link

+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
Copy link
Member

felschr commented Jun 24, 2024

A while ago I attempted to implement this functionality in nixpkgs using GitHub actions: NixOS/nixpkgs#261800

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

@Scriptkiddi
Copy link
Collaborator

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
Copy link
Member Author

Atemu commented Jun 26, 2024

Oh, and it works on any PR?

@Mic92
Copy link
Member

Mic92 commented Jun 26, 2024

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

@JohnRTitor
Copy link

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
Copy link
Member Author

Atemu commented Jun 26, 2024

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
Copy link
Member

Mic92 commented Jun 26, 2024

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

@Scriptkiddi
Copy link
Collaborator

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

@JohnRTitor
Copy link

@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. NixOS/nixpkgs#333074 (comment)

@patka-123
Copy link

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

@alyssais
Copy link
Member

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
Copy link
Member

pbsds commented Sep 12, 2024

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
Copy link
Member

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

Some of the common failures happen on Linux too.

@Scriptkiddi
Copy link
Collaborator

@pbsds

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

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

8 participants