-
-
Notifications
You must be signed in to change notification settings - Fork 4
INFRA-3041: Stable sync workflow fixes #154
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>
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 the PR, I added a few comments.
.github/scripts/stable-sync.js
Outdated
|
|
||
| // Preserve package.json from stable branch (no version bump) | ||
| await exec(`git checkout origin/${baseBranch} -- package.json`); | ||
| console.log(`Executed: git checkout origin/${baseBranch} -- package.json`); |
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 don't need to preserve package.json from stable branch. We can delete this line which will leave us without extension-specific commands.
For reference, we want to reproduce the logic that's currently in stable-sync-legacy.js script, i.e. create a stable-sync branch that will be merged into main while:
- Bringing commit history from
stable(the cherry-picked commits) intomain - Bringing CHANGELOG.md updates from
stableintomain - Avoiding code conflicts because the code changes are already in
main(they were cherry-picked to create the release)
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.
Ok, I was under the impression we wanted to preserve it. Will remove the extra logic here. Thanks
.github/workflows/stable-sync.yml
Outdated
| git config submodule.recurse false | ||
| git config diff.ignoreSubmodules all | ||
| # Start the rebase |
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 wonder whether we really need a rebase. Can we not just override the origin/stable-main-${{ inputs.semver-version }} branch with a git push force?
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.
Good call.
.github/workflows/stable-sync.yml
Outdated
| # Check if branch exists remotely | ||
| echo "Cleaning up github-tools" | ||
| rm -rf github-tools | ||
| # Ensure github-tools is not staged or tracked |
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 wonder where the github-tools file comes from. Maybe instead of introducing all this logic to avoid including it in the commit, we could just add it to .gitignore?
| echo "github-tools/" >> .gitignore | ||
| echo "Added github-tools/ to .gitignore" | ||
| fi | ||
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: Gitignore Overwritten by Git Commands Reinstating Exclusions
The workflow adds github-tools/ to .gitignore to prevent its commitment. However, the stable-sync.js script's git commands restore files from origin/main, overwriting .gitignore and undoing the exclusion. This means github-tools/ may still be committed.
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.
@XxdpavelxX is this a valid 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.
No I don't think it's relevant. We're adding github-tools/ to .gitignore in mobile, extension also so it shouldn't be an issue once we do that.
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** Ticket: https://consensyssoftware.atlassian.net/browse/INFRA-3041 Github-tools PR: MetaMask/github-tools#154 Summary of Changes Prevented github-tools from appearing in PRs File: github-tools/.github/workflows/stable-sync.yml What: Added explicit cleanup commands to unstage and remove github-tools directory before pushing Impact: The sync PR will no longer include a github-tools/ submodule or directory Disabled package.json version bump for Extension File: github-tools/.github/scripts/stable-sync.js What: Removed the yarn version logic that bumped package.json version Before: Extension ran yarn version to update the version After: Extension now preserves package.json from stable branch (same as mobile) Impact: No version bump will occur in the sync PR for either mobile or extension Verified PR title format File: github-tools/.github/workflows/stable-sync.yml (line 147) What: Confirmed it already uses release: prefix (no change needed) Format: "release: sync stable to main for version X.Y.Z" Testing in Extension: consensys-test#209, https://github.com/consensys-test/metamask-extension-test-workflow2/actions/runs/18951817173 Testing in Mobile: consensys-test/metamask-mobile-test-workflow#30, https://github.com/consensys-test/metamask-mobile-test-workflow/actions/runs/18951842114 Testing Post CR comments: consensys-test#212 ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: None ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Bumps stable-branch sync workflow to a new github-tools ref and ignores the `github-tools/` directory. > > - **CI/Workflows**: > - Update `.github/workflows/stable-branch-sync.yml` to use `metamask/github-tools/.github/workflows/stable-sync.yml@79ce6e3` and set `github-tools-version` to `79ce6e3`. > - **Repo hygiene**: > - Add `github-tools/` to `.gitignore`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit f8418e5. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Ticket: https://consensyssoftware.atlassian.net/browse/INFRA-3041
Summary of Changes
File: github-tools/.github/workflows/stable-sync.yml
What: Added explicit cleanup commands to unstage and remove github-tools directory before pushing
Impact: The sync PR will no longer include a github-tools/ submodule or directory
File: github-tools/.github/scripts/stable-sync.js
What: Removed the yarn version logic that bumped package.json version
Before: Extension ran yarn version to update the version
After: Extension now preserves package.json from stable branch (same as mobile)
Impact: No version bump will occur in the sync PR for either mobile or extension
File: github-tools/.github/workflows/stable-sync.yml (line 147)
What: Confirmed it already uses release: prefix (no change needed)
Format: "release: sync stable to main for version X.Y.Z"
Testing in Extension: consensys-test/metamask-extension-test-workflow2#209, https://github.com/consensys-test/metamask-extension-test-workflow2/actions/runs/18951817173
Testing in Mobile: consensys-test/metamask-mobile-test-workflow#30, https://github.com/consensys-test/metamask-mobile-test-workflow/actions/runs/18951842114
Testing Post CR comments: consensys-test/metamask-extension-test-workflow2#212
Note
Stabilizes the sync process by ignoring github-tools in PRs, removing the extension version bump, and force-pushing existing stable sync branches.
Workflows:
stable-sync.yml:github-tools/to.gitignoreto prevent committing it.stable-main-<version>branch exists; set upstream on first push.Scripts:
github-tools/.github/scripts/stable-sync.js:package.jsonversion bump; only run mobile-specific file checkouts.Written by Cursor Bugbot for commit ee74d68. This will update automatically on new commits. Configure here.