feat: Command and "Runner" Pattern for release-please #581

jbottigliero · 2020-10-08T15:37:45Z

Moves Yargs command parser to an isolated file (for reuse and testing).
Updates command handlers to use a shared "runner" module.
- This runner is meant to resemble the current release-please-action
Adds changelog-types option
Adds the ability to configure the CLI tool using a JSON file (via --config flag)

`src/runner.ts`

This module is being used to introduce a new entrypoint to this package that can be used by the internal command and consumers to centralize execution (and configuration mapping) for release-please in a CI context. As an example, the release-please-action could be replaced with something like the following*:

release-please-action/index.js

const core = require('@actions/core')
const releasePlease = require('release-please/build/src/runner')

const command = core.getInput('command') ? core.getInput('command') : undefined

const options = {
  label: 'autorelease: pending',
  bumpMinorPreMajor: Boolean(core.getInput('bump-minor-pre-major')),
  monorepoTags: Boolean(core.getInput('monorepo-tags')),
  packageName: core.getInput('package-name'),
  path: core.getInput('path') ? core.getInput('path') : undefined,
  releaseType: core.getInput('release-type'),
  token: core.getInput('token'),
  fork: core.getInput('fork') ? true : undefined,
  changelogTypes: core.getInput('changelog-types'),
  repoUrl: process.env.GITHUB_REPOSITORY
}

releasePlease(options, command, (release) => {
  const { upload_url, tag_name } = release
  core.setOutput('release_created', true)
  core.setOutput('upload_url', upload_url)
  core.setOutput('tag_name', tag_name)
}).catch((e) => {
  core.setFailed(`release-please failed: ${err.message}`)
});

The change is intended to allow for consumers to only need to focus on process input mapping and error/success handling. As another example, using the runner the GitHub Action behaviors can now be ported for usage via Google Cloud Build using a simple Docker image*.

* This code hasn't been tested.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

see #511 🦕

codecov · 2020-10-08T16:03:42Z

Codecov Report

Merging #581 (9d40c3a) into master (fe784e9) will increase coverage by 0.72%.
The diff coverage is 98.82%.

@@            Coverage Diff             @@
##           master     #581      +/-   ##
==========================================
+ Coverage   84.13%   84.86%   +0.72%     
==========================================
  Files          45       47       +2     
  Lines        5277     5530     +253     
  Branches      463      484      +21     
==========================================
+ Hits         4440     4693     +253     
+ Misses        836      832       -4     
- Partials        1        5       +4

Impacted Files	Coverage Δ
src/runner.ts	`93.61% <93.61%> (ø)`
src/bin/command.ts	`100.00% <100.00%> (ø)`
src/release-pr.ts	`89.38% <100.00%> (+0.19%)`	⬆️
src/github-release.ts	`92.15% <0.00%> (-1.97%)`	⬇️
src/releasers/index.ts	`100.00% <0.00%> (+12.96%)`	⬆️

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 fe784e9...9d40c3a. Read the comment docs.

jbottigliero · 2020-10-08T16:22:22Z

@bcoe, I wanted to open this (draft) PR to continue the discussion from #511 for feedback before I take this any further.

It's a bit larger than expected since I wanted to make sure tests could be added around the CLI. While the abstraction of the command handlers to the runner probably isn't necessary, it came from my own experience trying to replicate the functionality of the GitHub Action in other CI contexts (in addition to moving toward normalizing how custom CHANGELOG sections can be utilized).

I've also made the decision to go with a configuration file instead of pkgConf since it seems less likely for a package.json file to exist in the context where the release-please command winds up being run.

Let me know if you think it's best to break this PR up in some way or if you want to go a different direction entirely!

(Tentative) Required Work for this PR:

Add tests covering use of github-release and empty command using the runner.
Update the README to reference the ability to use --config (...maybe?)
Cover programmatic usage/configuration in the README

bcoe · 2020-10-08T18:44:38Z

@jbottigliero thanks for the patch 👍 will make an effort to give review ASAP.

bcoe · 2020-10-21T03:11:59Z

@jbottigliero I'm on a short vacation, so haven't carved out the time to look at this, hope to do so soon.

bcoe

A few nits mainly, this is looking really good to me 😄

I would be curious to see some pseudo code of what the action will end up looking like once we land your refactor.

src/bin/release-please.ts

bcoe · 2020-10-23T00:37:20Z

src/release-pr.ts

 export interface ReleasePROptions extends BuildOptions {
  releaseType: string;
-  changelogSections?: [];
+  changelogSections?: Array<ChangelogSection>;


I appreciate this refactor.

bcoe · 2020-10-23T00:53:04Z

test/cli.ts

+      });
+    });
+
+    it('can be configured using a file', () => {


I really appreciate the tests being added for the CLI.

jbottigliero · 2020-10-24T16:36:03Z

@bcoe – thanks for taking the time to consider this! I'll work on getting things cleaned up and knock the last few todos.

I would be curious to see some pseudo code of what the action will end up looking like once we land your refactor.

Since the new src/runner.ts export is based on the execution pattern found in the action, after this refactor, the release-please-action code can just be an abstraction that focuses on option parsing specific to that implementation (using @actions/core):

const core = require('@actions/core')
const releasePleaseRunner = require('release-please/build/src/runner')

const command = core.getInput('command') ? core.getInput('command') : undefined

const options = {
  label: 'autorelease: pending',
  bumpMinorPreMajor: Boolean(core.getInput('bump-minor-pre-major')),
  monorepoTags: Boolean(core.getInput('monorepo-tags')),
  packageName: core.getInput('package-name'),
  path: core.getInput('path') ? core.getInput('path') : undefined,
  releaseType: core.getInput('release-type'),
  token: core.getInput('token'),
  fork: core.getInput('fork') ? true : undefined,
  changelogTypes: core.getInput('changelog-types'),
  repoUrl: process.env.GITHUB_REPOSITORY
}

releasePleaseRunner(options, command, (release) => {
  const { upload_url, tag_name } = release
  core.setOutput('release_created', true)
  core.setOutput('upload_url', upload_url)
  core.setOutput('tag_name', tag_name)
}).catch((e) => {
  core.setFailed(`release-please failed: ${err.message}`)
});

While there might not be a huge benefit in this change on its own (except maybe allowing greater separation of concerns in the release-please-action repository)... I see it as creating the benefit of making a more obvious entry point for creating additional CI solutions.

This POC cloud-builder utilizing the new runner export is a good example of this: https://gist.github.com/jbottigliero/29e8de2dda4db7d3f07626dc99547026

I'm doing something similar to this in a few private repositories. But, without the runner code living in this repository (as an export), I have to essentially duplicate the release-please-action/main/index.js code without the GitHub specific configuration to get what I'd expect to be default behavior in a CI context.

What I'd like to see/explore after these changes is potentially looking into better ways of sharing an option parser in the runner context(s) and CLI, potentially eliminating configuration drift across implementations.

jbottigliero · 2020-11-11T02:50:52Z

@bcoe I ran into a bit of snag here with testing. I was hoping to add a snapshot test for the custom CHANGELOG sections, but I'm hitting an issue where the section order is not guaranteed. You can see this in the current CI state with ci / windows passing but ci / test (10) failing.

GitHub Actions Diff

Here, the commit order changed: ### Other\n\n* **deps:** update dependency [...] vs. ### Other\n\n* update common templates [...]

----------
Difference
----------
 [
   [
     "CHANGELOG.md",
     {
-      "content": "# Changelog\n\n### [1.0.1](https://www.github.com/googleapis/release-please/compare/v1.0.0...v1.0.1) (1983-10-10)\n\n\n### Other\n\n* **deps:** update dependency com.google.cloud:google-cloud-spanner to v1.50.0 ([1f9663c](https://www.github.com/googleapis/release-please/commit/1f9663cf08ab1cf3b68d95dee4dc99b7c4aac373))\n* **deps:** update dependency com.google.cloud:google-cloud-storage to v1.120.0 ([fcd1c89](https://www.github.com/googleapis/release-please/commit/fcd1c890dc1526f4d62ceedad561f498195c8939))\n* update common templates ([3006009](https://www.github.com/googleapis/release-please/commit/3006009a2b1b2cb4bd5108c0f469c410759f3a6a))\n",
+      "content": "# Changelog\n\n### [1.0.1](https://www.github.com/googleapis/release-please/compare/v1.0.0...v1.0.1) (1983-10-10)\n\n\n### Other\n\n* update common templates ([3006009](https://www.github.com/googleapis/release-please/commit/3006009a2b1b2cb4bd5108c0f469c410759f3a6a))\n* **deps:** update dependency com.google.cloud:google-cloud-spanner to v1.50.0 ([1f9663c](https://www.github.com/googleapis/release-please/commit/1f9663cf08ab1cf3b68d95dee4dc99b7c4aac373))\n* **deps:** update dependency com.google.cloud:google-cloud-storage to v1.120.0 ([fcd1c89](https://www.github.com/googleapis/release-please/commit/fcd1c890dc1526f4d62ceedad561f498195c8939))\n",
       "mode": "100644"
     }
   ],

source

OS X Node 10 vs OS X Node 12

Here, the section order changes.

-------------------
Saved snapshot text
-------------------
# Changelog

### [1.0.1](https://www.github.com/googleapis/release-please/compare/v1.0.0...v1.0.1) (1983-10-10)


### Other

* **deps:** update dependency com.google.cloud:google-cloud-spanner to v1.50.0 ([1f9663c](https://www.github.com/googleapis/release-please/commit/1f9663cf08ab1cf3b68d95dee4dc99b7c4aac373))
* **deps:** update dependency com.google.cloud:google-cloud-storage to v1.120.0 ([fcd1c89](https://www.github.com/googleapis/release-please/commit/fcd1c890dc1526f4d62ceedad561f498195c8939))


### Chores

* update common templates ([3006009](https://www.github.com/googleapis/release-please/commit/3006009a2b1b2cb4bd5108c0f469c410759f3a6a))
------------
Current text
------------
# Changelog

### [1.0.1](https://www.github.com/googleapis/release-please/compare/v1.0.0...v1.0.1) (1983-10-10)


### Chores

* update common templates ([3006009](https://www.github.com/googleapis/release-please/commit/3006009a2b1b2cb4bd5108c0f469c410759f3a6a))


### Other

* **deps:** update dependency com.google.cloud:google-cloud-spanner to v1.50.0 ([1f9663c](https://www.github.com/googleapis/release-please/commit/1f9663cf08ab1cf3b68d95dee4dc99b7c4aac373))
* **deps:** update dependency com.google.cloud:google-cloud-storage to v1.120.0 ([fcd1c89](https://www.github.com/googleapis/release-please/commit/fcd1c890dc1526f4d62ceedad561f498195c8939))

I recall seeing issues related to this behavior in the conventional-* projects, so I'm not totally sure on the best way to address it for the sake of this PR.

I considered opting into the various writerOpts sort options in src/conventional-commits.ts, but figured I'd touch base first – any thoughts or insights would be appreciated!

bcoe · 2020-11-16T14:57:40Z

@jbottigliero I ran into this bug the other week too, and am not quite sure what's happening. I think it's potentially related to Node.js version, and seems to relate to the order that items are emitted from the stream.

There's honestly no real reason for us to be using a streaming API for this sort of thing, if we could figure out a fix upstream that guarantees ordering, that seems like it would be ideal.

bcoe · 2020-12-20T18:51:01Z

@jbottigliero see https://github.com/conventional-commits/parser, it will be a little while but I'd like to move towards this reference implementation of a conventional commits parser, which will no longer be streaming and should address these ordering issues.

- Moves Yargs command parser to an isolated file (for reuse and testing). - Updates command handlers to use a shared "runner" module in order to centralize execuation and configuration parsing. - Adds `changelog-types` option - Adds ability to configure the CLI tool using a JSON file (via '--config' flag) see googleapis#511

…tional command.ts coverage)

bcoe

A couple more nits.

I'm excited that this will make the CLI more testable, and will allow us to cleanup the action.

I would like to be careful when we land the change, to make sure that we coordinate updating the action shortly after -- do you expect this will be a breaking change?

bcoe · 2021-01-06T22:44:24Z

src/bin/command.ts

@@ -0,0 +1,200 @@
+#!/usr/bin/env node


moving the hasang seems like an accident, as bin/release-please.ts is still the entrypoint?

Good catch - yeah bin/release-please.ts is still the entry point and this is just the yargs configuration. I'll update this.

bcoe · 2021-01-06T22:45:39Z

src/bin/command.ts

+// Copyright 2021 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.


In general I don't love the command.ts file, why don't we use individual command files instead for release-pr and github-release, and then test these in isolation?

see: https://github.com/yargs/yargs/blob/master/docs/advanced.md#example-command-hierarchy-using-commanddir

I find the commands/options configuration here confusing - @bcoe more a question for you:

many release-pr sub-options have the same name as github-release sub-options:
release-type, package-name, repo-url, path, label

release-type appears in both commands AND here

some options outside of a command are only relevant to a particular command:
bump-minor-pre-major and release-as belong only to release-pr

I haven't fully grokked yargs but it seems like we could clean this up a bit by deduplicating release-type (already done?), package-name, repo-url, path, and label (though maybe this last one is different for each command?) down into this common section and moving bump-minor-pre-major and release-as up into release-pr?

I like the addition of .config() but it also kind of further confuses me wrt to which options go with which command. Is any option from either command allowed in the config file json? if so, what happens the config has a changelog-types key (only defined on release-pr) and the github-release command is invoked?

I haven't fully grokked yargs but it seems like we could clean this up a bit by deduplicating release-type (already done?), package-name, repo-url, path, and label (though maybe this last one is different for each command?) down into this common section and moving bump-minor-pre-major and release-as up into release-pr?

This seems like it would be a smart cleanup, options can be defined at the top level, or at the command level in a builder function.

jbottigliero · 2021-01-08T16:07:07Z

I would like to be careful when we land the change, to make sure that we coordinate updating the action shortly after -- do you expect this will be a breaking change?

I don't expect the actual changeset here to be a breaking change, but the updated configuration handling might warrant one to be safe. I need to review the current state of the action, but the intent is that once this change is landed all configuration flags/options will be supported in both places (logic driven by the runner.ts) – I can see how this could lead to unexpected output based on what flags are being passed in the wild.

I'll wrap up your latest round of feedback here and then can open a PR against the action to give a clearer picture of what the changeset will look like across projects.

joeldodge79

I'd really like to build off these changes for the "aggregate PR" feature. @jbottigliero do you think you'll be able to work on this PR?

joeldodge79 · 2021-01-28T04:28:04Z

src/runner.ts

+  if (!command || command === 'release-pr') {
+    const opts = options as ReleasePROptions;
+    if (options.changelogTypes) {
+      opts.changelogSections = JSON.parse(options.changelogTypes);


@jbottigliero - TLDR - can we offer --config users a shortcut here so they can just define changelog-types as an array of nested objects (i.e. Array<ChangelogSection>) rather than a JSON encoded string? maybe

Suggested change

opts.changelogSections = JSON.parse(options.changelogTypes);

opts.changelogSections =

typeof options.changelogTypes === "string"

? JSON.parse(options.changelogTypes)

: options.changelogTypes as Array<ChangelogSection>;

I'm designing a feature to include multiple monorepo packages in a single release PR. I need to add some config that looks like this:

"packages": [ { "path": "path/to/pkg1", "release-type": "python", "package-name": "pkg1-py" }, { "path": "path/to/pkg2", "release-type": "node" // no package-name, gleaned from package.json } ]

and I think it would be a bummer for --config users to have to write/read that has a json string. cli-flag folks will just have to deal.

joeldodge79 · 2021-01-28T04:44:40Z

src/bin/command.ts

+  .option('release-type', {
+    describe: 'what type of repo is a release being created for?',
+    choices: getReleaserNames(),
+    default: 'node',
+  })


one thing I've found confusing about our options - @bcoe more a question for you:

many release-pr sub-options have the same name as github-release sub-options:
release-type, package-name, repo-url, path, label

release-type appears in both commands AND here

some options outside of a command are only relevant to a particular command:
bump-minor-pre-major and release-as belong only to release-pr

I haven't fully grokked yargs but it seems like we could clean this up a bit by deduplicating release-type (already done?), package-name, repo-url, path, and label (though maybe this last one is different for each command?) down into this common section and moving bump-minor-pre-major and release-as up into release-pr?

joeldodge79 · 2021-01-28T05:01:16Z

src/bin/command.ts

+// Copyright 2021 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.


I find the commands/options configuration here confusing - @bcoe more a question for you:

many release-pr sub-options have the same name as github-release sub-options:
release-type, package-name, repo-url, path, label

release-type appears in both commands AND here

some options outside of a command are only relevant to a particular command:
bump-minor-pre-major and release-as belong only to release-pr

I haven't fully grokked yargs but it seems like we could clean this up a bit by deduplicating release-type (already done?), package-name, repo-url, path, and label (though maybe this last one is different for each command?) down into this common section and moving bump-minor-pre-major and release-as up into release-pr?

I like the addition of .config() but it also kind of further confuses me wrt to which options go with which command. Is any option from either command allowed in the config file json? if so, what happens the config has a changelog-types key (only defined on release-pr) and the github-release command is invoked?

joeldodge79 · 2021-01-28T05:32:26Z

src/bin/command.ts

+}
+
+export default yargs
+  .config('config')


I think 'config' is the default?

Suggested change

.config('config')

.config()

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 8, 2020

jbottigliero force-pushed the feat-command branch 2 times, most recently from 961e54d to c1d715f Compare October 8, 2020 15:52

bcoe added the release-please:force-run To run release-please label Oct 8, 2020

release-please bot removed the release-please:force-run To run release-please label Oct 8, 2020

bcoe suggested changes Oct 23, 2020

View reviewed changes

jbottigliero force-pushed the feat-command branch from eef2031 to 19a6e65 Compare November 2, 2020 00:14

jbottigliero force-pushed the feat-command branch 2 times, most recently from 9f11dbb to 7ce3f30 Compare November 11, 2020 01:02

jbottigliero force-pushed the feat-command branch from 6f7efed to 1d9368a Compare December 12, 2020 02:33

bcoe mentioned this pull request Dec 25, 2020

feat: add helper for parsing array of commits conventional-changelog/conventional-changelog#711

Merged

jbottigliero force-pushed the feat-command branch from 1d9368a to faaecd2 Compare December 30, 2020 01:39

This was referenced Dec 30, 2020

fix: drop compare-func making sort consistent across node versions conventional-changelog/conventional-changelog#729

Merged

refactor: migrate to @conventional-commits/parser and update conventional-changelog #674

Merged

Joe Bottigliero added 2 commits January 5, 2021 19:49

chore: new year, new header

8500cda

jbottigliero force-pushed the feat-command branch from a627e77 to 09fd9cc Compare January 6, 2021 01:50

test: adds simple tests for github-release CLI and --debug flag (addi…

9d40c3a

…tional command.ts coverage)

jbottigliero force-pushed the feat-command branch from 09fd9cc to 9d40c3a Compare January 6, 2021 01:55

bcoe suggested changes Jan 6, 2021

View reviewed changes

bcoe changed the title ~~feat: Command and "Runner" Pattern for release-please~~ [break commit] feat: Command and "Runner" Pattern for release-please Jan 8, 2021

bcoe changed the title ~~[break commit] feat: Command and "Runner" Pattern for release-please~~ feat: Command and "Runner" Pattern for release-please Jan 8, 2021

joeldodge79 reviewed Jan 28, 2021

View reviewed changes

bcoe closed this Jan 29, 2021

bcoe mentioned this pull request Jan 29, 2021

feat(cli)!: refactor factory/CLI to be more testable #725

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Command and "Runner" Pattern for release-please #581

feat: Command and "Runner" Pattern for release-please #581

jbottigliero commented Oct 8, 2020 •

edited

Loading

codecov bot commented Oct 8, 2020 •

edited

Loading

jbottigliero commented Oct 8, 2020

bcoe commented Oct 8, 2020

bcoe commented Oct 21, 2020 •

edited

Loading

bcoe left a comment

bcoe Oct 23, 2020

bcoe Oct 23, 2020

jbottigliero commented Oct 24, 2020

jbottigliero commented Nov 11, 2020

bcoe commented Nov 16, 2020

bcoe commented Dec 20, 2020

bcoe left a comment

bcoe Jan 6, 2021

jbottigliero Jan 8, 2021

bcoe Jan 6, 2021

joeldodge79 Jan 28, 2021

bcoe Jan 29, 2021

jbottigliero commented Jan 8, 2021

joeldodge79 left a comment

joeldodge79 Jan 28, 2021

joeldodge79 Jan 28, 2021

joeldodge79 Jan 28, 2021

joeldodge79 Jan 28, 2021

-      opts.changelogSections = JSON.parse(options.changelogTypes);
+      opts.changelogSections =
+        typeof options.changelogTypes === "string"
+          ? JSON.parse(options.changelogTypes)
+          : options.changelogTypes as Array<ChangelogSection>;

feat: Command and "Runner" Pattern for release-please #581

feat: Command and "Runner" Pattern for release-please #581

Conversation

jbottigliero commented Oct 8, 2020 • edited Loading

src/runner.ts

codecov bot commented Oct 8, 2020 • edited Loading

Codecov Report

jbottigliero commented Oct 8, 2020

bcoe commented Oct 8, 2020

bcoe commented Oct 21, 2020 • edited Loading

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

jbottigliero commented Oct 24, 2020

jbottigliero commented Nov 11, 2020

bcoe commented Nov 16, 2020

bcoe commented Dec 20, 2020

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

jbottigliero commented Jan 8, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbottigliero commented Oct 8, 2020 •

edited

Loading

`src/runner.ts`

codecov bot commented Oct 8, 2020 •

edited

Loading

bcoe commented Oct 21, 2020 •

edited

Loading