ci: Check if any changes made ahead of PR affect a release to support merge queue #7114
Conversation
| needs: check-workflows | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Check if the commit or pull request is a release |
There was a problem hiding this comment.
I considered reusing the is-release job below, but it adds a little bit of complexity around the conditionals, and we don't need the extra options below for merged commits.
There was a problem hiding this comment.
Nit: We could maybe skip this for push events? Just because it wouldn't be relevant anymore for push, and this might make it a bit more clear why we have to similarly-named jobs
There was a problem hiding this comment.
We could skip the individual steps on push, but not the job itself, since that would cause all-jobs-complete to be skipped.
| uses: MetaMask/action-is-release@d063725cd15ee145d7e795a2e77db31a3e3f2c92 | ||
| with: | ||
| commit-starts-with: 'Release [version],Release v[version],Release/[version],Release/v[version],Release `[version]`' | ||
| commit-message: ${{ github.event.pull_request.title }} |
There was a problem hiding this comment.
This is used so we can determine if an unmerged pull request is a release pull request. github.event.pull_request.title is not defined for other events (push and merge_group), so doesn't affect those.
.github/workflows/main.yml
Outdated
| with: | ||
| commit-starts-with: 'Release [version],Release v[version],Release/[version],Release/v[version],Release `[version]`' | ||
| commit-message: ${{ github.event.pull_request.title }} | ||
| before: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha || github.event.before }} |
There was a problem hiding this comment.
github.event.before is only defined on push, not on pull_request or merge_group, so I had to add this.
There was a problem hiding this comment.
base.sha doesn't seem correct here.
In the action-is-release action, we used github.event.before to get the diff of what changed in a given commit, to check whether the version number had changed. For the push event, the diff betweeen github.event.before and the current commit would only include changes from that push event.
Whereas base.sha refers to the commit on the base branch that the PR was originally branched from. It does not get updated when the branch is updated with a merge commit. That means if a core release goes out while a PR is open, github.event.pull_request.base.sha..HEAD will include a version bump from the unrelated core release on main.
Instead I think we need the merge-base here, between the base branch and the current commit
There was a problem hiding this comment.
Either that, or get the merge-base in action-is-release instead
| needs: check-workflows | ||
| uses: ./.github/workflows/lint-build-test.yml | ||
|
|
||
| check-release: |
There was a problem hiding this comment.
I didn't add a conditional here, but only to the step below. This way we can add check-release as dependency to all-jobs-complete. Otherwise, if check-release (the job) is skipped, all-jobs-complete would be skipped too.
| done | ||
| # Get all files changed ahead of this PR. | ||
| git diff --name-only "$BEFORE..$(git merge-base HEAD refs/remotes/origin/main)" > changed-files.txt |
There was a problem hiding this comment.
From my understanding, git merge-base HEAD refs/remotes/origin/main finds the common ancestor between HEAD (i.e., the current commit), and origin/main. We can then diff $BEFORE with that to find any differences on main not included in the current branch.
There was a problem hiding this comment.
This is an action rather than a workflow to support the uses syntax in a step, rather than as a job. It's related to this comment.
Mrtenz
force-pushed
the
mrtenz/merge-queue-release-validation
branch
from
d677951 to
56f08cb
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| EVENT_NAME: ${{ github.event_name }} | ||
| run: | | ||
| if [ "$EVENT_NAME" = "pull_request" ]; then | ||
| echo "TARGET=$(git merge-base HEAD refs/remotes/origin/main)" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Bug: Wrong Target Prevents PR Conflict Detection
For pull_request events, TARGET is set to the merge-base of HEAD and main, but it should be the current state of main itself. Since BEFORE (line 64) is also a merge-base, the diff comparison at line 65 will compare two merge-bases that are likely identical, resulting in an empty diff that fails to detect conflicts with changes that landed on main after the PR was created.
There was a problem hiding this comment.
The merge-base of HEAD and main should nearly always be identical to main, except in the case where the a new commit was made to main since the workflow began. In that scenario, the merge base of HEAD is correct here.
| elif [ "$EVENT_NAME" = "merge_group" ]; then | ||
| echo "TARGET=$(git rev-parse HEAD^)" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "::error::This action only supports `pull_request` and `merge_group` events." |
There was a problem hiding this comment.
When running this through shellcheck.net, it's suggesting that Bash is trying to execute what's in these backticks. Escaping it should ensure they don't get interpreted as a command.
Alternatively we could use a single quotes here instead
| echo "::error::This action only supports `pull_request` and `merge_group` events." | |
| echo "::error::This action only supports \`pull_request\` and \`merge_group\` events." |
Gudahtt
left a comment
Gudahtt
left a comment
There was a problem hiding this comment.
Looks great! Just one minor suggestion
Mrtenz
force-pushed
the
mrtenz/merge-queue-release-validation
branch
from
375f5ed to
babd67a
Compare
Mrtenz
changed the title
…7162) ## Explanation This adds some logic to #7114 to add a comment to a pull request if a conflict was detected, either in the pull request checks or in the merge queue. Old comments will be hidden automatically as well: <img width="933" height="475" alt="image" src="https://github.com/user-attachments/assets/94153654-9aea-4b97-b67a-19205f8fdb7c" /> ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Enhances the release-conflict check to post a PR comment (minimizing old ones), expose outputs for conflicted package names, and conditionally fail; updates workflow permissions. > > - **CI / Release conflict checks**: > - **Action `check-release`** (`.github/actions/check-release/action.yml`): > - Emits outputs `package-names` and `has-conflicts` instead of exiting immediately upon conflicts; final separate step now fails when conflicts exist. > - Adds `id: check-release` to expose outputs. > - Posts a PR comment listing conflicted packages and notes merge-queue context; hides previous such comments via GraphQL minimization. > - **Workflow** (`.github/workflows/main.yml`): > - Grants `contents: read` and `pull-requests: write` permissions to `check-release` job to allow commenting. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 3fe207d. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Explanation
This adds the
merge_grouptarget and adds a check for releases, so we can safely enable the merge queue.The release check will check for any differences on
mainor in the merge queue compared to the release branch. If any conflicts are detected, the release will be rejected from the merge queue, and will need to be updated with the latest changes first.References
Checklist
Note
Add merge queue support and a release-conflict check action integrated into the main workflow for release PRs.
merge_grouptrigger to.github/workflows/main.yml.check-releasejob to detect PR/merge-queue releases and run conflict checks.MetaMask/action-is-releaseto detect release PRs and extracts PR number (supports merge queue refs).all-jobs-completenow needscheck-release)..github/actions/check-release/action.ymlto detect release conflicts across packages.main, computes changed files ahead of the PR/merge group target, and errors on conflicts.pull_requestandmerge_groupevents; fetches PR branch viaghand compares against calculated target.Written by Cursor Bugbot for commit babd67a. This will update automatically on new commits. Configure here.