-
-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add automated dependency bump checker and changelog validator #186
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
base: main
Are you sure you want to change the base?
Conversation
a88a703 to
e9b5b6c
Compare
e9b5b6c to
abcda3f
Compare
|
@cryptodev-2s I haven't had time to review this yet, but I have one initial thought: Should we rename |
Good point about future validation commands! However, I think
Suggestion:
Side note: Given we're adding more commands beyond release creation, we could consider renaming the package to something like |
Introduces a new tool to automatically detect dependency version changes and validate/update changelog entries accordingly. Features: - Detects dependency bumps from git diffs in package.json files - Validates changelog entries with exact version matching - Automatically updates changelogs with missing or outdated entries - Smart PR reference concatenation when updating existing entries - Dynamically reads repository URLs and package names - Validates by default with optional --fix flag for updates Usage: yarn check-dependency-bumps # Validate changelogs yarn check-dependency-bumps --fix # Auto-update changelogs yarn check-dependency-bumps --fix --pr 1234 # With PR number
Optimizes package name resolution by reading package.json inline during git diff parsing instead of in a separate enrichment pass. Changes: - Make parseDiff async to read package names inline - Remove enrichWithPackageNames function (no longer needed) - Read packageName immediately when first encountering a package - Simplify validateChangelogs and updateChangelogs signatures - Remove packageNames parameter (now part of PackageInfo) Benefits: - Single-pass processing (parse + enrich in one step) - Simpler code flow (24 lines removed) - Better data locality (package info complete at creation) - Cleaner API (functions receive unified PackageChanges structure) Test coverage maintained: 100% (339 passing tests)
9b796b6 to
88d116a
Compare
|
@metamaskbot publish-preview |
|
A preview build for this branch has been published. You can configure your project to use the preview build with this identifier: See these instructions for more information about preview builds. |
1 similar comment
|
A preview build for this branch has been published. You can configure your project to use the preview build with this identifier: See these instructions for more information about preview builds. |
Hmm. Currently this tool is centered around release management, so adding code that doesn't strictly relate to releases feels wrong. I did see your note about renaming this tool to |
BREAKING entries (peerDependencies) now appear before regular dependencies, both alphabetically ordered in final changelog output.
|
@metamaskbot publish-preview |
|
A preview build for this branch has been published. You can configure your project to use the preview build with this identifier: See these instructions for more information about preview builds. |
hasChangelogEntry now checks for **BREAKING:** prefix when matching peerDependencies entries, preventing same dependency in both sections from matching the wrong entry. This fixes the bug where updating both entries would fail because both matched the first entry found.
When validating changelogs for a release version, the error message now correctly shows the version section (e.g., [1.2.3]) instead of always showing [Unreleased].
Automatically detect package rename info from package.json scripts and pass it to parseChangelog to correctly handle changelogs with old package name tags.
When updating existing entries and adding new ones for renamed packages, the second parseChangelog call was missing the packageRename parameter. This ensures both calls include packageRename for consistency.
Remove restrictive '@' filter that was silently ignoring non-scoped packages like lodash, react, and typescript. The regex pattern already handles both scoped and non-scoped packages correctly.
a40520c to
0d1de51
Compare
fcf0548 to
23d29bf
Compare
23d29bf to
259d980
Compare
CHANGELOG.md
Outdated
| - Auto-updates changelogs with `--fix` flag, preserving PR history | ||
| - Detects package releases and validates/updates in correct changelog section (Unreleased vs specific version) | ||
| - Smart PR concatenation when same dependency bumped multiple times | ||
| - Usage: `yarn check-dependency-bumps --fix --pr <number>` |
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.
This is how we'd call the script from other repos? How is that possible when this isn't registered in package.json as a bin script?
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.
The command was incorrect, this step is actually handled by the create-release-branch CLI. I’ve just fixed the changelog accordingly.
By the way, I’m in the process of moving this over to auto-changelog instead. Here’s the draft PR: MetaMask/auto-changelog#267, I’ll mark it ready shortly.
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 I see, didn't see this comment until now. Maybe this should be moved back into draft then?
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 worries, I will move it back to draft and apply all the suggestions you mentioned here. They are also relevant for auto-changelog, since much of the logic is moved.
Thank you for the review!
|
|
||
| // Track current file | ||
| if (line.startsWith('diff --git')) { | ||
| const match = line.match(/b\/(.+)/u); |
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: File path regex fails for paths containing /b/ directory
The regex /b\/(.+)/u used to extract file paths from git diff headers matches the first occurrence of b/ in the line. For a diff line like diff --git a/packages/b/package.json b/packages/b/package.json, the regex incorrectly matches the b/ inside packages/b/ and captures package.json b/packages/b/package.json instead of the intended packages/b/package.json. The regex could be changed to b\/(.+)$/u to match the space-prefixed b/ that marks the destination path at the end of the line.
|
|
||
| export type CommandLineArguments = { | ||
| export type ReleaseCommandArguments = { | ||
| _: string[]; |
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.
Was is this for? Do we need to return this from readCommandLineArguments? It doesn't seem to be used anywhere.
| defaultBranch: args.defaultBranch, | ||
| interactive: args.interactive, | ||
| port: args.port, | ||
| } as ReleaseCommandArguments; |
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 see that this type assertion can be removed if you undo the tempDirectory type change (back to string | undefined). Is there a reason that type change was made? Ideally we'd avoid type assertions if we can.
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 see that the CheckDepsCommandArguments type assertion is there for the same reason.
| * Represents a single dependency version change | ||
| */ | ||
| export type DependencyChange = { | ||
| package: string; |
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: Descriptions would be helpful for these properties
| * @param options - Configuration options. | ||
| * @param options.fromRef - The starting git reference (optional). | ||
| * @param options.toRef - The ending git reference (defaults to HEAD). | ||
| * @param options.defaultBranch - The default branch to compare against (defaults to main). |
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: Should we call this baseBranch instead? The term defaultBranch suggests that this command is primarily meant for feature branches directed at main. But there is nothing about this workflow that seems to care whether the base branch is the default branch for the repo or not.
| // Auto-detect branch changes if fromRef not provided | ||
| if (!actualFromRef) { | ||
| const currentBranch = await getCurrentBranchName(projectRoot); | ||
| stdout.write(`\n📌 Current branch: ${currentBranch}\n`); |
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: Some of these messages could be pretty confusing if one of the parameters gets set to an empty string; it might return a warning message that doesn't tip us off to the problem. For example, if currentBranch was an empty string, this would suggest that the next line is the branch name, which it isn't. And with some of the later messages, inputting an empty string as the default branch name would be similarly confusing.
I have a habit of wrapping embedded parameters like this in single-quotes, so it's clear that there is something here in case it gets set to an unexpected value like an empty string, e.g.:
| stdout.write(`\n📌 Current branch: ${currentBranch}\n`); | |
| stdout.write(`\n📌 Current branch: '${currentBranch}'\n`); |
Perhaps that would be helpful for these messages too
| { cwd: projectRoot }, | ||
| ); | ||
| } catch { | ||
| // If local branch doesn't exist, try remote |
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: This would end up being misleading a lot of the time, e.g. if main was out-of-date locally, the first attempt would succeed and give the wrong answer.
It also feels unexpected for this function to make guesses about what the default branch is. main and origin/main are different branches. If we're going to auto-prefix a remote name, at the very least we should document that the function will do that.
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 would suggest that this function accept the default branch as-is, but make the default ${remote}/main instead of main.
And speaking of which, we should also allow customizing the remote name. Using something other than origin is not uncommon.
| try { | ||
| return await getStdoutFromCommand( | ||
| 'git', | ||
| ['merge-base', 'HEAD', defaultBranch], |
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: I see we're using merge-base in a similar manner in the restoreFiles function in src/repo.ts. Maybe we can consolidate the two implementations by using a utility function?
| 'git', | ||
| [ | ||
| 'diff', | ||
| '-U9999', // Show maximum context to ensure section headers are visible |
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: 😅 sorry it took me way too long to understand what a "section header" was in this context. Here's a suggestion to maybe make it clearer
| '-U9999', // Show maximum context to ensure section headers are visible | |
| '-U9999', // Show maximum context to ensure full dependency lists are visible |
| * @param projectRoot - The project root directory. | ||
| * @returns The raw git diff output. | ||
| */ | ||
| async function getGitDiff( |
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: We should perhaps call this something less misleading, like getManifestGitDiff?
| { cwd: projectRoot }, | ||
| ); | ||
| } catch (error: any) { | ||
| // Git diff returns exit code 1 when there are no changes |
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.
Does it? It doesn't seem to when I've tried it.
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.
It looks like it only does this if you pass the --exit-code flag, which we haven't done here.
Description
Adds a new
check-depscommand to automatically detect, validate, and update dependency bump entries in CHANGELOGs.Key Features
package.jsonfiles--fixflag## [X.Y.Z]section when package version changes, or[Unreleased]otherwisepackage.jsonscripts to correctly parse changelogs with old package name tagsExample:
Implementation
New files:
src/check-dependency-bumps.ts+ tests (24 tests)src/changelog-validator.ts+ tests (27 tests)Modified:
src/command-line-arguments.ts- Addedcheck-depscommandsrc/main.ts- Command routingCoverage: 100% (statements, branches, functions, lines) - 340 passing tests
Testing in MetaMask/core
Note
Introduces
check-depsto detect dependency bumps from git diffs and validate/update package changelogs (incl. releases, peerDeps BREAKING, and package renames).check-depscommand viayargsinsrc/command-line-arguments.ts; route insrc/main.ts.src/check-dependency-bumps.tsto parsegit diffof**/package.json, detectdependencies/peerDependenciesversion bumps (skip dev/optional), dedupe, and capture package version changes.src/changelog-validator.tsto validate and update changelog entries:**BREAKING:**).#XXXXXwhen missing).[Unreleased]or a specific[X.Y.Z]section.--tag-prefix-before-package-rename,--version-before-package-rename).src/types.tsforDependencyChange,PackageInfo,PackageChanges.CHANGELOG.mdwith new command and usage.Written by Cursor Bugbot for commit 703e1c3. This will update automatically on new commits. Configure here.