-
-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add require-pr-numbers flag for changelog generation #181
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
Add --requirePrNumbers flag to filter out direct commits without PR numbers from changelogs. This helps maintain changelog quality for projects using strict PR-based workflows. Changes: - Add require-pr-numbers input to update-release-changelog and create-release-pr actions - Make --requirePrNumbers flag conditional based on input (defaults to false) - Add merge logic to sync changelog branch with release branch updates Requires @metamask/auto-changelog@5.3.0 with --requirePrNumbers support.
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.
Pull request overview
This PR adds a new require-pr-numbers flag to changelog generation actions, enabling filtering of commits without PR numbers to ensure all changelog entries represent reviewed changes. This is important for projects following strict PR-based workflows where direct commits should be excluded from changelogs.
Key changes:
- Added
require-pr-numbersinput parameter (default:false) to bothupdate-release-changelogandcreate-release-practions - Modified shell scripts to conditionally add
--requirePrNumbersflag when enabled - Added merge logic in
checkout_or_create_branch()to sync changelog branch with release branch updates
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
.github/actions/update-release-changelog/action.yml |
Added require-pr-numbers input and passed it to the shell script |
.github/actions/create-release-pr/action.yml |
Added require-pr-numbers input and passed it to the shell script |
.github/scripts/update-release-changelog.sh |
Added parameter handling, conditional flag addition, and new merge logic in checkout_or_create_branch() |
.github/scripts/create-platform-release-pr.sh |
Added parameter handling and conditional flag addition for changelog generation |
Comments suppressed due to low confidence (2)
.github/scripts/update-release-changelog.sh:226
- Using
evalwith a dynamically constructed command string poses a security risk if any of the variables contain special characters or malicious input. WhileGITHUB_REPOSITORY_URLandVERSIONare likely controlled, this pattern is generally discouraged.
Consider using an array to build the command and execute it directly:
CHANGELOG_CMD=(yarn auto-changelog update --rc --repo "${GITHUB_REPOSITORY_URL}" --currentVersion "${VERSION}" --autoCategorize --useChangelogEntry --useShortPrLink)
if [[ "${REQUIRE_PR_NUMBERS}" == "true" ]]; then
CHANGELOG_CMD+=(--requirePrNumbers)
fi
"${CHANGELOG_CMD[@]}"This approach is safer and more maintainable.
CHANGELOG_CMD="yarn auto-changelog update --rc --repo \"${GITHUB_REPOSITORY_URL}\" --currentVersion \"${VERSION}\" --autoCategorize --useChangelogEntry --useShortPrLink"
# Add --requirePrNumbers flag if enabled
if [[ "${REQUIRE_PR_NUMBERS}" == "true" ]]; then
CHANGELOG_CMD="${CHANGELOG_CMD} --requirePrNumbers"
fi
eval "${CHANGELOG_CMD}"
.github/scripts/create-platform-release-pr.sh:362
- Using
evalwith a dynamically constructed command string poses a security risk if any of the variables contain special characters or malicious input. While the variables are likely controlled in this context, this pattern is generally discouraged.
Consider using an array to build the command and execute it directly:
CHANGELOG_CMD=(yarn auto-changelog update --rc --repo "${GITHUB_REPOSITORY_URL}" --currentVersion "${new_version}" --autoCategorize --useChangelogEntry --useShortPrLink)
if [[ "${REQUIRE_PR_NUMBERS}" == "true" ]]; then
CHANGELOG_CMD+=(--requirePrNumbers)
fi
"${CHANGELOG_CMD[@]}"This approach is safer and more maintainable.
CHANGELOG_CMD="yarn auto-changelog update --rc --repo \"${GITHUB_REPOSITORY_URL}\" --currentVersion \"${new_version}\" --autoCategorize --useChangelogEntry --useShortPrLink"
# Add --requirePrNumbers flag if enabled
if [[ "${REQUIRE_PR_NUMBERS}" == "true" ]]; then
CHANGELOG_CMD="${CHANGELOG_CMD} --requirePrNumbers"
fi
eval "${CHANGELOG_CMD}"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Replace eval with direct command execution for security - Add platform-specific logic to create-platform-release-pr.sh (extension uses yarn, mobile uses npx with pinned version) - Clarify documentation that require_pr_numbers is ignored for mobile - Add detailed comment explaining merge logic rationale in update-release-changelog.sh (intentionally different from create-platform-release-pr.sh)
Update to use the published version with --requirePrNumbers support.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix typo: 'heckout_or_create_branch' -> 'checkout_or_create_branch' - Improve merge conflict handling: reset to HEAD (pre-merge state) instead of release branch to preserve existing changelog content - Add clearer logging for merge conflict scenario Note: Code duplication suggestion skipped (nitpick, over-engineering)
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.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Consolidate changelog generation for both extension and mobile platforms to use yarn auto-changelog. - Remove redundant merge logic in update-release-changelog.sh to simplify the process. - Ensure consistent handling of the --requirePrNumbers flag across scripts for improved clarity and functionality.
|
bugbot run |
- Fix bug: ${new_version} -> ${VERSION} in update-release-changelog.sh
- Add trailing newline to package.json for Prettier compliance
|
bugbot run |
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.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Standardize changelog generation for both platforms. The require-pr-numbers flag can now be used for both extension and mobile, though it's currently only enabled for extension.
|
bugbot run |
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.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
✅ Bugbot reviewed your changes and found no bugs!
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.
I'm tempted to say we don't even need a parameter for it, we can just assume we'll always call yarn auto-changelog update with --requirePrNumbers flag. That should simplify the code.
If @Gudahtt and mobile team agree, no problem from my side. |
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.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Remove conditional require-pr-numbers parameter and always apply the --requirePrNumbers flag to filter out direct commits. This simplifies the code as both extension and mobile teams have agreed to use this behavior by default.
|
bugbot run |
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.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
✅ Bugbot reviewed your changes and found no bugs!
## **Description** This PR updates the changelog workflows to use the new `@metamask/auto-changelog@5.3.0` which always filters out direct commits without PR numbers. This ensures all changelog entries represent reviewed and approved changes. **Changes:** - Update `@metamask/auto-changelog` from `^5.1.0` to `^5.3.0` - Update workflows to use `github-tools@v1.1.2` which includes the `--requirePrNumbers` flag enabled by default The `--requirePrNumbers` flag is now always applied by default in github-tools, so no additional configuration is needed. [](https://codespaces.new/MetaMask/metamask-extension/pull/38520?quickstart=1) ## **Changelog** CHANGELOG entry: null ## **Related issues** - ✅ [MetaMask/auto-changelog#253](MetaMask/auto-changelog#253) - Merged - ✅ [MetaMask/github-tools#181](MetaMask/github-tools#181) - Merged and released as v1.1.2 ## **Manual testing steps** 1. Tested on [consensys-test/metamask-extension-test](https://github.com/consensys-test/metamask-extension-test) with `release/1100.0.0` 2. Verified PR commits are included in changelog 3. Verified direct commits are excluded from changelog 4. See [generated changelog](https://github.com/consensys-test/metamask-extension-test/blob/release/1100.0.0-Changelog/CHANGELOG.md) ## **Screenshots/Recordings** ### **Before** Direct commits without PR numbers would appear in the changelog. ### **After** Only commits with PR numbers (representing reviewed changes) appear in the changelog. ## **Pre-merge author checklist** - [x] 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). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] 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.
## **Description** This PR updates the changelog workflows to use the new `@metamask/auto-changelog@5.3.0` which always filters out direct commits without PR numbers. This ensures all changelog entries represent reviewed and approved changes. **Changes:** - Update `@metamask/auto-changelog` from `^5.1.0` to `^5.3.0` - Update `update-release-changelog` workflow to use `github-tools@v1.1.2` The `--requirePrNumbers` flag is now always applied by default in github-tools, so no additional configuration is needed. This aligns `metamask-mobile` with `metamask-extension` for consistent changelog generation across platforms. ## **Changelog** CHANGELOG entry: null ## **Related issues** - Related to: [MetaMask/metamask-extension#38520](MetaMask/metamask-extension#38520) - ✅ [MetaMask/auto-changelog#253](MetaMask/auto-changelog#253) - Merged (v5.3.0) - ✅ [MetaMask/github-tools#181](MetaMask/github-tools#181) - Merged and released as v1.1.2 ## **Manual testing steps** ```gherkin Feature: Changelog generation with PR number filtering Scenario: Only PR commits appear in changelog Given a release branch exists When a commit with an associated PR is merged Then the changelog should include that commit When a direct commit without PR is pushed Then the changelog should NOT include that commit ``` Tested on [consensys-test/metamask-extension-test](https://github.com/consensys-test/metamask-extension-test) with `release/1100.0.0`: - [Generated changelog](https://github.com/consensys-test/metamask-extension-test/blob/release/1100.0.0-Changelog/CHANGELOG.md) ## **Screenshots/Recordings** ### **Before** Direct commits without PR numbers would appear in the changelog. ### **After** Only commits with PR numbers (representing reviewed changes) appear in the changelog. ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] 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-mobile/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.
This PR updates the changelog workflows to use the new `@metamask/auto-changelog@5.3.0` which always filters out direct commits without PR numbers. This ensures all changelog entries represent reviewed and approved changes. **Changes:** - Update `@metamask/auto-changelog` from `^5.1.0` to `^5.3.0` - Update `update-release-changelog` workflow to use `github-tools@v1.1.2` The `--requirePrNumbers` flag is now always applied by default in github-tools, so no additional configuration is needed. This aligns `metamask-mobile` with `metamask-extension` for consistent changelog generation across platforms. CHANGELOG entry: null - Related to: [MetaMask/metamask-extension#38520](MetaMask/metamask-extension#38520) - ✅ [MetaMask/auto-changelog#253](MetaMask/auto-changelog#253) - Merged (v5.3.0) - ✅ [MetaMask/github-tools#181](MetaMask/github-tools#181) - Merged and released as v1.1.2 ```gherkin Feature: Changelog generation with PR number filtering Scenario: Only PR commits appear in changelog Given a release branch exists When a commit with an associated PR is merged Then the changelog should include that commit When a direct commit without PR is pushed Then the changelog should NOT include that commit ``` Tested on [consensys-test/metamask-extension-test](https://github.com/consensys-test/metamask-extension-test) with `release/1100.0.0`: - [Generated changelog](https://github.com/consensys-test/metamask-extension-test/blob/release/1100.0.0-Changelog/CHANGELOG.md) Direct commits without PR numbers would appear in the changelog. Only commits with PR numbers (representing reviewed changes) appear in the changelog. - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] 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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [ ] 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.
What is the current state of things and why does it need to change?
Currently,
auto-changelogincludes all commits in the changelog regardless of whether they went through a PR review process. Direct commits without PR numbers should be excluded to ensure all changelog entries represent reviewed and approved changes.Additionally, the changelog generation was inconsistent between platforms (extension vs mobile), using different tool versions and flags.
What is the solution your changes offer and how does it work?
This PR standardizes changelog generation for both platforms:
--requirePrNumbers- Filters out direct commits without PR numbers by default@metamask/auto-changelog@5.3.0- Both platforms use the same version with consistent flagsChanges:
update-release-changelog.sh--requirePrNumbersflagcreate-platform-release-pr.sh--requirePrNumbersflagpackage.json/yarn.lock@metamask/auto-changelog@^5.3.0Simplified command (used for both platforms):
yarn auto-changelog update --rc \ --repo "${GITHUB_REPOSITORY_URL}" \ --currentVersion "${VERSION}" \ --autoCategorize \ --useChangelogEntry \ --useShortPrLink \ --requirePrNumbersRelated Links
@metamask/auto-changelog@5.3.0- See MetaMask/auto-changelog#253 ✅ Mergedrelease/1100.0.0Note
Standardizes changelog generation with consistent flags (including --requirePrNumbers) across scripts and bumps @metamask/auto-changelog to 5.3.0.
yarn auto-changelog updateinvocation with flags:--rc,--repo,--currentVersion,--autoCategorize,--useChangelogEntry,--useShortPrLink,--requirePrNumbers.create-platform-release-pr.shto use the standardized command.update-release-changelog.shto use the standardized command and removecommits.csvgeneration logic.@metamask/auto-changelogfrom^5.2.0to^5.3.0inpackage.jsonandyarn.lock.Written by Cursor Bugbot for commit f284967. Configure here.