(Re-)infer candidate from state #141

rudymatela · 2022-07-22T11:55:04Z

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

TODO

Test comments looping back from GitHub #139
- to detect the infinite comment loop
rebase revert of revert of Merge train: infer candidate from state #131 on top of the new master
fix the issue
- PRs with BuildFailed are not candidates
- Use needsFeedback on handleBuildStatusChanged
test that failing PRs do not "clog" the queue
- using GitHub
- in the actual test files

rudymatela · 2022-07-22T14:04:49Z

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

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

... in order to prepare for a future change.

This eliminates the need for the getIntegrationCandidate function and simplifies test fixtures.

from the 'Project' module in favour of just using 'integratedPullRequests'. The getIntegrationCandidates function was added to tests/Spec as it has nice uses there.

... in order to detect that PRs are merged in the order of merge comments and not by PR id.

rudymatela · 2022-07-26T11:11:39Z

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

fatho

LGTM, thanks for adding the tests!

fatho · 2022-07-27T09:58:27Z

src/Logic.hs

-      BuildPending _ -> pure state
-      BuildSucceeded -> pushCandidate (pullRequestId, pullRequest) sha state
-                        -- If the build failed, give feedback on the PR
-      BuildFailed _  -> pure $ Pr.setNeedsFeedback pullRequestId True state


Ah, so instead of always setting this to true when there's activity on a failed PR, we now only do this when the state initially changes?

Yes. Before, for a BuildFailed candidate:

set PR build to failed when event is received

go through the event loop

in proceedCandidate, "discover" and set that the PR needs feedback

go through the event loop again

post comment (for the PR that needs feedback)

Now:

set PR build to failed and that it needs feedback

go through the event loop

post comment (for the PR that needs feedback)

fatho · 2022-07-27T09:58:36Z

src/Logic.hs

+      pr { Pr.integrationStatus = Integrated buildSha newStatus
+         , Pr.needsFeedback = case newStatus of
+                              BuildPending (Just _) -> True
+                              BuildFailed _         -> True


... (cont'd from previous comment), which would be here?

Yes, exactly. And we only need to post comments when we get the BuildPending status with a URL and when the build fails.

rudymatela · 2022-07-27T10:56:24Z

@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 · 2022-07-27T10:56:25Z

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

Approved-by: rudymatela Auto-deploy: false

OpsBotPrime · 2022-07-27T10:56:30Z

Rebased as 1bd9c99, waiting for CI …

rudymatela self-assigned this Jul 22, 2022

rudymatela added bug Something isn't working enhancement New feature or request labels Jul 22, 2022

Base automatically changed from fix/comment-loop to master July 22, 2022 13:27

rudymatela force-pushed the merge-train/re-infer-candidate-from-state branch from 6d97c67 to a21b16e Compare July 22, 2022 14:01

rudymatela mentioned this pull request Jul 22, 2022

Test the build status comment. #143

Merged

rudymatela added 6 commits July 22, 2022 17:04

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.

Reindent and golf handleBuildStatusChanged

c251948

... in order to prepare for a future change.

Fix infinite comment loop.

254e8c3

simplify handleBuildStatusChanged

7f7c24b

further simplify handleBuildStatusChanged

20a5ddd

refactor handleBuildStatusChanged

023df3b

rudymatela force-pushed the merge-train/re-infer-candidate-from-state branch from 66bf88c to 023df3b Compare July 22, 2022 15:04

Make describeStatus consistent

dcc9b2b

rudymatela changed the base branch from master to test/pending-status-comment July 22, 2022 15:15

Base automatically changed from test/pending-status-comment to master July 26, 2022 06:57

rudymatela added 9 commits July 26, 2022 09:35

Simplify proceedCandidate

aa85b2d

further simplify proceedCandidate

e6731d0

eventLoopSpec: add and use integrationShas

a8b4974

This eliminates the need for the getIntegrationCandidate function and simplifies test fixtures.

Project: remove 'getIntegrationCandidates'

f67b7a2

from the 'Project' module in favour of just using 'integratedPullRequests'. The getIntegrationCandidates function was added to tests/Spec as it has nice uses there.

tests/Spec: add a big "full" test involving 3 PRs

e032d3a

tests/Spec: add success, failure, success test

8c0bfd9

tests/Spec, sequence: reverse order

ef47930

... in order to detect that PRs are merged in the order of merge comments and not by PR id.

tests/Spec: rename shas (more distinctive test)

7c0756a

tests/Spec: review comments of full tests

544edf3

rudymatela requested a review from fatho July 26, 2022 11:09

rudymatela marked this pull request as ready for review July 26, 2022 11:12

fatho approved these changes Jul 27, 2022

View reviewed changes

Merge #141: (Re-)infer candidate from state

1bd9c99

Approved-by: rudymatela Auto-deploy: false

OpsBotPrime merged commit 1bd9c99 into master Jul 27, 2022

OpsBotPrime deleted the merge-train/re-infer-candidate-from-state branch July 27, 2022 11:15

rudymatela mentioned this pull request Jul 28, 2022

Merge train #77

Closed

43 tasks

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

rudymatela mentioned this pull request Aug 25, 2022

Fix web display of merge trains #178

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

(Re-)infer candidate from state #141

(Re-)infer candidate from state #141

Uh oh!

rudymatela commented Jul 22, 2022 •

edited

Loading

Uh oh!

rudymatela commented Jul 22, 2022

Uh oh!

rudymatela commented Jul 26, 2022

Uh oh!

fatho left a comment

Uh oh!

fatho Jul 27, 2022

Uh oh!

rudymatela Jul 27, 2022

Uh oh!

fatho Jul 27, 2022

Uh oh!

rudymatela Jul 27, 2022

Uh oh!

rudymatela commented Jul 27, 2022

Uh oh!

OpsBotPrime commented Jul 27, 2022

Uh oh!

OpsBotPrime commented Jul 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

(Re-)infer candidate from state #141

(Re-)infer candidate from state #141

Uh oh!

Conversation

rudymatela commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

rudymatela commented Jul 22, 2022

Uh oh!

rudymatela commented Jul 26, 2022

Uh oh!

fatho left a comment

Choose a reason for hiding this comment

Uh oh!

fatho Jul 27, 2022

Choose a reason for hiding this comment

Uh oh!

rudymatela Jul 27, 2022

Choose a reason for hiding this comment

Uh oh!

fatho Jul 27, 2022

Choose a reason for hiding this comment

Uh oh!

rudymatela Jul 27, 2022

Choose a reason for hiding this comment

Uh oh!

rudymatela commented Jul 27, 2022

Uh oh!

OpsBotPrime commented Jul 27, 2022

Uh oh!

OpsBotPrime commented Jul 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rudymatela commented Jul 22, 2022 •

edited

Loading