changelog checker #54

jake-perkins · 2025-03-13T18:17:46Z

This Change introduces a new shared workflow that will be confused by metamask-mobile and metamask-extension.
It will enforce that incoming PRs have either a modification to the Unreleased Section of the README or a label on the PR ( no-changelog ) which will indicate theres no external facing changes users need to know about

https://github.com/MetaMask/MetaMask-planning/issues/3678

socket-security · 2025-03-13T20:24:17Z

New and updated dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@types/[email protected]	None	`0`	1.7 kB	types
npm/@types/[email protected]	None	`0`	20.4 kB	types
npm/@types/[email protected]	None	`0`	36.8 kB	types
npm/@types/[email protected]	None	`0`	5.63 kB	types
npm/@types/[email protected]	None	`0`	11.9 kB	types
npm/[email protected] ➜ 1.8.3	None	`+1`	2.15 MB	jasonsaayman
npm/[email protected]	None	`0`	53.5 kB	ungoldman
npm/[email protected]	None	`0`	621 kB	explodingcabbage
npm/[email protected]	filesystem	`0`	14.5 kB	nickewing
npm/[email protected]	None	`0`	17.5 kB	zuchka

View full report↗︎

socket-security · 2025-03-13T20:24:19Z

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert	Package	Note	Source	CI
New author	npm/[email protected]	New Author: nickewing Previous Author:	`yarn.lock`	🚫
Deprecated	npm/@types/[email protected]	Reason: This is a stub types definition. axios provides its own type definitions, so you do not need this installed.	`package.json` `yarn.lock`	⚠︎

View full report↗︎

Next steps

What is new author?

A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package.

Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights.

What is a deprecated package?

The maintainer of the package marked it as deprecated. This could indicate that a single version should not be used, or that the package is no longer maintained and any new vulnerabilities will not be fixed.

Research the state of the package and determine if there are non-deprecated versions that can be used, or if it should be replaced with a new, supported solution.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

@SocketSecurity ignore npm/[email protected]
@SocketSecurity ignore npm/@types/[email protected]

makemesteaks · 2025-03-14T22:24:34Z

src/changelog-check.ts

+  console.log("🔍 Diff between base and feature '[Unreleased]' sections:");
+
+  if (removedLines.length > 0) {
+    console.log('❌ Removed:');


use core library instead?

src/changelog-check.ts

Gudahtt · 2025-03-20T15:38:44Z

.github/actionlint.yml

+  .github/workflows/changelog-check.yml:
+    ignore:
+      # Ignore SC2086 warnings related to shell commands
+      - 'SC2086:.+'


Why ignore these warnings? String splitting is the cause of many Bash script bugs

It's because the shell gh actions scripts yml is handling expansion of github variables which in the context of running github action's can't be done syntactically without triggering these warnings, the CI check for these is assuming you're running a vanilla shell script but in GH actions the expansion of variables is done before it's passed to shell but the linter doesn't / cant know that so it flags

Gudahtt · 2025-03-20T15:42:44Z

src/changelog-check.ts

+  filePath?: string;
+  text?: string;
+  // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires
+}) => Promise<Changelog> = require('changelog-parser');


Why bring in a third-party library for parsing changelogs when we have our own? @metamask/auto-changelog can do this, and is written specifically for the format we use

The parsing and validation becomes trivial with auto-changelog, we could remove most of this script.

Parsing would essentially become

const unreleasedChanges = parseChangelog({ changelogContent, repoUrl }).getReleaseChanges('Unreleased')

(keyed by category, but you can Object.values(...).flat() to get an array of string change entries).

No need for a custom changelog library or diff library.

src/changelog-check.ts

Gudahtt · 2025-03-20T16:24:48Z

src/changelog-check.ts

+      `❌ Error fetching CHANGELOG.md from ${branch} on ${repo}:`,
+      error,
+    );
+    return '';


Nit: Why return here rather than throw? It looks like this empty string would get passed along to `parseChangelog, which maybe would fail if given an empty string, not sure. But even there the error is caught and logged, letting the script continue.

We can't check a changelog that doesn't exist

Gudahtt · 2025-03-20T16:28:44Z

src/changelog-check.ts

+
+  if (baseChanges === featureChanges) {
+    console.log(
+      "❌ No new entries detected under '## Unreleased'. Please update the changelog.",


Two suggestions related to this:

This suggests that updating the changelog is the recommended way to address this error, but that's not necessarily true. We really don't want junk entries for non user-facing changes, that would make releases much more difficult rather than easier. We should be suggesting that the contributor either document user-facing changes, or add no-changelog to bypass the check if there are no user-facing changes.

Have you considered putting this in a comment rather than the console output? It will be much easier to see this feedback if it's in a comment, particularly as we first roll out this workflow.

itsyoboieltr · 2025-03-24T17:48:57Z

.github/workflows/changelog-check.yml

+            echo "Merge group event detected, auto-succeeding."
+            exit 0
+          fi
+          echo "Proceeding with pull request checks."


Why are we running it on the merge group event, if it is always skipped? Let's just run it only on PR instead?

It's an unfortunate side effect of pull_request and merge_group event types, if you run in only on pull_request but have merge queue enabled on your repository you won't be able to make it a required check as it will fail in the merge_group as an unresolved/omitted check.

If we don't want this to be a required check in the future then we can safely remove all the merge_group references

itsyoboieltr · 2025-03-24T17:49:45Z

.github/workflows/changelog-check.yml

+        type: string
+      event-type:
+        required: true
+        type: string


All of the inputs are already part of the gh context, they do not need to be passed down as inputs

not all context is propagated on workflow calls as it would normally be if you're running a typical job, this is a side effect of using workflow_call based flows

itsyoboieltr · 2025-03-24T17:50:30Z

.github/workflows/changelog-check.yml

+        if: ${{ inputs.event-type == 'pull_request' }}
+        run: yarn --immutable
+        shell: bash
+        working-directory: ./github-tools


Instead of all these steps, we could use the checkout and setup reusable workflow?

If unclear how it works, reach out to @HowardBraham

It would require some changes checkout-and-setup particularly around handing the directories/ .nvmrc files which was a bit more work then I originally wanted to sign up for and that one was being rolled out with it's new features while this one was under development. One of the challenges with these re-useable workflows that have code executed and especially if they need to perform work on the invoking repository's files is that we have multiple github repositories and codebases in context. I think the current checkout-and-setup has a 1 repository in mind approach which i think is correct that's generally the normal usage.

itsyoboieltr · 2025-03-24T17:52:44Z

.github/workflows/changelog-check.yml

+        with:
+          repository: MetaMask/github-tools
+          ref: changelog-checker
+          path: github-tools


Why is it in a subfolder instead of root?

Because you have two repositories in context invoking repository and the github-tools repo/base

itsyoboieltr · 2025-03-24T17:54:54Z

src/changelog-check.ts

+  const token = process.env.GITHUB_TOKEN ?? '';
+
+  try {
+    const response = await axios.get(url, {


why are we using axios instead of fetch? it would be nice if we could remove this unnecessary dependency (unless there is a very good reason to use it, that I cannot think about)

itsyoboieltr · 2025-03-24T17:56:16Z

src/changelog-check.ts

+  const featureChangelogContent = await fetchChangelogFromGitHub(
+    repo,
+    featureBranch,
+  );


these two should be able to be fetched in parallel:

await Promise.all([...])

itsyoboieltr · 2025-03-24T17:57:06Z

src/changelog-check.ts

+
+  if (!featureChangelogContent) {
+    console.error('❌ CHANGELOG.md is missing in the feature branch.');
+    throw new Error('❌ CHANGELOG.md is missing in the feature branch.');


Why is the same message displayed twice?

itsyoboieltr · 2025-03-24T17:57:20Z

src/changelog-check.ts

+
+  // Parse the changelogs
+  const baseChanges = await parseChangelog(baseChangelogContent);
+  const featureChanges = await parseChangelog(featureChangelogContent);


these two should be able to be parsed in parallel:

await Promise.all([...])

src/changelog-check.ts

Co-authored-by: Norbert Elter <[email protected]>

cryptodev-2s · 2025-04-01T19:31:56Z

@Gudahtt @itsyoboieltr I’ve opened a PR in Core for this same workflow action: MetaMask/core#5575.
I made sure to extract the shared workflow into a separate file so we can easily move it to github-tools later.
(cc @jake-perkins)

changelog checker

7b25c0d

metamaskbot added the team-dev-ops label Mar 13, 2025

jake-perkins added 3 commits March 13, 2025 13:27

updated syntax

5ab5c52

updated logic

8a89ebd

updates to ts approach

c8d98fd

jake-perkins added 23 commits March 13, 2025 15:29

remove pr number arg

f94591b

script updates

0e2c3dd

fmt

cc3acd8

lint

a2325bf

remove un-used dependencies

8c3846a

yarn.lock

2c7bbae

dedup

8338f2c

add label check

f61d45a

pr

4281ed0

fix label logic

6bb2bb5

syntax

c71a795

syntax

b2e4abf

fix

b32e2b9

syntax

778c758

changelog

1df1e5d

globbing lint

2f2ee48

diff

1d2a658

shellcheck

947179d

shellcheck

d49b71c

shellcheck

39d6379

test

7bdaf82

lint

1fcd744

testing

70ca876

jake-perkins added 5 commits March 14, 2025 14:36

lint

9e9ca3f

fmt

b8d06f3

fmt

fe80e61

fmt

c25f415

lint

265ca44

makemesteaks reviewed Mar 14, 2025

View reviewed changes

makemesteaks previously approved these changes Mar 14, 2025

View reviewed changes

makemesteaks reviewed Mar 14, 2025

View reviewed changes

src/changelog-check.ts Show resolved Hide resolved

Gudahtt reviewed Mar 20, 2025

View reviewed changes

itsyoboieltr requested changes Mar 24, 2025

View reviewed changes

chore: remove comments

3b79198

jake-perkins dismissed makemesteaks’s stale review via 3b79198 March 24, 2025 20:05

jake-perkins and others added 2 commits March 24, 2025 15:39

Update src/changelog-check.ts

4c8b7d4

Co-authored-by: Norbert Elter <[email protected]>

Update src/changelog-check.ts

0020d1a

Co-authored-by: Norbert Elter <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changelog checker #54

changelog checker #54

jake-perkins commented Mar 13, 2025 •

edited

Loading

socket-security bot commented Mar 13, 2025 •

edited

Loading

socket-security bot commented Mar 13, 2025 •

edited

Loading

makemesteaks Mar 14, 2025

Gudahtt Mar 20, 2025

jake-perkins Mar 24, 2025 •

edited

Loading

Gudahtt Mar 20, 2025

Gudahtt Mar 20, 2025

Gudahtt Mar 20, 2025

Gudahtt Mar 20, 2025

itsyoboieltr Mar 24, 2025

jake-perkins Mar 24, 2025

jake-perkins Mar 24, 2025

itsyoboieltr Mar 24, 2025

jake-perkins Mar 24, 2025 •

edited

Loading

itsyoboieltr Mar 24, 2025

itsyoboieltr Mar 24, 2025

jake-perkins Mar 24, 2025

itsyoboieltr Mar 24, 2025

jake-perkins Mar 24, 2025 •

edited

Loading

itsyoboieltr Mar 24, 2025

itsyoboieltr Mar 24, 2025

itsyoboieltr Mar 24, 2025

itsyoboieltr Mar 24, 2025

cryptodev-2s commented Apr 1, 2025

changelog checker #54

Are you sure you want to change the base?

changelog checker #54

Conversation

jake-perkins commented Mar 13, 2025 • edited Loading

socket-security bot commented Mar 13, 2025 • edited Loading

socket-security bot commented Mar 13, 2025 • edited Loading

Next steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jake-perkins Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jake-perkins Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jake-perkins Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptodev-2s commented Apr 1, 2025

jake-perkins commented Mar 13, 2025 •

edited

Loading

socket-security bot commented Mar 13, 2025 •

edited

Loading

socket-security bot commented Mar 13, 2025 •

edited

Loading

jake-perkins Mar 24, 2025 •

edited

Loading

jake-perkins Mar 24, 2025 •

edited

Loading

jake-perkins Mar 24, 2025 •

edited

Loading