feat(manifest): node workspace package dependency updates #844

joeldodge79 · 2021-03-29T21:28:49Z

Introduces the concept of a "plugin" that can run after release-please
has generated changes but before the PR is created.

In order to support plugin releaser-post-processing (before submitting
the PR) we want to provide the whole contents of the changed file to the
plugin.

codecov · 2021-03-29T21:30:09Z

Codecov Report

Merging #844 (d679c1c) into master (6f24661) will increase coverage by 0.29%.
The diff coverage is 98.80%.

@@            Coverage Diff             @@
##           master     #844      +/-   ##
==========================================
+ Coverage   93.78%   94.07%   +0.29%     
==========================================
  Files          64       68       +4     
  Lines        9218     9809     +591     
  Branches      939     1014      +75     
==========================================
+ Hits         8645     9228     +583     
- Misses        570      578       +8     
  Partials        3        3

Impacted Files	Coverage Δ
src/manifest.ts	`98.94% <93.44%> (-1.06%)`	⬇️
src/github.ts	`90.13% <100.00%> (+0.01%)`	⬆️
src/index.ts	`100.00% <100.00%> (ø)`
src/plugins/index.ts	`100.00% <100.00%> (ø)`
src/plugins/node-workspace.ts	`100.00% <100.00%> (ø)`
src/plugins/plugin.ts	`100.00% <100.00%> (ø)`
src/updaters/package-json.ts	`100.00% <100.00%> (ø)`
src/util/package-json-stringify.ts	`100.00% <100.00%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f24661...d679c1c. Read the comment docs.

joeldodge79

@bcoe this is the behemoth PR that represents that last piece of functionality we need for the looker-open-source/sdk-codegen release requirements. not my usual style to drop a 3K LOC PR but, well, here it is (and 2/3rds of the LOC is test code).

I could split it up if you'd like but I think it's at least useful to see the whole picture. I left a bunch of inline comments as a guide

src/github.ts

src/index.ts

src/manifest.ts

joeldodge79 · 2021-03-29T22:02:30Z

src/plugins/node-workspace.ts

+    return changelogUpdater.updateContent(changelog);
+  }
+
+  private async setChangelogEntry(


all this changelog entry manipulation got a bit exasperating at the end but it seems to work pretty well now. There may be a better way of doing it but I was just trying to jam through to the finish line... really wanted to see changelog entries for lerna-based dependency updates.

See comment about us moving towards a better markdown generator that actually understands the tree.

src/plugins/plugin.ts

src/util/package-json-stringify.ts

test/helpers.ts

bcoe

Left some initial comments.

The big thing that jumps out at me, is that the assertions are getting pretty large when confirming actualChanges. For what is partially text content like this, I feel this is where snpshots can really shine.

Here are the snapshots I use for c8, our test coverage library. I feel like they read very well, and it's clear when they beak -- perhaps the problem with the snapshots in relese-please is just that we need a helper for structured objects, that helps output their contents in a format that's slightly more diffable?

bcoe · 2021-03-31T14:48:41Z

package.json

@@ -51,6 +51,7 @@
    "@types/yargs": "^16.0.0",
    "c8": "^7.0.0",
    "chai": "^4.2.0",
+    "chai-better-shallow-deep-equal": "^1.1.0",


When possible, I'd rather use the built in assert.strictDeepEqual, I like trying to dogfood Node.js with as few dependencies as possible.

bcoe · 2021-03-31T14:49:24Z

package.json

@@ -61,9 +62,14 @@
  "dependencies": {
    "@conventional-commits/parser": "^0.4.1",
    "@iarna/toml": "^2.2.5",
+    "@lerna/collect-updates": "^4.0.0",
+    "@lerna/package": "^3.16.0",


It's nice that lerna is decomposed like this.

bcoe · 2021-03-31T14:51:22Z

src/index.ts

+// 3. currently unsupported but possible future support:
+//   - `snapshot`
+//   - `label`
+export type ManifestPackage = Pick<


I like the use of Pick here, my colleague @SurferJeffAtGoogle was walking me through some of these advanced TypeScript features the other day.

src/index.ts

src/manifest.ts

src/plugins/index.ts

bcoe · 2021-03-31T15:00:30Z

src/plugins/node-workspace.ts

+    return changelogUpdater.updateContent(changelog);
+  }
+
+  private async setChangelogEntry(


See comment about us moving towards a better markdown generator that actually understands the tree.

bcoe · 2021-03-31T15:03:03Z

test/manifest.ts

@@ -2049,4 +2049,80 @@ describe('Manifest', () => {
      });
    }
  });
+
+  describe('plugins', () => {
+    it('runs the node-workspace plugin', async function () {


A few additional comments in this test would help me. Shouldn't we see @pkg2 being updated as well since it depends on @pkg1 which was updated? I only see updates for @pkg1 and python.

if you look at the snapshot it captured the @pkg2 updates. Perhaps confusing because I'm only checking the logging that Manifest is performing here, not any logging the plugin is doing. Given how the plugin is instantiated via the config it's hard to inject a logger (though I could probably stub it). Or I could just remove the Manifest logging for this test case to avoid confusion.

I'll get around to moving the logging out of this file into the snapshot too at some point.

bcoe · 2021-03-31T15:07:40Z

test/plugins/node-workspace.ts

+                    '\n\nAll notable changes to this project will be ' +
+                    'documented in this file.' +
+                    '\n\n### [2.2.3](https://www.github.com/fake/repo/compare' +
+                    '/pkgB-v2.2.2...pkgB-v2.2.3) (1983-10-10)' +


I think that these assertions are going to become a bit fragile, as even subtle changes to the markdown generation will lead to a bunch of fiddling in tests.

Having confirmed the behavior manually (i.e., now that you trust the approach), I would be tempted to move the assertion into a snapshot -- at which point, we should just see small stylistic differences in the snapshot as changes are made to content.

absolutely. converted the log tracking and the changes to snapshots

bcoe · 2021-03-31T15:08:35Z

typings/lerna__package-graph.d.ts

+
+/**
+ * skeleton only representing what we use.
+ */


Can we pull any of these typings in from DefinitelyTyped rather than floating them ourselves?

They don't currently exist there. What I've added here is pretty sparse coverage for the relevant packages. I was hoping to avoid taking on the load of publishing them there which I feel like would require a more comprehensive effort (also, never done it before...)

chingor13

Can you add an expected usage example for this feature?

Is the plugin a Javascript/Typescript function in the release repo? or in the release-please repo?

joeldodge79 · 2021-04-01T19:43:09Z

@chingor13

Can you add an expected usage example for this feature

I can add documentation on how to use this for sure. I wanted to get it out for some initial feedback first though in case it needs to change significantly before writing up a doc

Yes, the "plugin" exists in the release-please repo (currently just the one: src/plugins/node-workspace.ts

In the real world I've used this for our looker-open-source/sdk-codegen release PR. Here the node-workspace plugin correctly updated all our intra-monorepo-package-dependencies and added corresponding changelog entries.

@bcoe I think I've covered your initial around of feedback (mostly converting to snapshots).

bcoe · 2021-04-05T23:37:04Z

test/plugins/node-workspace.ts

+
+const sandbox = sinon.createSandbox();
+
+function stringifyActual(actual: ManifestPackageWithPRData[]) {


I like the approach of using this stringifyActual helper 👍

bcoe

This is looking good to me, pending @chingor13's comments.

joeldodge79 · 2021-04-07T00:16:32Z

@chingor13 I added a section to the manifest docs

bcoe

Small nit about README, I think this is looking good though.

bcoe · 2021-04-07T01:04:26Z

docs/manifest-releaser.md

+Unlike the individual releasers, which only have the context of the source files
+relevant to one package, a plugin receives all the current changes for all
+updated packages as well as the configuration containing every package. One
+place this is particularly useful is for monorepos that have intra-repo local


I think it would be worth breaking this paragraph in half, the first half can talk about how the plugins work:

what they're passed, and what they should output.

In this section I'd then have a code snippet that demonstrates the tiniest potential plugin that someone might right, e.g,. adding one additional update? it can be a toy example.

The second paragraph can then describe why plugins would be useful, exactly as you have right now.

let me know what you think of my num-packages plugin :-)

Introduces the concept of a "plugin" that can run after release-please has generated changes but before the PR is created. In order to support plugin releaser-post-processing (before submitting the PR) we want to provide the whole contents of the changed file to the plugin.

joeldodge79 · 2021-04-12T14:33:15Z

thanks for landing this @bcoe

Are there any docs on local/branch testing release-please changes in the action? I want to test the dynamic importing of plugins in the action and make changes if needed

joeldodge79 requested review from bcoe and a team March 29, 2021 21:28

joeldodge79 requested a review from a team as a code owner March 29, 2021 21:28

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 29, 2021

joeldodge79 commented Mar 29, 2021

View reviewed changes

joeldodge79 force-pushed the node-package-depenedency-updates-with-lerna branch 2 times, most recently from 8491fb1 to 62e3988 Compare March 30, 2021 22:12

bcoe suggested changes Mar 31, 2021

View reviewed changes

chingor13 reviewed Mar 31, 2021

View reviewed changes

joeldodge79 force-pushed the node-package-depenedency-updates-with-lerna branch from bde90b7 to 04d732d Compare April 5, 2021 21:20

joeldodge79 requested a review from bcoe April 5, 2021 21:20

bcoe reviewed Apr 5, 2021

View reviewed changes

joeldodge79 requested review from chingor13 and bcoe April 7, 2021 00:16

bcoe suggested changes Apr 7, 2021

View reviewed changes

joeldodge79 and others added 5 commits April 9, 2021 14:16

chore: cleanup for review

2cd541d

chore: feedback

19bf0e2

chore: add plugins section to manifest doc

7036439

chore: better docs for plugins

a99a535

joeldodge79 force-pushed the node-package-depenedency-updates-with-lerna branch from 5e8dc9c to a99a535 Compare April 9, 2021 15:25

joeldodge79 requested a review from bcoe April 9, 2021 15:26

bcoe approved these changes Apr 9, 2021

View reviewed changes

Merge branch 'master' into node-package-depenedency-updates-with-lerna

d679c1c

bcoe merged commit 9ebd422 into master Apr 9, 2021

bcoe deleted the node-package-depenedency-updates-with-lerna branch April 9, 2021 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(manifest): node workspace package dependency updates #844

feat(manifest): node workspace package dependency updates #844

joeldodge79 commented Mar 29, 2021

codecov bot commented Mar 29, 2021 •

edited

Loading

joeldodge79 left a comment

joeldodge79 Mar 29, 2021

bcoe Mar 31, 2021

bcoe left a comment

bcoe Mar 31, 2021

bcoe Mar 31, 2021

bcoe Mar 31, 2021

bcoe Mar 31, 2021

bcoe Mar 31, 2021

joeldodge79 Apr 1, 2021

bcoe Mar 31, 2021

joeldodge79 Apr 1, 2021

bcoe Mar 31, 2021

joeldodge79 Apr 1, 2021

chingor13 left a comment

joeldodge79 commented Apr 1, 2021

bcoe Apr 5, 2021

bcoe left a comment

joeldodge79 commented Apr 7, 2021

bcoe left a comment

bcoe Apr 7, 2021

joeldodge79 Apr 9, 2021

joeldodge79 commented Apr 12, 2021


		const sandbox = sinon.createSandbox();

		function stringifyActual(actual: ManifestPackageWithPRData[]) {

feat(manifest): node workspace package dependency updates #844

feat(manifest): node workspace package dependency updates #844

Conversation

joeldodge79 commented Mar 29, 2021

codecov bot commented Mar 29, 2021 • edited Loading

Codecov Report

joeldodge79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chingor13 left a comment

Choose a reason for hiding this comment

joeldodge79 commented Apr 1, 2021

Choose a reason for hiding this comment

bcoe left a comment

Choose a reason for hiding this comment

joeldodge79 commented Apr 7, 2021

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeldodge79 commented Apr 12, 2021

codecov bot commented Mar 29, 2021 •

edited

Loading