Skip to content
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

Move auto-changelog to deps, pin to ~3.3.0 #122

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Dec 6, 2023

We started to use @metamask/auto-changelog at runtime in a recent commit. As it turns out, recent versions of this tool contain breaking changes that we didn't document, and so we need to rely on an older version. Adding this to dependencies ensures that we are doing so, even if the project where this tool is run against a project that also also uses @metamask/auto-changelog (but a different version).

We started to use `@metamask/auto-changelog` at runtime in a recent
commit. As it turns out, recent versions of this tool contain breaking
changes that we didn't document, and so we need to rely on an older
version. Adding this to `dependencies` ensures that we are doing so,
even if the project where this tool is run against a project that also
also uses `@metamask/auto-changelog` (but a different version).
@mcmire mcmire requested a review from a team as a code owner December 6, 2023 23:54
@mcmire
Copy link
Contributor Author

mcmire commented Dec 6, 2023

For more context, this PR in auto-changelog seems to have changed the behavior of updateChangelog so that it does nothing if isReleaseCandidate is false: MetaMask/auto-changelog#158. That conflicts with the expectations this tool makes, because we initially call updateChangelog with isReleaseCandidate: false just to get the Unreleased section of changelogs initially populated:

isReleaseCandidate: false,

Copy link

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@metamask/auto-changelog 3.4.3...3.3.0 None +0/-0 174 kB metamaskbot

Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see where I went wrong with updateChangelog. I could open a quick PR with this fix:

@@ -197,6 +197,12 @@ export async function updateChangelog({
   tagPrefixes = ['v'],
   formatter = undefined,
 }: UpdateChangelogOptions): Promise<string | undefined> {
+  if (isReleaseCandidate && !currentVersion) {
+    throw new Error(
+      `A version must be specified if 'isReleaseCandidate' is set.`,
+    );
+  }
+
   const changelog = parseChangelog({
     changelogContent,
     repoUrl,
@@ -210,6 +216,15 @@ export async function updateChangelog({
     tagPrefixes,
   });
 
+  if (
+    isReleaseCandidate &&
+    mostRecentTag === `${tagPrefixes[0]}${currentVersion || ''}`
+  ) {
+    throw new Error(
+      `Current version already has tag, which is unexpected for a release candidate.`,
+    );
+  }
+
   const commitRange =
     mostRecentTag === null ? 'HEAD' : `${mostRecentTag}..HEAD`;
   const commitsHashesSinceLastRelease = await getCommitHashesInRange(
@@ -233,19 +248,7 @@ export async function updateChangelog({
     return undefined;
   }
 
-  if (isReleaseCandidate) {
-    if (!currentVersion) {
-      throw new Error(
-        `A version must be specified if 'isReleaseCandidate' is set.`,
-      );
-    }
-
-    if (mostRecentTag === `${tagPrefixes[0]}${currentVersion ?? ''}`) {
-      throw new Error(
-        `Current version already has a tag ('${mostRecentTag}'), which is unexpected for a release candidate.`,
-      );
-    }
-
+  if (isReleaseCandidate && currentVersion) {
     // Ensure release header exists, if necessary
     if (
       !changelog
@@ -258,22 +261,22 @@ export async function updateChangelog({
     if (hasUnreleasedChanges) {
       changelog.migrateUnreleasedChangesToRelease(currentVersion);
     }
+  }

For now pinning to a non-breaking version seems like the right move.

@mcmire mcmire merged commit e37d05d into main Dec 7, 2023
16 checks passed
@mcmire mcmire deleted the move-auto-changelog-to-deps branch December 7, 2023 04:31
@mcmire
Copy link
Contributor Author

mcmire commented Dec 7, 2023

@MajorLift That would be great — whenever you have time. I'm really curious why this didn't break any of the tests. I suspect we have some missing test cases. In the meantime I've filed a ticket here: MetaMask/auto-changelog#180

@MajorLift
Copy link
Contributor

I'm really curious why this didn't break any of the tests.

Oh that's easy. I don't believe we have any tests for update-changelog! I'll make a PR for the fix asap and then look into adding some unit tests as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants