channable / hoff

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

Ignore builds on other branches #148

Closed rudymatela closed 2 years ago

rudymatela commented 2 years ago

closes: #147

If there are two builds on different branches but with the same hash, GitHub reports these at least twice! (Once per branch.) This makes it so that we only consider build status changes on the actual relevant testing branch. There's a detailed explanation here.

rudymatela commented 2 years ago

@FPtje This is a fix for the multiple-comment issue you and Bert reported yesterday. Can you please review when you have the time?

GitHub's documentation isn't super clear of what's the criteria for including the branch in the branches list: https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#status. Here's what it says:

"An array of branch objects containing the status' SHA. Each branch contains the given SHA, but the SHA may or may not be the head of the branch. The array includes a maximum of 10 branches."

If that's true in the most straightforward interpretation, there is no way to actually identify the "canonical" build for a sha in a branch! In this PR I assume the list only includes the branches for which the build refers to (this would make more sense). In the case of Semaphore, there can be multiple builds for the same commit sha if they are on different branches.

My plan is to see in practice what happens after we deploy this change:

  1. if it only includes the relevant branch, the issue is fixed;
  2. if it includes all branches containing the sha (even those that do not refer to the build, the issue will still be present, but Hoff will continue working "as-is".

In case of "2.", I will have to prepare a second PR where different URLs for the same SHA are simply ignored. This will make Hoff "internally inconsistent" but at least we will not get duplicate comments. Either way, we will get a clarification of what exactly GitHub does because the webhook PR branches will be logged.

FPtje commented 2 years ago

I'm a little short on time. @frank-channable could you review? Thanks!

rudymatela commented 2 years ago

@frank-channable Thanks for the review.

@OpsBotPrime mergeee

OpsBotPrime commented 2 years ago

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

OpsBotPrime commented 2 years ago

Rebased as 88a0ec601b8d1008e08ff3a6ea50815daba67d31, waiting for CI …

OpsBotPrime commented 2 years ago

CI job started.

rudymatela commented 2 years ago

@OpsBotPrime mergeee

@OpsBotPrime You do accept trailing alphabetic characters... Interesting...

OpsBotPrime commented 2 years ago

CI job started.

rudymatela commented 2 years ago

@OpsBotPrime test.

OpsBotPrime commented 2 years ago

That was not a valid command.

rudymatela commented 2 years ago

@OpsBotPrime merge

OpsBotPrime commented 2 years ago

CI job started.

rudymatela commented 2 years ago

CI job started.

This makes sense. The reply to the merge command when it is in progress is the current status: "CI job started". :+1:

rudymatela commented 2 years ago

Message from the future: this merge has been reverted in faf04c96486cb9dc30b6f133f6574ceab3401955.