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

feat: add option to wait for ci on approval #202

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Sakib25800
Copy link
Contributor

@Sakib25800 Sakib25800 commented Feb 24, 2025

Fixes #60

This PR adds a new opt-out feature to only allow for approvals if the check suite passes.

On @bors r+ command received:

  • If CI is currently running:

    • Wait
    • On success: approve PR
    • On failure: unapprove PR
  • If CI already passed:

    • Approve immediately
  • If CI already failed:

    • Approval rejected with notification about failing tests

Opt-out mechanism:

  • If p ≥ 1

@Sakib25800 Sakib25800 force-pushed the feat/approve-pr-on-ci-pass branch from 6387957 to c1252d5 Compare February 24, 2025 18:44
@Sakib25800 Sakib25800 marked this pull request as ready for review February 24, 2025 18:50
@Kobzol
Copy link
Contributor

Kobzol commented Feb 25, 2025

Hi, thanks! This is a good start, implementation wise, although there are some edge cases that will need to be resolved, I'll comment on these once the question below is resolved.

Regarding the configuration, I have imagined this to work a bit differently, where the user would have to opt into this behavior using some special command, e.g. @bors r+ wait-for-pr-ci. I asked in https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/bors.20r.2B.20after.20pr.20ci.20passes to see what other people think.

@Sakib25800
Copy link
Contributor Author

Sakib25800 commented Feb 25, 2025

Ahh, I misunderstood - a command would definitely make more sense.

When the question is answered, I'll fix the issue and work on resolving the edge cases too.

I think @bors r+ await would be nice syntax (taken from rust-lang/homu).

@Sakib25800 Sakib25800 marked this pull request as draft February 27, 2025 09:13
@Sakib25800 Sakib25800 force-pushed the feat/approve-pr-on-ci-pass branch 2 times, most recently from 28d5be8 to 6ea65cd Compare February 27, 2025 13:34
@Sakib25800 Sakib25800 force-pushed the feat/approve-pr-on-ci-pass branch from 6ea65cd to 3c988f9 Compare February 27, 2025 13:35
@Kobzol
Copy link
Contributor

Kobzol commented Feb 27, 2025

Setting PR priority is a prerequisite for this, and it's relatively self-contained, so it would be nice to implement that in a separate PR :)

@Sakib25800
Copy link
Contributor Author

Thanks for the review! Was meaning to ask you that and wanted to clarify:

  1. I'm assuming we need command stacking, which is not yet supported? e.g. being able to do something like @bors p=1 and @bors r+ p=1 (with this being two separate commands). Should I make a PR for this?

  2. I'll make a separate PR for priority, which I'm assuming for now won't do anything meaningful just yet.

In terms of implementing this PR, I'm planning on adding a new column approval_state (none / pending / approved) in PullRequestModel? Unless you have a different suggestion.

@Kobzol
Copy link
Contributor

Kobzol commented Feb 27, 2025

I'm not sure if we necessarily need stacking, p=x or priority=x could just be seen as an argument for r or r+. We already have command arguments. I would like to use the parameter vs generating multiple commands from @bors r+ p=x, so that the r+ command knows about the priority, and can use it e.g. to decide whether to apply the "wait-for-PR-CI" mode or not (otherwise it would introduce unnecessary race conditions, or we would need to generate some canonical ordering for the commands).

Yeah, a separate PR for priority would be nice. I think that storing an integer in the PR with the priority is enough, should be simple.

I'll have to think a bit more about this PR and the DB representation, let's deal with that after priority 😅

@Sakib25800
Copy link
Contributor Author

Side quest finished, should be able to work on this now :)

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

Successfully merging this pull request may close these issues.

Add the option to approve a PR once PR CI passes
2 participants