tide: changes requested block merge when branch protection requires reviews #537
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
✅ Deploy Preview for k8s-prow ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Prucek The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/XL
|
/cc |
petr-muller
left a comment
petr-muller
left a comment
There was a problem hiding this comment.
Some good news and bad news here ;)
Good news: the requirementDiff test coverage alone is an excellent contribution and I'd like to get it merged no matter what, likely separately from the actual attempted fix (see the bad news).
Bad news: I do not think this actually resolves the underlying problem. The requirementDiff method is used in Tide's status-managing controller (the one that manages the GH status context on the PR commit, including the message shown on the commit) but not in the controller that actually manages mergeability decisions and attempts merge. There are some notes on this in the linked issue.
|
|
||
| if q.ReviewApprovedRequired && pr.ReviewDecision != githubql.PullRequestReviewDecisionApproved { | ||
| // PullRequestReviewDecisionChangesRequested is similar to not approved | ||
| if pr.ReviewDecision == githubql.PullRequestReviewDecisionChangesRequested { |
There was a problem hiding this comment.
Even if this was sufficient to fix the issue (which I'm afraid it isn't :( ), would this always be correct to do? I believe that "prevent PRs from merge if there are changes requested" is actually configurable behavior (part of branch protection), so depending on what the repository has configured (and I think also depending on who authored the "request changes" review - maybe that part is propagated to the pr.ReviewDecision here though) such review may or may not block merging, but here we'd treat it as if such branch protection was always enabled.
There was a problem hiding this comment.
right, understood. So now, when I get that info from branch protection and update the isAllowedToMerge that should work?
There was a problem hiding this comment.
Yes, but it's complicated under that "get that info from branch protection" sentence. "Will this PR be mergeable per all GitHub-side configuration" is a complicated problem, because branch protection is now actually two kinds of GH-side concepts (rulesets -- new thing and protected branches -- old thing). And both are not trivial to intepret. Getting info from branch protection would mean you'd need to interpret them, basically reverse engineering GH functionality, in Tide which I believe is not sustainable. It may not be that hard to do for specific merge-blocking setting (like "changes requested" here) but the whole thing would become a whack-a-mole where various GH-side settings prevent PRs from merging, and Tide would need to learn all of them them.
I believe the suitable medium-term approach is to improve how this whole class of failure cases is surfaced. One of the main problems today is that when you hit any case where Tide thinks something is mergeable but GH prohibits that (basically whenever you hit this place), it is not surfaced to the user in any way. The user will see something like a green dot saying In merge pool and Tide will be logging these "user errors" in its log.
If we could somehow improve this behavior in general (no matter what the actual reason GH has for disallowing the merge), it would benefit this whole class of failure cases, and we would have more space to experiment with how to make Tide play better with these GH concepts.
Obtaining GH-side information for each merge attempt has its flaws, too; it is API-intensive (although it can be cached, like you are doing in the current revision) and imagine doing these checks for every cause that can prevent the merge.
One additional thought I have is whether GH surfaces the result of its mergeability checking machine somethow through APIs available to Tide. The GitHub UI itself can disable (grey-out) the merge button if the PR does not meet the mergeability criteria - this means GH is able to expose the data at least to its UI. Now the question is whether this is also exposed through the public APIs.
There was a problem hiding this comment.
Some breadcrumbs on:
whether GH surfaces the result of its mergeability checking machine somethow through APIs available to Tide
- There is something called
mergeable_statein the v3 PR API; we do not seem to use that at all (we usemergeablewhich AFAIK surfaces the git-level mergeability). It is not documented in https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28&versionId=free-pro-team%40latest&category=pulls (at least I have not ofund anything relevant but it is listed in the example API responses) - v4 docs have something called
MergeStateStatuswhich looks worth looking into: https://docs.github.com/en/graphql/reference/enums#mergestatestatus
Sources: I interrogated Claude Code which then lead me to the follwoing links:
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
Prucek
changed the title
|
Conversation from #537 (comment) aside, I am not opposed to trying to improve it for this specific case with the approach you are trying, and I will review the code as it is now. What kind of testing have you done on this? Have you tried reproducing the failure case from the linked issue and verifying the modified Tide really improves the behavior as expected? |
3 participants
Fixes #269
When a PR has "changes requested", Tide was attempting to merge it even though GitHub blocks it when branch protection requires reviews.
Changes:
isAllowedToMerge()to block merges only when branch protection requires reviews and the PR has "changes requested"requirementDiff()to respect branch protection settings for status messagesbranchRequiresReviews()helper with caching to minimize API callsResult: Tide no longer attempts to merge PRs with "changes requested" when branch protection would block the merge.
🤖 Assisted by Claude.