Merge PRs in approval order

[duijf]

Moving a subdiscussion from #19

Currently, Hoff does not remember the order in which pull requests were approved. It instead chooses merge candidates based on PR id (lowest go first).

To guarantee fairness and preserve potential user intent (when there dependent PRs) we can consider Hoff maintaining a queue in which merge commands were received. It should rebase + check + merge approved PRs in that order.

CC @wesleybowman

[duijf]

preserve potential user intent

One argument against this is: Hoff currently doesn't have a notion of PRs which depend on one another. Approval order does not imply a specific merge order, as the following can happen even if Hoff uses approval order to decide what to integrate when:

• User approves PR 1
• User approves PR 2 (thinking it will be merged after PR 1)
• Hoff rebases and builds PR 1.
• The rebased build for PR 1 fails. Hoff does not merge PR 1 to master.
• Hoff rebases and builds PR 2.
• The rebased build for PR 2 succeeds.
• Hoff merges PR 2.

If we want to avoid that behavior we should have something like @bot merge after #X

[wesleybowman]

(also want to mention I don't have a strong preference here, I had just thought of a use case which I thought might happen fairly often. However, it could still be up to the user to wait on the appropriate PRs to be merged before requesting another to be merged)

[ruuda]

There are arguments to be made both for ordering by PR number or by approval time. There was a lot of discussion about it for the Rust merge bot. (some, links, I thought there was a more recent one too but I can’t find it.) But in any case, this matters when the queue is long and builds take a long time; I don’t think it matters for us at this point.

when there dependent PRs

If you create the dependency first, then it will have a lower PR number, and things will work out.

[ruuda]

Ah so @wesleybowman said

Wouldn't this be better in general anyway? What if I open a PR, and then a reviewer is like, "Please split out the typing from this PR in a separate PR..." or something similar, and then the new PR is approved as well as the old. It seems like you should be able to request a merge in the order you want, so I could go to the new PR and request a merge, and then go to the older PR and request immediately as well, without having to wait on the newer one to be merged first.

I think having to watch out with the order of your approval comments is not too bad in this case. Properly supporting dependencies would be nice, but not easy. #6 is also related to this.