-
-
Notifications
You must be signed in to change notification settings - Fork 4
feat(INFRA-3081): add --require-pr-numbers flag for generation-time filtering #253
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
d411f4a to
7cc079b
Compare
7cc079b to
3a65103
Compare
…ut PR numbers - Add requirePrNumbers option to AddNewCommitsOptions type - Add requirePrNumbers parameter to getNewChangeEntries function - Filter commits without PR numbers before deduplication when enabled - Add requirePrNumbers option to UpdateChangelogOptions type - Pass requirePrNumbers through updateChangelog to getNewChangeEntries - Add --requirePrNumbers CLI flag with description - Update README with documentation and examples
9e9b647 to
5bf51ec
Compare
When commits reference PRs from forked repositories (e.g., original MetaMask repo PRs in a fork), the GitHub API returns 404. Instead of throwing an error, we now return empty labels and continue processing. This allows the --requirePrNumbers flag to work correctly in forked repositories.
HTML comments from PR templates were breaking the markdown rendering of changelogs. This adds a helper function to strip both complete and unclosed HTML comments from changelog entry descriptions.
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 implements the --requirePrNumbers flag to enable generation-time filtering of commits without PR numbers, supporting stricter PR-based changelog workflows. The implementation follows the codebase's existing patterns for boolean options (defaulting to false for backward compatibility) and threads the parameter through the entire changelog update pipeline. However, the PR includes two unrelated changes (stripHtmlComments function and error handling in getPrLabels) that should be addressed separately, and the main feature lacks test coverage.
Key Changes
- Added
requirePrNumbersparameter to filter commits without PR numbers during changelog generation - Added filtering logic in
getNewChangeEntriesto exclude non-PR commits when the flag is enabled - Added CLI flag
--requirePrNumberswith documentation and examples
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/get-new-changes.ts | Added filtering logic to exclude commits without PR numbers; also includes unrelated stripHtmlComments function and error handling changes |
| src/update-changelog.ts | Added requirePrNumbers parameter to type definitions and function signatures, passed through to getNewChangeEntries |
| src/cli.ts | Added --requirePrNumbers CLI option with proper type definitions and wiring through the update command |
| README.md | Added usage examples and documentation for the new --requirePrNumbers flag |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
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 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
bugbot run |
…entries The existing case-insensitive check at line 147 only prevented processing, but the filter at line 162 was case-sensitive, allowing 'Null' and 'NULL' entries to leak through into the changelog.
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.
PR looks great, thanks!
## **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, even direct commits without PR numbers. For projects following strict PR-based workflows (like MetaMask Extension), these direct commits should be excluded to ensure all changelog entries represent reviewed changes.What is the solution your changes offer and how does it work?
1. New
--requirePrNumbersFlagAdds an opt-in CLI flag that filters out commits without PR numbers at generation time. Defaults to
falsefor backward compatibility.Files changed:
cli.ts,update-changelog.ts,get-new-changes.ts2. Graceful 404 Handling for Forked Repos
When running on forked repositories, the tool fetches PR details from the fork. PRs originating from the upstream repo return 404 errors. Added
try-catchto return empty labels instead of crashing.3. HTML Comment Stripping
PR templates often contain HTML comments (
<!-- ... -->) that were breaking markdown rendering in changelogs. AddedstripHtmlComments()function to clean descriptions.4.
CHANGELOG entry: nullFixPRs marked with
CHANGELOG entry: nullwere appearing as "Null (#PR)" due to:null)Fixed by adding
.trim()and case-insensitive comparison at both check points.Usage
Testing
Tested with consensys-test/metamask-extension-test on
release/1100.0.0:feat: test commit with PR for 1100.0.0 (#40)chore: direct commit without PR for 1100.0.0Null (#PR)entriesChangelog Excerpt
Note:
N/AandNoneentries still appear (e.g.,N/A (#37810)) because these are different strings fromnull- they're valid descriptions that PRs chose to use.References