channable / hoff

A gatekeeper for your commits
Apache License 2.0
41 stars 3 forks source link

(Re-)infer candidate from state #141

Closed rudymatela closed 2 years ago

rudymatela commented 2 years ago

Fixes: #131

This reapplies the changes of #131 starting from a version where tests assure that Hoff is not stuck in an infinite comment loop. The summary of #131 still holds:

The changes in this PR do not change Hoff behaviour, the high level functionality should be the same.

Instead of maintaining a global candidate, we infer it from the state always. This is in preparation for merge trains (#77), where we will have multiple builds running in parallel and thus multiple candidates.

  • The ProjectState datatype is now simpler and easier to maintain (before we could risk it being inconsistent);
  • Some code paths are now simpler, as we don't have to do the work maintaining the global candidate;
  • There is a new explicit integration status Promoted to signify that the PR branch is now part of master, before this was signified by being Integrated ... BuildSucceeded but not being the global candidate.

A linear search on the number of open PRs is imposed. I believe this is not a problem because:

  • the number of PRs should be at most a few dozen, examples:
    • there are only 5 open PRs on channable/hoff currently;
    • a dozen open PRs seems to be a reasonable average (:wink:);
    • a busy project (:wink:) should have between 100 and 200 open PRs in which a linear search is still reasonable;
    • ... and we could still have way more than that without suffering significant performance impact (1000 or 10000 open PRs).
  • we already perform linear search for other operations, the candidate was just a special case in where we don't. If we were to actually get rid of linear search altogether, we perhaps should have a more generic solution that covers all cases. (If linear search were to be a problem, we would already be suffering from it.)
  • with #77, we are bound to perform a linear search on the candidate PRs anyway.

TODO

rudymatela commented 2 years ago

@alex-mckenna Since I am sort of restoring my previous changes now, I took the traversePullRequests function and your implementation of handleBuildStatusChanged from your (now-orphan) commit. This won't be merged soon, as I still need to fix the bug I caused.

rudymatela commented 2 years ago

@fatho You have previously reviewed #131, which was the PR that had the bug and was reverted. This PR re-adds the same functionality, so part of the code is the same as in the previous PR. I think it makes sense for you to review again in this case.

The end result is quite different in some parts, and the code of this new version is IMHO more elegant than the one of #131.

rudymatela commented 2 years ago

@fatho Thanks for the review. I will merge this into master now and tag it so that I can have this ready for deploy at an appropriate time this week.

@OpsBotPrime merge

OpsBotPrime commented 2 years ago

Pull request approved for merge by @rudymatela, rebasing now.

OpsBotPrime commented 2 years ago

Rebased as 1bd9c99f4f1a0fb08ee00546b4c5958d850857a0, waiting for CI …