fix(monorepos): github-release not created #669

joeldodge79 · 2020-12-16T19:11:41Z

fixes a couple of issues:

if node releaser doesn't specify packageName then github-release
wasn't filtering merged PRs correctly.
when solving the above, we need to be consistent about the
relationship between packageName and the release branch name.

Fixes #668

codecov · 2020-12-16T19:12:52Z

Codecov Report

Merging #669 (69206ef) into master (5c0203d) will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #669      +/-   ##
==========================================
+ Coverage   85.15%   85.38%   +0.22%     
==========================================
  Files          48       49       +1     
  Lines        5894     5945      +51     
  Branches      549      531      -18     
==========================================
+ Hits         5019     5076      +57     
+ Misses        874      868       -6     
  Partials        1        1

Impacted Files	Coverage Δ
src/github-release.ts	`96.13% <100.00%> (+2.01%)`	⬆️
src/github.ts	`83.26% <100.00%> (+0.03%)`	⬆️
src/release-pr.ts	`90.08% <100.00%> (+0.89%)`	⬆️
src/releasers/node.ts	`94.44% <100.00%> (+0.24%)`	⬆️
src/util/package-branch-prefix.ts	`100.00% <100.00%> (ø)`

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 5c0203d...69206ef. Read the comment docs.

joeldodge79 · 2020-12-16T19:13:28Z

__snapshots__/github-release.js

@@ -111,13 +111,13 @@ exports['GitHubRelease extractLatestReleaseNotes php-yoshi extracts appropriate

 `

-exports['GitHubRelease createRelease creates and labels release on GitHub 1'] = {
-  'tag_name': 'v1.0.3',
+exports['GitHubRelease createRelease creates releases for submodule in monorepo 1'] = {


I'm having a hard time with the snapshot files. when I run npm test any change introduced re-writes all ' -> " for the entire file. Here I've tried to hand curate the appropriate changes but I'd really like to understand what my issue is with the auto-writing.

@joeldodge79 rather than manually updating the snapshot files, you can run:

npm run test:snap

and it will capture the current state of things.

Even running npm run test:snap I still get the ' -> " problem so the diff (see below) is totally useless :-( I'm having a hard time tracking down why my environment wants to make these changes (something with prettier somehow?) any thoughts?

diff --git a/__snapshots__/commit-split.js b/__snapshots__/commit-split.js index e8ec512..9e7e5ff 100644 --- a/__snapshots__/commit-split.js +++ b/__snapshots__/commit-split.js @@ -1,569 +1,569 @@ exports['CommitSplit partitions commits based on path from root directory by default 1'] = { - 'PubSub': [ + "PubSub": [ { - 'sha': '0a8477108a26aeb21d7af06e62be4ae5cb00ad42', - 'message': 'fix: Update PubSub timeouts. (#1967)', - 'files': [ - 'PubSub/src/V1/resources/subscriber_client_config.json', - 'PubSub/synth.metadata' + "sha": "0a8477108a26aeb21d7af06e62be4ae5cb00ad42", + "message": "fix: Update PubSub timeouts. (#1967)", + "files": [ + "PubSub/src/V1/resources/subscriber_client_config.json", + "PubSub/synth.metadata" ] } ],

joeldodge79 · 2020-12-16T19:15:39Z

src/util/package-branch.ts

+// limitations under the License.
+
+// map from a packageName to the prefix used in release branch creation.
+export function packageBranch(packageName: string) {


not super thrilled with this approach but it felt better than copy/pasting this around

better name?

better scoping (per releaser somehow? right now this just handles specific node/npm remapping)

@joeldodge79 I might call it parsePackageName and have it return a scope, and name, which are the names given to the two parts of the package name?

https://docs.npmjs.com/cli/v6/using-npm/scope

I'm going to suggest another route. updated PR coming soon

joeldodge79 · 2020-12-16T19:19:20Z

src/releasers/node.ts

@@ -125,11 +125,14 @@ export class Node extends ReleasePR {
  // A releaser can implement this method to automatically detect
  // the release name when creating a GitHub release, for instance by returning
  // name in package.json, or setup.py.
-  static async lookupPackageName(gh: GitHub): Promise<string | undefined> {
+  static async lookupPackageName(


it seems like some code above (L71) could be DRYed out using this method unless I'm missing something?

nm, I see you need contents further down so L71 should probably just stay as is.

bcoe · 2020-12-18T00:19:31Z

@joeldodge79 I think this fix might step on your toes a bit:

#670

We had a bug where we weren't successfully looking up package names in the action, I'll get it merged shortly.

bcoe

A few nits 👍 this is great work, it's awesome to have another person digging into the mono-repo logic, I think it's almost getting there 😆

bcoe · 2020-12-18T00:21:29Z

src/github-release.ts

@@ -78,6 +78,14 @@ export class GitHubRelease {
  }

  async createRelease(): Promise<ReleaseResponse | undefined> {
+    // Attempt to lookup the package name from a well known location, such
+    // as package.json, if none is provided:
+    if (!this.packageName && this.releaseType) {


I've just been specifying package-name in my monorepo implementations, this seems smarter.

I moved this up to ensure the same packageName was used for tag creation on L133 as for finding the release PR (L93). IIRC I was doing something stupid like trying to set package-name to something different from package.json (or maybe just not setting it at all?).

bcoe · 2020-12-18T00:24:09Z

src/github.ts

@@ -584,7 +585,7 @@ export class GitHub {
        // it's easiest/safest to just pull this out by string search.
        const version = match[2];
        if (!version) continue;
-        if (prefix && match[1] !== prefix) {
+        if (prefix && match[1] !== packageBranch(prefix)) {


could we drop the @foo/ prefix before passing it into this method? I think the reason I didn't bump into this error is that I've been setting packageName when using the monorepo functionality.

I don't fully understand your comment. In

before passing it into this method

does "this method" refer to the method we're in (findMergedReleasePR) or packageBranch? you meant the former... I think it's reasonable to move packageBranch(prefix) up to packageBranch(this.packageName) in the parent call site

bcoe · 2020-12-18T00:25:33Z

src/util/package-branch.ts

+// limitations under the License.
+
+// map from a packageName to the prefix used in release branch creation.
+export function packageBranch(packageName: string) {


@joeldodge79 I might call it parsePackageName and have it return a scope, and name, which are the names given to the two parts of the package name?

https://docs.npmjs.com/cli/v6/using-npm/scope

joeldodge79

I'm going to push an update for further discussion

joeldodge79 · 2021-01-07T16:19:46Z

src/github-release.ts

@@ -78,6 +78,14 @@ export class GitHubRelease {
  }

  async createRelease(): Promise<ReleaseResponse | undefined> {
+    // Attempt to lookup the package name from a well known location, such
+    // as package.json, if none is provided:
+    if (!this.packageName && this.releaseType) {


I moved this up to ensure the same packageName was used for tag creation on L133 as for finding the release PR (L93). IIRC I was doing something stupid like trying to set package-name to something different from package.json (or maybe just not setting it at all?).

joeldodge79 · 2021-01-07T16:42:50Z

src/github.ts

@@ -584,7 +585,7 @@ export class GitHub {
        // it's easiest/safest to just pull this out by string search.
        const version = match[2];
        if (!version) continue;
-        if (prefix && match[1] !== prefix) {
+        if (prefix && match[1] !== packageBranch(prefix)) {


I don't fully understand your comment. In

before passing it into this method

does "this method" refer to the method we're in (findMergedReleasePR) or packageBranch? you meant the former... I think it's reasonable to move packageBranch(prefix) up to packageBranch(this.packageName) in the parent call site

joeldodge79 · 2021-01-07T18:45:02Z

src/releasers/node.ts

@@ -125,11 +125,14 @@ export class Node extends ReleasePR {
  // A releaser can implement this method to automatically detect
  // the release name when creating a GitHub release, for instance by returning
  // name in package.json, or setup.py.
-  static async lookupPackageName(gh: GitHub): Promise<string | undefined> {
+  static async lookupPackageName(


nm, I see you need contents further down so L71 should probably just stay as is.

joeldodge79 · 2021-01-07T18:52:09Z

src/release-pr.ts

+    gh: GitHub,
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    path?: string
+  ): Promise<string | undefined> {


will add a test

joeldodge79 · 2021-01-07T18:52:31Z

src/util/package-branch.ts

+// limitations under the License.
+
+// map from a packageName to the prefix used in release branch creation.
+export function packageBranch(packageName: string) {


I'm going to suggest another route. updated PR coming soon

joeldodge79

@bcoe I annotated some changes in the last commit. It might be worth a few minutes syncing in person too if you have time.

joeldodge79 · 2021-01-07T18:57:38Z

src/github-release.ts

+    if (this.packageName === undefined) {
+      throw Error(
+        `could not determine package name for release repo = ${this.repoUrl}`
+      );
+    }


moved this up in my last commit so that the type checker knows that this.packageName is defined (for passing to packageBranchPrefix)

joeldodge79 · 2021-01-07T18:58:15Z

src/github-release.ts

+      this.monorepoTags
+        ? packageBranchPrefix(this.packageName, this.releaseType)
+        : undefined


Feedback: moved this packageBranchPrefix call up out of this.gh.findMergedReleasePR

joeldodge79 · 2021-01-07T18:59:38Z

src/github.ts

@@ -551,10 +551,12 @@ export class GitHub {
  async findMergedReleasePR(
    labels: string[],
    perPage = 100,
-    prefix: string | undefined = undefined,
+    branchPrefix: string | undefined = undefined,


cosmetic/readability change. I'm not married to it but as I was trying to reason about what this parameter really is this name made more sense to me (especially given the tag lookup functions taking both a prefix and a branchPrefix)

I like this change 👍

I actually recently stopped accepting the two prefixes, which seemed like a hack that wasn't working, and switched to checking for -, / and @ if we've fallen back to looking up by tag.

joeldodge79 · 2021-01-07T19:04:28Z

src/util/package-branch-prefix.ts

+// limitations under the License.
+
+// map from a packageName to the prefix used in release branch creation.
+export function packageBranchPrefix(packageName: string, releaseType?: string) {


I kept the name more generically related to converting a package name into a branch prefix. I added the releaseType argument to support future releasers possibly needing to make a similar transformation.

I like this approach 👍 I'd added some of this handling for Node.js, because I know the domain well, but I could certainly imagine us wanting similar behavior for other languages.

bcoe

This looks great to me 👍 with the caveat that it's complex enough that I'd love for us to test it in the real world.

When we land, this repository I'd expect to continue working without any configuration changes -- we can use it as a test 😄

bcoe · 2021-01-08T01:25:45Z

src/github.ts

@@ -551,10 +551,12 @@ export class GitHub {
  async findMergedReleasePR(
    labels: string[],
    perPage = 100,
-    prefix: string | undefined = undefined,
+    branchPrefix: string | undefined = undefined,


I like this change 👍

I actually recently stopped accepting the two prefixes, which seemed like a hack that wasn't working, and switched to checking for -, / and @ if we've fallen back to looking up by tag.

bcoe · 2021-01-08T01:28:17Z

src/util/package-branch-prefix.ts

+// limitations under the License.
+
+// map from a packageName to the prefix used in release branch creation.
+export function packageBranchPrefix(packageName: string, releaseType?: string) {


I like this approach 👍 I'd added some of this handling for Node.js, because I know the domain well, but I could certainly imagine us wanting similar behavior for other languages.

googleapis/release-please#669

joeldodge79 · 2021-01-08T19:49:46Z

@bcoe some udpates:

I made the change in the node releaser you pointed out so if you have a minute it would be nice to get it pushed up to the github action so I can test again on my repo
I found that adding __snapshots__/ to .eslintignore didn't solve my issue with npm run test:snap converting all single to double quotes so I left that out. I'm ok with things the way they are now, maybe I'll dig in a little more to see why my environment wants to make that change so badly.
I'm gonna work on increasing test coverage for this PR today so I put it in "draft" mode
I squashed all the commits down to keep things simpler

fixes a couple of issues: 1. if node releaser doesn't specify packageName then github-release wasn't filtering merged PRs correctly. 2. when solving the above, we need to be consistent about the relationship between packageName and the release branch name.

joeldodge79 · 2021-01-08T20:52:15Z

@bcoe ok, I think I'm done (for this round anyway). Let me know if you have a chance to push this through to master on release-please-action and I'll test it again.

…ease-fix

joeldodge79 requested a review from a team as a code owner December 16, 2020 19:11

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

joeldodge79 commented Dec 16, 2020

View reviewed changes

bcoe suggested changes Dec 18, 2020

View reviewed changes

erezrokah mentioned this pull request Jan 3, 2021

fix(monorepo): remove package scope before matching merged PR #678

Closed

4 tasks

joeldodge79 commented Jan 7, 2021

View reviewed changes

joeldodge79 force-pushed the joeldodge/node-monorepo-package-github-release-fix branch from 1ddbae0 to 95376ad Compare January 7, 2021 18:55

joeldodge79 requested a review from bcoe January 7, 2021 19:05

joeldodge79 commented Jan 7, 2021

View reviewed changes

bcoe approved these changes Jan 8, 2021

View reviewed changes

bcoe added a commit to googleapis/release-please-action that referenced this pull request Jan 8, 2021

fix(release-please): fixes for monorepo path handling

0f9fd32

googleapis/release-please#669

bcoe mentioned this pull request Jan 8, 2021

fix(release-please): fixes for monorepo path handling googleapis/release-please-action#181

Merged

bcoe added a commit to googleapis/release-please-action that referenced this pull request Jan 8, 2021

fix(release-please): fixes for monorepo path handling (#181)

349920d

googleapis/release-please#669

joeldodge79 force-pushed the joeldodge/node-monorepo-package-github-release-fix branch from bcaaf28 to aa7339f Compare January 8, 2021 19:39

joeldodge79 marked this pull request as draft January 8, 2021 19:44

joeldodge79 force-pushed the joeldodge/node-monorepo-package-github-release-fix branch from aa7339f to d8cfed6 Compare January 8, 2021 20:37

joeldodge79 marked this pull request as ready for review January 8, 2021 20:41

joeldodge79 force-pushed the joeldodge/node-monorepo-package-github-release-fix branch from d8cfed6 to 7c56589 Compare January 8, 2021 20:41

joeldodge79 force-pushed the joeldodge/node-monorepo-package-github-release-fix branch from 7c56589 to d737545 Compare January 8, 2021 20:48

Merge branch 'master' into joeldodge/node-monorepo-package-github-rel…

69206ef

…ease-fix

bcoe added the automerge Merge the pull request once unit tests and other checks pass. label Jan 9, 2021

gcf-merge-on-green bot merged commit 9f69f41 into googleapis:master Jan 9, 2021

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jan 9, 2021

joeldodge79 deleted the joeldodge/node-monorepo-package-github-release-fix branch February 9, 2021 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(monorepos): github-release not created #669

fix(monorepos): github-release not created #669

joeldodge79 commented Dec 16, 2020

codecov bot commented Dec 16, 2020 •

edited

joeldodge79 Dec 16, 2020

bcoe Dec 18, 2020

joeldodge79 Jan 5, 2021

joeldodge79 Dec 16, 2020

bcoe Dec 18, 2020

joeldodge79 Jan 7, 2021

joeldodge79 Dec 16, 2020

joeldodge79 Jan 7, 2021

bcoe commented Dec 18, 2020

bcoe left a comment

bcoe Dec 18, 2020

joeldodge79 Jan 7, 2021

bcoe Dec 18, 2020

joeldodge79 Jan 7, 2021

bcoe Dec 18, 2020

joeldodge79 left a comment

joeldodge79 Jan 7, 2021

joeldodge79 Jan 7, 2021

joeldodge79 Jan 7, 2021

joeldodge79 Jan 7, 2021

joeldodge79 Jan 7, 2021

joeldodge79 left a comment

joeldodge79 Jan 7, 2021

joeldodge79 Jan 7, 2021

joeldodge79 Jan 7, 2021

bcoe Jan 8, 2021

joeldodge79 Jan 7, 2021

bcoe Jan 8, 2021

bcoe left a comment

bcoe Jan 8, 2021

bcoe Jan 8, 2021

joeldodge79 commented Jan 8, 2021

joeldodge79 commented Jan 8, 2021

fix(monorepos): github-release not created #669

fix(monorepos): github-release not created #669

Conversation

joeldodge79 commented Dec 16, 2020

codecov bot commented Dec 16, 2020 • edited

Codecov Report

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

bcoe commented Dec 18, 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

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

Choose a reason for hiding this comment

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

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

joeldodge79 commented Jan 8, 2021

joeldodge79 commented Jan 8, 2021

codecov bot commented Dec 16, 2020 •

edited