refactor!: normalize configuration across classes #763

rollyjoel · 2021-02-11T05:22:32Z

This is a first take at normalizing the configuration options used to
construct GitHub, ReleasePR, and GitHubRelease instances as well as
higher level configuration used by the factory entrypoints to build
them. The principal motivation is to bring the code more inline with
IoC/DI patterns (https://stackoverflow.com/a/3140/13802304)

refactor!: interfaces for constructor options moved to src/index.ts
and signficantly reorganized
refactor!: GitHub constructor: GitHubConstructorOptions
refactor!: GitHubRelease constructor: GitHubReleaseConstructorOptions
refactor!: ReleasePR constructor: ReleasePRConstructorOptions
refactor!: ReleasePR package name handling refactored -
lookupPackageName removed in favor of getPackageName
refactor!: ConventionalCommits constructor takes owner and repo instead
of just repoUrl
refactor!: remove GitHubRelease.labels in favor of ReleasePR.labels

codecov · 2021-02-11T05:23:40Z

Codecov Report

Merging #763 (46c7368) into master (c9fbf78) will increase coverage by 0.30%.
The diff coverage is 87.82%.

@@            Coverage Diff             @@
##           master     #763      +/-   ##
==========================================
+ Coverage   88.92%   89.22%   +0.30%     
==========================================
  Files          59       60       +1     
  Lines        7690     7753      +63     
  Branches      675      728      +53     
==========================================
+ Hits         6838     6918      +80     
+ Misses        851      834      -17     
  Partials        1        1

Impacted Files	Coverage Δ
src/releasers/index.ts	`100.00% <ø> (ø)`
src/releasers/ruby-yoshi.ts	`26.77% <0.00%> (-0.76%)`	⬇️
src/releasers/helm.ts	`29.00% <18.75%> (-0.16%)`	⬇️
src/releasers/ruby.ts	`34.74% <45.45%> (-0.26%)`	⬇️
src/releasers/java-yoshi.ts	`87.46% <82.35%> (+0.15%)`	⬆️
src/bin/release-please.ts	`84.86% <86.30%> (+2.96%)`	⬆️
src/releasers/php-yoshi.ts	`93.10% <91.66%> (+0.10%)`	⬆️
src/release-pr.ts	`91.77% <97.26%> (+0.34%)`	⬆️
src/constants.ts	`100.00% <100.00%> (ø)`
src/conventional-commits.ts	`99.19% <100.00%> (-0.02%)`	⬇️
... and 14 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 c9fbf78...9a79a64. Read the comment docs.

rollyjoel · 2021-02-11T15:10:54Z

found some holes in src/factory.ts while manually testing the cli against some example repos I have. Now that the factory has much more responsibility for correctly constructing different pieces it should have much better testing. working on that.

bcoe

Just a couple initial questions, will give a more thorough review later today.

In general I really like the approach, it seems like you've gone the route that: rather than pulling more functionality into ReleasePR, so that it's available when creating a release, you now have an instance of a ReleasePR in a GithubRelease?

This seems really smart to me.

bcoe · 2021-02-11T17:21:49Z

src/bin/release-please.ts

  })
  .option('default-branch', {
-    describe: '',
+    describe: 'The branch to open release PRs against and tag releases on',


Good catch.

bcoe · 2021-02-11T17:23:41Z

src/factory.ts

+    versionFile,
+  });
+
+  return new GitHubRelease({github, releasePR, ...remaining});


I like this approach, this ends up meaning that the GitHub release can use any releaser specific methods on releasePR?

GitHubRelease already relies on a ReleasePR instance for to perform it's one job (run) - this change is simple removing the knowledge of how to construct it (and thus also removing any duplicate constructor options) and centralizing it in case other future classes (aka aggregate) need one too. Same pattern as pulling out the building of a GitHub instance because that was already being duplicated (by ReleasePR)

bcoe · 2021-02-11T17:25:53Z

src/index.ts

+  repo: string;
+}
+
+// Used by GitHubRelease and ReleasePR Ctor


nit: might spell this out constructor, had to Google the shorthand.

will do. I had to look it up too not that long ago (finally got tired of seeing it everywhere on SO and not really knowing what it mean). I come from python where init is both short and accurate :-)

bcoe · 2021-02-11T17:30:43Z

src/releasers/go-yoshi.ts

    const cc = new ConventionalCommits({
      commits: commits,
-      githubRepoUrl: this.repoUrl,
+      owner: this.gh.owner,


Just standardizing on splitting own owner and repo in more places?

the idea is to consider repoUrl as denormalized shorthand user input for factory.ts and that it should be normalized only once into {owner, repo} and everything else inside the release-please codebase should rely on those directly rather than either carry around this copy of the short hand that everyone then has to convert. It would be a different story if different actors converted it differently but they all just use parse-github-repo-url to get the owner and repo.

bcoe · 2021-02-11T17:35:39Z

src/releasers/node.ts

+    }
+    return {
+      name: this.packageName,
+      refSafeName: () =>


If I'm understanding refSafeName properly, it codifies dropping the @pkg/ prefix from a branch name?

yeah, but I think I'll change it to getComponent() per a discussion with @chingor13

In this refactor I tried to steer away as much as possible from functionality refactoring or re-naming. The one exception is that darned quagmire of (packagePrefix vs packageName) x (set via configuration vs releaser knows how to get it): there was a nasty tangle of responsibility leaking that I needed to cleanup to normalize the configuration. Maybe PackageName.refSafeName() is better named PackageName.getComponent() - that's more inline with the domain vocabulary we've chosen and being safe to use as/in a git ref is just a requirement of a component rather than the name

bcoe

One question for you, was scratching my head trying to figure out where remaining is assigned (maybe I'm misunderstanding something magic about spread operators).

It also jumped out at me that perhaps label should only be a string during parsing.

bcoe · 2021-02-12T00:03:21Z

src/bin/release-please.ts

+    choices: getReleaserTypes(),
+  };
+  if (defaultType) {
+    relTypeOptions['default'] = defaultType;


nit: relTypeOptions.default.

ug, brain frazzle (when I get tired the python slips back in)

bcoe · 2021-02-12T00:07:58Z

src/factory.ts

+    changelogSections,
+    lastPackageVersion,
+    versionFile,
+    ...remaining


I seem to be missing where remaining is defined?

I can't hyperlink to the exact location but search for the string "Rest in Object Destructuring " in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment

in this case remaining is whatever is left of the options parameter that wasn't pulled out in the destructuring.

neat, haven't used that pattern.

bcoe · 2021-02-12T00:12:59Z

src/release-pr.ts

    this.bumpMinorPreMajor = options.bumpMinorPreMajor || false;
-    this.defaultBranch = options.defaultBranch;
    this.fork = !!options.fork;
    this.labels = options.label


Feels a bit weird that label starts as a string, that's split, and then we pass it around as an array of strings to the GitHub instance.

We could coerce labels into an array in the CLI, using .coerce and in the GitHub action, and perhaps make labels an array from the factory onwards?

I like it. Do you want me to do that in this PR?

I'm doing it in this PR - and it's leading to another GitHubRelease option culling: that class doesn't need this.labels now that it has a releasePR instance. I hadn't looked closely but it turns out ReleasePR.labels is supposed be the same as GitHubRelease.labels. Back before GitHubRelease had a releaser instance that duplication was necessary, but not anymore

bcoe · 2021-02-12T00:14:49Z

src/releasers/node.ts


 export class Node extends ReleasePR {
-  async getOpenPROptions(
+  private pkgJsonContents?: GitHubFileContents;


Is this just an optimization so that we only load once?

yeah - the original _run code loads the contents at the beginning only once and passes it to the PackageJson Updater as well as sets self.packageName - but now that I've tried to make getPackageName() available potentially prior to any "run" I thought I'd cache it here. But maybe potential caching bugs aren't worth the small cost of an extra API call or two?

bcoe · 2021-02-12T00:16:49Z

test/factory.ts

+    const repoUrls = [
+      `${owner}/${repo}`,
+      `${owner}/${repo}`,
+      `https://github.com/${owner}/${repo}.git`,


I appreciate these tests, and that we're so forgiving with the input.

oops, I have the same input listed twice. I was just trying to copy some examples from https://github.com/repo-utils/parse-github-repo-url#features

bcoe · 2021-02-12T17:51:51Z

src/factory.ts

+    octokitAPIs,
+  });
+
+  const labels = getLabels(label);


Nice, I like this approach.

This is a first take at normalizing the configuration options used to construct GitHub, ReleasePR, and GitHubRelease instances as well as higher level configuration used by the factory entrypoints to build them. The principal motivation is to bring the code more inline with IoC/DI patterns (https://stackoverflow.com/a/3140/13802304) refactor!: interfaces for constructor options moved to src/index.ts and signficantly reorganized refactor!: GitHub constructor: GitHubConstructorOptions refactor!: GitHubRelease constructor: GitHubReleaseConstructorOptions refactor!: ReleasePR constructor: ReleasePRConstructorOptions refactor!: ReleasePR package name handling refactored - lookupPackageName removed in favor of getPackageName refactor!: ConventionalCommits constructor takes owner and repo instead of just repoUrl refactor!: remove GitHubRelease.labels in favor of ReleasePR.labels

rollyjoel requested a review from a team as a code owner February 11, 2021 05:22

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

rollyjoel marked this pull request as draft February 11, 2021 15:08

rollyjoel changed the title ~~Factory overhaul~~ refactor!: IoC/DI to normalize configuration Feb 11, 2021

bcoe reviewed Feb 11, 2021

View reviewed changes

bcoe mentioned this pull request Feb 11, 2021

feat: add support for helm #748

Merged

4 tasks

rollyjoel force-pushed the factory-overhaul branch 2 times, most recently from 2cb9f20 to e19dc1c Compare February 11, 2021 21:20

bcoe approved these changes Feb 12, 2021

View reviewed changes

rollyjoel marked this pull request as ready for review February 12, 2021 00:52

bcoe reviewed Feb 12, 2021

View reviewed changes

src/factory.ts

octokitAPIs,

});

const labels = getLabels(label);

Copy link

bcoe Feb 12, 2021

Choose a reason for hiding this comment

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

Nice, I like this approach.

bcoe approved these changes Feb 12, 2021

View reviewed changes

rollyjoel force-pushed the factory-overhaul branch from 2220605 to 9a79a64 Compare February 12, 2021 19:02

rollyjoel changed the title ~~refactor!: IoC/DI to normalize configuration~~ refactor!: normalize configuration across classes Feb 12, 2021

bcoe merged commit 44a3fd2 into googleapis:master Feb 12, 2021

release-please bot mentioned this pull request Feb 12, 2021

chore: release 11.0.0 #761

Merged

rollyjoel deleted the factory-overhaul branch February 18, 2021 12:58

release-please bot mentioned this pull request Mar 6, 2022

chore(main): release 14.0.0 mdvorak/release-please#1

Closed

refactor!: normalize configuration across classes #763

refactor!: normalize configuration across classes #763

Uh oh!

Conversation

rollyjoel commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rollyjoel commented Feb 11, 2021

Uh oh!

bcoe left a comment

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!

Choose a reason for hiding this comment

Uh oh!

bcoe left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rollyjoel commented Feb 11, 2021 •

edited

Loading

codecov bot commented Feb 11, 2021 •

edited

Loading