ci: use merge commit for apidiff to avoid false positives #9622
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes false positive breaking change detection in the apidiff workflow by using merge commits instead of head commits for comparison. The issue occurs when PRs are created from outdated base branches, causing newly added features in main to appear as "removed" in the PR.
Key changes:
- Changed checkout reference from
head.shato merge commit (refs/pull/{number}/merge) - Added merge conflict detection to prevent frozen merge commit issues
- Updated comments to explain the new approach and rationale
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
.github/workflows/apidiff.yaml
Outdated
| - name: Check for merge conflicts | ||
| if: ${{ github.event.pull_request.mergeable != true }} | ||
| run: | | ||
| echo "::error::This PR has merge conflicts. Please resolve conflicts before running apidiff." | ||
| exit 1 |
There was a problem hiding this comment.
hm... It looks like we need to update this.
Take a look this PR:
DmitriyLewen#191
There was a problem hiding this comment.
I tested this change in trivy-test, but I added this validation at the end and probably forgot to retest it.
aquasecurity#52
I'll recheck it. Thanks.
There was a problem hiding this comment.
If I understand correctly, the value of github.event.pull_request.mergeable is determined at the time the trigger runs (for example, when the PR is created) and isn’t updated afterward.
To get the current state, we can call the GH API.
Updated in 8067ef5
Test PRs:
There was a problem hiding this comment.
Isn't it possible that the conflict calculation hasn’t finished yet, causing it to return null? In other words, do we need to wait in a while loop until it returns a non-null value? I’m not sure about mergeable_state, but isn’t there a similar risk?
The value of the mergeable attribute can be true, false, or null. If the value is null, then GitHub has started a background job to compute the mergeability. After giving the job time to complete, resubmit the request. When the job finishes, you will see a non-null value for the mergeable attribute in the response. If mergeable is true, then merge_commit_sha will be the SHA of the test merge commit.
https://docs.github.com/en/rest/pulls/pulls?utm_source=chatgpt.com&apiVersion=2022-11-28
There was a problem hiding this comment.
Yeah, I was looking into this as well and had the same concern.
My thought was that when using the pull_request trigger, if there’s a conflict, the pipelines won’t even start.
Although this might not be the case for pull_request_target (where the pipelines still run), I believe conflicts are checked first, before the pipelines start.
So we should still get the correct mergeable_state.
As far as I understand, when a PR is created or updated, a background process starts to update the mergeable value.
By the time the pipelines start, this value should already be calculated.
However, when using github.event.pull_request.mergeable, the value is fixed (null) and doesn’t get updated afterward.
But if you’d like, we can add a loop with a delay to wait until the value is no longer null.
There was a problem hiding this comment.
If you're sure that conflicts are checked first, I'm okay.
There was a problem hiding this comment.
If you're sure that conflicts are checked first, I'm okay.
Unfortunately, I couldn’t find any confirmation of this for pull_request_target.
I haven't had any problems with the current approach.
I think we can always add a verification loop if we run into any issues.
There was a problem hiding this comment.
I checked again.
pull_request_target and mergeability are processed asynchronously.
So for large PRs, it’s possible that we start apidiff too early.
I added 5 retries with a 2-second delay - fc6559b
Could you take a look when you have time?
Co-authored-by: Teppei Fukuda <knqyf263@gmail.com>
|
LGTM |
.github/workflows/apidiff.yaml
Outdated
| # frozen at an old state and apidiff cannot run correctly. | ||
| - name: Check for merge conflicts | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} |
There was a problem hiding this comment.
While github.token should be equivalent to secrets.GITHUB_TOKEN, is there any reason to use github.token here? I was curious because GITHUB_TOKEN is used elsewhere in Trivy and also generally used in the documentation.
https://docs.github.com/en/actions/tutorials/authenticate-with-github_token
There was a problem hiding this comment.
I didn't notice the AI changed that...
Thanks for noticing.
Updated in b387659
…ty#9622) Co-authored-by: DmitriyLewen <dmitriy.lewen@smartforce.io> Co-authored-by: DmitriyLewen <91113035+DmitriyLewen@users.noreply.github.com>
2 participants
Description
This PR fixes false positive breaking change detection in the apidiff workflow when PRs are created from an old base branch.
Problem
When a PR is created from an old base and the main branch has new commits with API additions, the current workflow compares:
base.sha(latest main with new features)head.sha(PR branch without those features)This causes features added to main after PR creation to appear as "removed" in the PR, resulting in false breaking change detection.
Solution
head.shatorefs/pull/{number}/mergeto compare the actual post-merge stateReference
Checklist