-
-
Notifications
You must be signed in to change notification settings - Fork 4
Infra-2925: add merging GitHub action #172
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
Conversation
Signed-off-by: Pavel Dvorkin <pavel.dvorkin@consensys.net>
|
If this is intended to be used by other repositories, can you add it as composite action instead? |
| core.setFailed('Skipping: PR is not approved.'); | ||
| } else { | ||
| console.log('PR approval check passed.'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Approval check fails job instead of skipping merge
The approval verification step calls core.setFailed() when a PR lacks approval, which fails the entire job. This is inconsistent with the branch verification step which gracefully handles mismatches by setting a should_skip output. When approval is missing, the job should skip the merge rather than fail, allowing the workflow to complete successfully without executing the merge step.
gauthierpetetin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR @XxdpavelxX , I left 2 comments
| pull_number: ${{ inputs.pr-number }} | ||
| }); | ||
|
|
||
| // Check if there is at least one review with the state 'APPROVED' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check that the review was done by a member of the release team: https://github.com/orgs/MetaMask/teams/release-team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Do you know if the release-team specified in code somewhere, or do we have an existing mechanism to get the release-team list in gh actions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requiring a release team review would be ideal (certainly we expect the release team to be the reviewer), but it does not seem critical here. We allow anybody to review sync PRs today, that can remain the case for now.
Does that sounds OK @gauthierpetetin ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nevermind, I see you found a way to do it, this looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm experimenting with a way. But will need to use a PAT token to test this since github auth tokens don't have read:org permissions to fetch team members
| core.setOutput('base', pr.base.ref); | ||
| core.setOutput('head', pr.head.ref); | ||
|
|
||
| # Verify that the PR targets 'main' from 'stable' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not really merge 'stable' to 'main'.
Here are the different use cases where a merge commit is needed:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the discussion in this ticket it sounded like we want to merge stable into main. https://consensyssoftware.atlassian.net/browse/INFRA-2925.
Maybe this was referring to the first, Stable sync example you posted? If the desired branch syntax is stable-main-x.y.z -> main I can update the naming convention in the PR. And does this apply to both Mobile and Extension? @gauthierpetetin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gauthier is correct. We are kinda merging "stable" into "main", but we have to use an intermediary branch to resolve conflicts (and because merging between long-running branches directly is just confusing in the GitHub UI if you try to do it, it puts delete branch buttons in the PR that we really don't want for example).
This applies to both mobile and extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've used a few different names for this intermediary branch in the past, but using the format Gauthier listed sounds good to me. Presumably that's what the stable sync automation we already have uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it to do stable-main-x.y.z -> main merges
| @@ -0,0 +1,132 @@ | |||
| name: Merge Approved PR | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Since this only handles the stable sync PR right now, we can make this a bit more specific.
| name: Merge Approved PR | |
| name: Merge Approved Stable Sync PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, maybe we could make this workflow more generic and allow passing in patterns for expected target and base branches?
We do need to handle syncs between two release PRs anyway, so that would better prepare for that work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to start with passing specific branches and have that working. Then experiment and improve on that in the followup task. https://consensyssoftware.atlassian.net/browse/INFRA-3000 to see if it's better to make this more generic or keep it specific and perhaps in separate workflow.
Gudahtt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I had a couple of minor suggestions
Gudahtt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I still think the name isn't ideal for what the workflow currently does, but not a big deal
Allows you to merge stable -> main PRs on comment "Merge my PR" in Mobile/Extension
Task: https://consensyssoftware.atlassian.net/browse/INFRA-2925
PAT token generated in: https://github.com/MetaMask/patroll/pull/25
Successful Negative tests:
Not Merging on Incorrect comment: consensys-test/metamask-mobile-test-workflow#53, https://github.com/consensys-test/metamask-mobile-test-workflow/actions/runs/19575032129
Not Merging from wrong source branch: consensys-test/metamask-mobile-test-workflow#53, https://github.com/consensys-test/metamask-mobile-test-workflow/actions/runs/19575100357/job/56058172525
Not Merging into wrong target branch: consensys-test/metamask-mobile-test-workflow#54, https://github.com/consensys-test/metamask-mobile-test-workflow/actions/runs/19575460777/job/56059417843
Not Merging on unapproved PR: consensys-test/metamask-mobile-test-workflow#55, https://github.com/consensys-test/metamask-mobile-test-workflow/actions/runs/19575204618/job/56058536264
Desired Successful test: consensys-test/metamask-mobile-test-workflow#53, https://github.com/consensys-test/metamask-mobile-test-workflow/actions/runs/19575307260
Note
Adds a reusable GitHub Actions workflow that merges
stable-main-x.y.zPRs intomainonly after release-team approval./.github/workflows/merge-approved-pr.yml:stable-main-X.Y.Z-> basemain.release-teamand no outstanding change requests.Written by Cursor Bugbot for commit 276dff9. This will update automatically on new commits. Configure here.