feat: add option to wait for ci on approval

[Sakib25800]

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

[Kobzol]

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]

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

[Kobzol]

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]

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]

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 :sweat_smile:

[Sakib25800]

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

[Kobzol]

Brainstorming.

High-level behavior:

• @bors r+ where priority was already set, or @bors r+ p=X => immediate approval
• @bors r+ where PR CI is green at the time of the comment => immediate approval
  • We would have to figure out the status of PR CI, using mergeable_state or some other approach (listening for PR CI webhooks, if there is such a thing?).
• @bors r+ where PR CI is not green at the time of the comment => mark as "approval pending". We should record the approved SHA, and the approver, but not yet in the final form. We should also record the approval time (see below).

Implementation:

Store something like this in the DB (conceptually):

enum ApprovalStatus {
   NotApproved,
   Approved {
      sha: String,
      approver: String
   },
   ApprovalPending {
      sha: String,
      approver: String,
      approved_at: Utc
   }
}

• Once PR CI finishes, if the PR is marked as "approval pending", switch state to "approved".
• We should deal with missed webhooks, as PR CI webhooks might be missed. Have a periodic background check that scans PRs with pending approval.
  • If CI is still running for too long, remove the pending approval and post a comment.
  • If CI failed, remove the pending approval and post a comment.
  • If CI is green, confirm the approval (and post a comment?).

Does something like this make sense? I haven't thought of all the possible interactions, as this functionality is quite tricky and I haven't yet examined how can we find out from GH about PR CI in the first place - that would be a good place to start :)

[Sakib25800]

Yes, that makes sense.

I think we can use the status webhook to check if CI has completed.

status.state can tell us if CI passed/failed and sha can be used to query PRs from the database with that head_sha (we would have to add this to DB).

[Kobzol]

Interesting, that sounds very useful indeed. In theory we could also use that for checking the success of try/merge workflows, although there we also want to have more information about what workflows are executed.

[Kobzol]

https://stackoverflow.com/a/72312617/1107768 this post has a very comprehensive discussion of the various solutions for this.