Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Tracked by #77
rudymatela opened this issue Aug 5, 2022 · 4 comments · Fixed by #154
Closed
Tracked by #77

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

rudymatela opened this issue Aug 5, 2022 · 4 comments · Fixed by #154
Assignees
Labels
bug Something isn't working OKR Objectives and Key Results train Involves Merge Trains

Comments

@rudymatela
Copy link

... and these status shouldn't be reversible.

As noted in #147, GitHub may send multiple webhooks for the same commit hash whenever there are multiple branches pointing to it.

In general this means that when there two builds running in parallel for the same hash, Hoff will merge at the point of the first BuildSuccess on either of the branches. I believe this has always been the case even on early Hoff versions, and is a reasonable course of action.

However, after the changes introduced in #131 (and #141), GitHub is now able to override BuildFailures with BuildStarted if another build is started on a different branch:

  1. Hoff comments: rebased as abc1234. waiting for CI...;
  2. Hoff comments: Build started, URL: ...;
  3. Hoff comments: Build failed, URL: ...;
  4. [so far so good];
  5. Someone creates a new branch with the head pointing to abc1234 and pushes;
  6. Hoff comments: Build started, URL: ..., the build status has been overridden here and shouldn't!

The issue is easy to replicate, but should be uncommon in practice. When Hoff rebases with a merge commit, there shouldn't be problems. When the PR is a single commit, then the issue may arise: if one pushes a branch with a single commit on top of master and is quick enough with the @hoffbot merge comment, then it may appear. It nevertheless should be fixed.

@rudymatela rudymatela added the bug Something isn't working label Aug 5, 2022
@rudymatela rudymatela mentioned this issue Aug 5, 2022
43 tasks
@rudymatela rudymatela self-assigned this Aug 5, 2022
@rudymatela
Copy link
Author

The fix should be fairly straightforward, in handleBuildStatusChanged, we should not be allowed to overwrite BuildFailed or BuildSucceeded with other statuses, only with the same status on a different URL:

hoff/src/Logic.hs

Lines 545 to 558 in faf04c9

handleBuildStatusChanged :: Sha -> BuildStatus -> ProjectState -> Action ProjectState
handleBuildStatusChanged buildSha newStatus = pure . Pr.updatePullRequests setBuildStatus
where
setBuildStatus pr = case Pr.integrationStatus pr of
-- If there is an integration candidate, and its integration sha matches that of the build,
-- then update the build status for that pull request. Otherwise do nothing.
Integrated candidateSha oldStatus | candidateSha == buildSha && newStatus /= oldStatus ->
pr { Pr.integrationStatus = Integrated buildSha newStatus
, Pr.needsFeedback = case newStatus of
BuildStarted _ -> True
BuildFailed _ -> True
_ -> Pr.needsFeedback pr -- unchanged
}
_ -> pr

@rudymatela
Copy link
Author

I am marking this as one of the requisites of #77 as:

  1. the bug was introduced in preparation for it;
  2. it is very important that build statuses are handled correctly in the presence of merge trains.

@rudymatela
Copy link
Author

When solving this issue, one shouldn't forget to start with a failing ❌ automated test that exposes this issue.

@rudymatela
Copy link
Author

When solving this issue, one shouldn't forget to start with a failing x automated test that exposes this issue.

Done! 571901c

@rudymatela rudymatela added OKR Objectives and Key Results train Involves Merge Trains labels Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working OKR Objectives and Key Results train Involves Merge Trains
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant