Display releaseNotes during ext:update, and streamline ext:update code #3672

joehan · 2021-08-13T00:56:05Z

Description

Display releaseNotes during ext:update if they exist.

I also removed most of the code supporting old official extensions. @huangjeff5 and I checked the backend, and all instances have been migrated to the new registry, so these code paths should be totally unused and safe to remove at this point.

Scenarios Tested

Update containing a major version:

Update without a major version:

Updating when releaseNotes is not returned by the backend:

…e in general

huangjeff5 · 2021-08-18T01:03:47Z

src/commands/ext-update.ts

-        newSourceOrigin === SourceOrigin.OFFICIAL_EXTENSION_VERSION;
-      await displayChanges(existingSpec, newSpec, isOfficial);
+
+      await displayChanges(existingSpec, newSpec, false);


I remember this logic was supposed to display before/after sources only if the extension wasn't official, I believe. Worth checking with Sonakshi to see if it's okay to deprecate this logic.

If it is okay to deprecate that logic, then I think we can remove the last parameter altogether from displayChanges()

Cleaned this up as well, good eye!

elvisun · 2021-08-18T13:09:11Z

UX Question: If I see 0.2.0 and don't want to upgrade to that, but want to upgrade to 0.1.26 instead, is that a supported flow?

elvisun · 2021-08-18T13:11:15Z

src/extensions/changelog.ts

+ * @param toVersion the version you are upodating to
+ * @returns a Record of version number to releaseNotes for that version
+ */
+export async function getReleaseNotesForUpdate(


prefer named params to reduce confusion at call site
go/tsstyle#comments-when-calling-a-function

elvisun · 2021-08-18T13:16:22Z

src/extensions/changelog.ts

+  const releaseNotes: Record<string, string> = {};
+  const filter = `id<="${toVersion}" AND id>"${fromVersion}"`;
+  const extensionVersions = await listExtensionVersions(extensionRef, filter);
+  for (const extensionVersion of extensionVersions) {


This assumes listExtensionVersions returns in a sorted manner, which is not part of the contract from our backend

So it'd be safer to sort it here so users are guaranteed to see all versions in incremental order

elvisun · 2021-08-18T13:18:40Z

src/extensions/changelog.ts

+ */
+export function displayReleaseNotes(releaseNotes: Record<string, string>, fromVersion: string) {
+  const versions = [fromVersion].concat(Object.keys(releaseNotes));
+  const breaks = breakingChangesInUpdate(versions);


this is kinda confusing with break, maybe breakingVersions?

elvisun · 2021-08-18T13:29:15Z

src/extensions/changelog.ts

+      semvers[i - 1].major < semvers[i].major ||
+      (semvers[i - 1].major == 0 &&
+        semvers[i].major == 0 &&
+        semvers[i - 1].minor < semvers[i].minor)


This is pretty scary to read, can you extract the variables in if to const with some meaningful names? Something like:
hasMajorBump || hasMinorBumpInPreview

elvisun · 2021-08-18T13:33:04Z

src/extensions/updateHelper.ts

  } catch (err) {
-    sourceOrigin = SourceOrigin.PUBLISHED_EXTENSION;
+    // Ignore errors if we can't fetch the registry


is this registry.json?

silently failing worries me... do we have some kind of error reporting for the CLI?

Adding a debug log here, but I think this is necessary to ignore. We want to be able to take down the registry at some point without breaking this. In the case of an outage before then, this would just accidentally allow updates below minVersion, which is a small corner case that we're already in the process of migrating to the backend.

sg with noop just wanted to get some logs here in case we need to figure out how often this happens

elvisun · 2021-08-18T13:36:16Z

src/test/extensions/changelog.spec.ts

+      expect(got).to.deep.equal(want);
+    });
+
+    it("should exclude versions that don't have releaseNotes", async () => {


Or should we say no release note? providers forgetting to write release note might be pretty common

Gonna leave this as is since:

We're gonna check for a CHANGELOG entry and enforce that it should be there on ext:publish, so this should not happen frequently

In the case where an extension doesn't have release notes populated on the backend, it may still have release notes available elsewhere, so it could be misleading.

sgtm, thanks!

joehan · 2021-08-18T21:40:40Z

@elvisun There is! firebase ext:update 0.1.26 -P my-proj

elvisun · 2021-08-19T01:00:08Z

src/test/extensions/changelog.spec.ts

+        testExtensionVersion("0.1.1", "foo"),
+        testExtensionVersion("0.1.2", "bar"),


flip this to test the sort

Oooh, good idea!

Ahhh, on second though it won't actually cover that - the sorting happens in a different method that the one under test.

* save my place * implement parseChangelog * check for changelog on ext:dev:publish

firebase#3672) * Display releaseNotes during ext:update, and streamline ext:update code in general * Add typing to silence linter issues * adding jsdoc comments * bolding after UX feedback * pr fixes * Check for and display CHANGELOG.md during ext:dev:publish (firebase#3693) * save my place * implement parseChangelog * check for changelog on ext:dev:publish * adding changelog

joehan added 2 commits August 12, 2021 17:43

Display releaseNotes during ext:update, and streamline ext:update cod…

3d435a0

…e in general

fixing merge conflict

9a0a820

google-cla bot added the cla: yes Manual indication that this has passed CLA. label Aug 13, 2021

joehan requested a review from huangjeff5 August 13, 2021 00:56

joehan added 2 commits August 13, 2021 09:27

Add typing to silence linter issues

f6ba790

adding jsdoc comments

3281112

joehan requested a review from elvisun August 16, 2021 17:27

bolding after UX feedback

2fea12c

huangjeff5 reviewed Aug 18, 2021

View reviewed changes

elvisun reviewed Aug 18, 2021

View reviewed changes

joehan added 2 commits August 18, 2021 14:45

pr fixes

1262590

Merge branch 'master' into jh-changelogs

5492255

joehan requested review from elvisun and huangjeff5 August 18, 2021 21:47

huangjeff5 approved these changes Aug 18, 2021

View reviewed changes

elvisun reviewed Aug 19, 2021

View reviewed changes

elvisun approved these changes Aug 19, 2021

View reviewed changes

joehan added 3 commits August 31, 2021 16:11

Check for and display CHANGELOG.md during ext:dev:publish (#3693)

e28c82a

* save my place * implement parseChangelog * check for changelog on ext:dev:publish

Merge remote-tracking branch 'public/master' into jh-changelogs

ad7b31f

adding changelog

94c51df

joehan merged commit dd4dc12 into master Aug 31, 2021

joehan deleted the jh-changelogs branch August 31, 2021 23:30

		testExtensionVersion("0.1.1", "foo"),
		testExtensionVersion("0.1.2", "bar"),

Display releaseNotes during ext:update, and streamline ext:update code #3672

Display releaseNotes during ext:update, and streamline ext:update code #3672

Uh oh!

Conversation

joehan commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Scenarios Tested

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elvisun commented Aug 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elvisun Aug 18, 2021 • edited by joehan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joehan commented Aug 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joehan commented Aug 13, 2021 •

edited

Loading

elvisun Aug 18, 2021 •

edited by joehan

Loading