Merge train: infer candidate from state #131

rudymatela · 2022-07-14T13:42:51Z

... in preparation for #77 (partly closes: #77)

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 Merge train #77, we are bound to perform a linear search on the candidate PRs anyway.

Notes

This is an alternative to PR #130. Here, instead of changing the datatype of integrationCandidate, we simply remove it and try to infer from the rest of the state every time.

rudymatela · 2022-07-19T09:04:24Z

c'mon GitHub, I thought you could handle the renaming of branches... apparently not.

rudymatela · 2022-07-19T09:16:54Z

c'mon GitHub, I thought you could handle the renaming of branches... apparently not.

So I tried renaming the branch from junk/merge-train/infer-integration-candidate by removing the junk/ prefix, signifying that this was no longer an experiment. GitHub does not let me do it without opening a new PR. So I'll leave the junk/ prefix then even though it should be not there in spirit.

Infer the `integrationCandidate` instead of storing it explicitly on the project state. This may make it easier to implement the merge train as we will need to handle multiple integration candidates. This specific change tries to maintain backward compatibility where possible, this backwards compatibility will gradually be cleaned up in further commits. The code typechecks but the tests fail for now.

where applicable to replace candidatePullRequests. In the code "candidate" can mean two different (but related things): * a candidate for integration, as returned by the function candidatePullRequests; * something that has been integrated and is waiting for build results to become master integrationCandidate field of ProjectState; If I understand correctly, the latter is perhaps better described as a PR that is "integrated-but-waiting-for-build-results" or an integratedCandidate. The names have not yet been updated to reflect this. cf: #77 (comment) This fixes some failing tests, but there are still quite a few to go...

... this makes the tests pass again.

... as it was now a no-op.

... and simplify one of the unreachable cases.

... in actual code. It is now only used in tests.

in favour of getIntegrationCandidate

fatho

Basically LGTM, but some questions:

fatho · 2022-07-20T07:51:37Z

src/Project.hs

@@ -287,6 +295,7 @@ classifyPullRequest pr = case approval pr of
      BuildPending    -> PrStatusBuildPending
      BuildSucceeded  -> PrStatusIntegrated
      BuildFailed url -> PrStatusFailedBuild url
+    Promoted -> PrStatusIntegrated -- TODO: state-of-its-own?


Is there still something you want to do about this TODO?

Not really, I removed it from the code now: e6c9da1

fatho · 2022-07-20T08:07:39Z

tests/Spec.hs

      cId     `shouldBe` PullRequestId 2
      actions `shouldBe`
-        [ ATryIntegrate "Merge #2: Add my test results\n\nApproved-by: deckard\nAuto-deploy: false\n" (Branch "refs/pull/2/head", Sha "f37") False
+        [ ATryPromote (Branch "results/leon") (Sha "38d")


Where does this extra action come from?

In this test suite, there are two open PRs:

the first which has been rebased and the build has (just) passed;

the second which was not rebased yet;

So the actions should be to promote the first to be the new master, and try to rebase (integrate) the second.

If we instead set the first PR to be Promoted, the TryPromote should disappear.

This extra action just came to how the state is represented now: before a Promoted pr was one that was Integrated BuildSuceeded while the candidate was Nothing.

I decided to change the test to include the promotion.

I could update the test name, but I think it still applies 😁:

"picks a new candidate from the queue after a successful push"

rudymatela · 2022-07-20T12:48:00Z

@fatho Thanks for the review.

Since I think I have addressed your comments, I am going to merge and tag now.

I will leave the deploy for later today at around 19:00 (if there are no items in the build queue).

@OpsBotPrime merge and tag

OpsBotPrime · 2022-07-20T12:48:01Z

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

Approved-by: rudymatela Auto-deploy: false

OpsBotPrime · 2022-07-20T12:48:07Z

Rebased as c3d7e54, waiting for CI …

OpsBotPrime · 2022-07-20T12:51:52Z

@rudymatela Sorry, I could not tag your PR. The previous tag v0.25.6 seems invalid

rudymatela · 2022-07-20T12:52:56Z

@rudymatela Sorry, I could not tag your PR. The previous tag v0.25.6 seems invalid

Oohh yes, of course. #117 will make you support it.

This reverts commit c3d7e54, reversing changes made to 37bc6c4.

... now the tests should catch the bug where Hoff is stuck in an infinite loop. Revert "Revert "Merge #131: Merge train: infer candidate from state"" This reverts commit 0ef0478.

rudymatela self-assigned this Jul 14, 2022

rudymatela added the enhancement New feature or request label Jul 14, 2022

rudymatela changed the title ~~Merge train~~ Merge train: infer candidate from state Jul 14, 2022

This was referenced Jul 14, 2022

Merge train #77

Closed

Merge train: qualify the test branch by PR id #134

Merged

Merge train: candidates with parents #130

Closed

rudymatela force-pushed the junk/merge-train/infer-integration-candidate branch from 8ad5644 to 4910759 Compare July 19, 2022 08:38

rudymatela requested a review from fatho July 19, 2022 09:01

rudymatela closed this Jul 19, 2022

rudymatela deleted the junk/merge-train/infer-integration-candidate branch July 19, 2022 09:03

rudymatela restored the junk/merge-train/infer-integration-candidate branch July 19, 2022 09:03

rudymatela reopened this Jul 19, 2022

rudymatela marked this pull request as ready for review July 19, 2022 09:06

rudymatela added 11 commits July 19, 2022 13:31

Add and use the Promoted integration status

17ed032

... this makes the tests pass again.

remove the setIntegrationCandidate function

856930f

... as it was now a no-op.

Add and use updatePullRequests

bdb17bc

fix documentation of getIntegrationCandidates

f06d965

remove deprecated integrationCandidate function

9086652

mark unreachable / dead code clearly

1251a84

... and simplify one of the unreachable cases.

remove use of getIntegrationCandidate

2e1d58f

... in actual code. It is now only used in tests.

refactor: use getIntegrationCandidates

28e2640

in favour of getIntegrationCandidate

tests/Spec: reindent candidateState

9188ef9

rudymatela force-pushed the junk/merge-train/infer-integration-candidate branch from 4910759 to 9188ef9 Compare July 19, 2022 11:31

rudymatela added refactor Refactor and removed enhancement New feature or request labels Jul 19, 2022

rudymatela removed the request for review from fatho July 19, 2022 12:07

rudymatela requested a review from fatho July 19, 2022 12:07

fatho reviewed Jul 20, 2022

View reviewed changes

Commit to Promoted -> PrStatusIntegrated (rm TODO)

e6c9da1

Merge #131: Merge train: infer candidate from state

c3d7e54

Approved-by: rudymatela Auto-deploy: false

OpsBotPrime merged commit c3d7e54 into master Jul 20, 2022

OpsBotPrime deleted the junk/merge-train/infer-integration-candidate branch July 20, 2022 12:51

rudymatela added a commit that referenced this pull request Jul 21, 2022

Revert "Merge #131: Merge train: infer candidate from state"

0ef0478

This reverts commit c3d7e54, reversing changes made to 37bc6c4.

rudymatela added a commit that referenced this pull request Jul 22, 2022

Revert the revert of Merge #131

6d97c67

... now the tests should catch the bug where Hoff is stuck in an infinite loop. Revert "Revert "Merge #131: Merge train: infer candidate from state"" This reverts commit 0ef0478.

rudymatela mentioned this pull request Jul 22, 2022

(Re-)infer candidate from state #141

Merged

9 tasks

rudymatela added a commit that referenced this pull request Jul 22, 2022

Revert the revert of Merge #131

a21b16e

... now the tests should catch the bug where Hoff is stuck in an infinite loop. Revert "Revert "Merge #131: Merge train: infer candidate from state"" This reverts commit 0ef0478.

rudymatela added a commit that referenced this pull request Jul 22, 2022

Revert the revert of Merge #131

92512f0

... now the tests should catch the bug where Hoff is stuck in an infinite loop. Revert "Revert "Merge #131: Merge train: infer candidate from state"" This reverts commit 0ef0478.

rudymatela mentioned this pull request Aug 5, 2022

Build failures can be reverted if GitHub sends multiple webhooks #150

Closed

rudymatela added the train Involves Merge Trains label Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge train: infer candidate from state #131

Merge train: infer candidate from state #131

rudymatela commented Jul 14, 2022 •

edited

Loading

rudymatela commented Jul 19, 2022

rudymatela commented Jul 19, 2022

fatho left a comment

fatho Jul 20, 2022

rudymatela Jul 20, 2022 •

edited

Loading

fatho Jul 20, 2022

rudymatela Jul 20, 2022

rudymatela Jul 20, 2022

rudymatela commented Jul 20, 2022

OpsBotPrime commented Jul 20, 2022

OpsBotPrime commented Jul 20, 2022

OpsBotPrime commented Jul 20, 2022

rudymatela commented Jul 20, 2022

Merge train: infer candidate from state #131

Merge train: infer candidate from state #131

Conversation

rudymatela commented Jul 14, 2022 • edited Loading

Notes

rudymatela commented Jul 19, 2022

rudymatela commented Jul 19, 2022

fatho left a comment

Choose a reason for hiding this comment

fatho Jul 20, 2022

Choose a reason for hiding this comment

rudymatela Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

fatho Jul 20, 2022

Choose a reason for hiding this comment

rudymatela Jul 20, 2022

Choose a reason for hiding this comment

rudymatela Jul 20, 2022

Choose a reason for hiding this comment

rudymatela commented Jul 20, 2022

OpsBotPrime commented Jul 20, 2022

OpsBotPrime commented Jul 20, 2022

OpsBotPrime commented Jul 20, 2022

rudymatela commented Jul 20, 2022

rudymatela commented Jul 14, 2022 •

edited

Loading

rudymatela Jul 20, 2022 •

edited

Loading