-
Notifications
You must be signed in to change notification settings - Fork 379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(monorepos): github-release not created #669
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
|
||
import chalk = require('chalk'); | ||
import {checkpoint, CheckpointType} from './util/checkpoint'; | ||
import {packageBranchPrefix} from './util/package-branch-prefix'; | ||
import {ReleasePRFactory} from './release-pr-factory'; | ||
import {GitHub, OctokitAPIs} from './github'; | ||
import {parse} from 'semver'; | ||
|
@@ -78,14 +79,29 @@ 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) { | ||
this.packageName = await ReleasePRFactory.class( | ||
this.releaseType | ||
).lookupPackageName(this.gh, this.path); | ||
} | ||
if (this.packageName === undefined) { | ||
throw Error( | ||
`could not determine package name for release repo = ${this.repoUrl}` | ||
); | ||
} | ||
Comment on lines
+89
to
+93
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved this up in my last commit so that the type checker knows that |
||
|
||
// In most configurations, createRelease() should be called close to when | ||
// a release PR is merged, e.g., a GitHub action that kicks off this | ||
// workflow on merge. For tis reason, we can pull a fairly small number of PRs: | ||
const pageSize = 25; | ||
const gitHubReleasePR = await this.gh.findMergedReleasePR( | ||
this.labels, | ||
pageSize, | ||
this.monorepoTags ? this.packageName : undefined | ||
this.monorepoTags | ||
? packageBranchPrefix(this.packageName, this.releaseType) | ||
: undefined | ||
Comment on lines
+102
to
+104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feedback: moved this |
||
); | ||
if (!gitHubReleasePR) { | ||
checkpoint('no recent release PRs found', CheckpointType.Failure); | ||
|
@@ -111,24 +127,11 @@ export class GitHubRelease { | |
`found release notes: \n---\n${chalk.grey(latestReleaseNotes)}\n---\n`, | ||
CheckpointType.Success | ||
); | ||
|
||
// Attempt to lookup the package name from a well known location, such | ||
// as package.json, if none is provided: | ||
if (!this.packageName && this.releaseType) { | ||
this.packageName = await ReleasePRFactory.class( | ||
this.releaseType | ||
).lookupPackageName(this.gh); | ||
} | ||
// Go uses '/' for a tag separator, rather than '-': | ||
let tagSeparator = '-'; | ||
if (this.releaseType) { | ||
tagSeparator = ReleasePRFactory.class(this.releaseType).tagSeparator(); | ||
} | ||
if (this.packageName === undefined) { | ||
throw Error( | ||
`could not determine package name for release repo = ${this.repoUrl}` | ||
); | ||
} | ||
|
||
const release = await this.gh.createRelease( | ||
this.packageName, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -551,10 +551,12 @@ export class GitHub { | |
async findMergedReleasePR( | ||
labels: string[], | ||
perPage = 100, | ||
prefix: string | undefined = undefined, | ||
branchPrefix: string | undefined = undefined, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
preRelease = true | ||
): Promise<GitHubReleasePR | undefined> { | ||
prefix = prefix?.endsWith('-') ? prefix.replace(/-$/, '') : prefix; | ||
branchPrefix = branchPrefix?.endsWith('-') | ||
? branchPrefix.replace(/-$/, '') | ||
: branchPrefix; | ||
const baseLabel = await this.getBaseLabel(); | ||
const pullsResponse = (await this.request( | ||
`GET /repos/:owner/:repo/pulls?state=closed&per_page=${perPage}${ | ||
|
@@ -594,9 +596,9 @@ 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 (branchPrefix && match[1] !== branchPrefix) { | ||
continue; | ||
} else if (!prefix && match[1]) { | ||
} else if (!branchPrefix && match[1]) { | ||
continue; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ type PullsListResponseItems = PromiseValue< | |
import * as semver from 'semver'; | ||
|
||
import {checkpoint, CheckpointType} from './util/checkpoint'; | ||
import {packageBranchPrefix} from './util/package-branch-prefix'; | ||
import {ConventionalCommits} from './conventional-commits'; | ||
import {GitHub, GitHubTag, OctokitAPIs} from './github'; | ||
import {Commit} from './graphql-to-commits'; | ||
|
@@ -98,6 +99,7 @@ export class ReleasePR { | |
snapshot?: boolean; | ||
lastPackageVersion?: string; | ||
changelogSections?: []; | ||
releaseType: string; | ||
|
||
constructor(options: ReleasePROptions) { | ||
this.bumpMinorPreMajor = options.bumpMinorPreMajor || false; | ||
|
@@ -123,6 +125,7 @@ export class ReleasePR { | |
this.gh = this.gitHubInstance(options.octokitAPIs); | ||
|
||
this.changelogSections = options.changelogSections; | ||
this.releaseType = options.releaseType; | ||
} | ||
|
||
async run(): Promise<number | undefined> { | ||
|
@@ -186,8 +189,12 @@ export class 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. | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
static async lookupPackageName(gh: GitHub): Promise<string | undefined> { | ||
static async lookupPackageName( | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
gh: GitHub, | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
path?: string | ||
): Promise<string | undefined> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will add a test |
||
return Promise.resolve(undefined); | ||
} | ||
|
||
|
@@ -273,11 +280,10 @@ export class ReleasePR { | |
const updates = options.updates; | ||
const version = options.version; | ||
const includePackageName = options.includePackageName; | ||
// Do not include npm style @org/ prefixes in the branch name: | ||
const branchPrefix = this.packageName.match(/^@[\w-]+\//) | ||
? this.packageName.split('/')[1] | ||
: this.packageName; | ||
|
||
const branchPrefix = packageBranchPrefix( | ||
this.packageName, | ||
this.releaseType | ||
); | ||
const title = includePackageName | ||
? `chore: release ${this.packageName} ${version}` | ||
: `chore: release ${version}`; | ||
|
@@ -321,13 +327,17 @@ export class ReleasePR { | |
return changelogEntry.split('\n').length === 1; | ||
} | ||
|
||
addPath(file: string) { | ||
if (this.path === undefined) { | ||
static addPathStatic(file: string, path?: string) { | ||
if (path === undefined) { | ||
return file; | ||
} else { | ||
const path = this.path.replace(/[/\\]$/, ''); | ||
path = path.replace(/[/\\]$/, ''); | ||
file = file.replace(/^[/\\]/, ''); | ||
return `${path}/${file}`; | ||
} | ||
} | ||
|
||
addPath(file: string) { | ||
return ReleasePR.addPathStatic(file, this.path); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ import {ReleasePR, ReleaseCandidate} from '../release-pr'; | |
import {ConventionalCommits} from '../conventional-commits'; | ||
import {GitHub, GitHubTag, GitHubFileContents} from '../github'; | ||
import {checkpoint, CheckpointType} from '../util/checkpoint'; | ||
import {packageBranchPrefix} from '../util/package-branch-prefix'; | ||
import {Update} from '../updaters/update'; | ||
import {Commit} from '../graphql-to-commits'; | ||
|
||
|
@@ -29,8 +30,18 @@ import {SamplesPackageJson} from '../updaters/samples-package-json'; | |
export class Node extends ReleasePR { | ||
static releaserName = 'node'; | ||
protected async _run(): Promise<number | undefined> { | ||
// Make an effort to populate packageName from the contents of | ||
// the package.json, rather than forcing this to be set: | ||
const contents: GitHubFileContents = await this.gh.getFileContents( | ||
this.addPath('package.json') | ||
); | ||
const pkg = JSON.parse(contents.parsedContent); | ||
if (pkg.name) this.packageName = pkg.name; | ||
|
||
const latestTag: GitHubTag | undefined = await this.gh.latestTag( | ||
this.monorepoTags ? `${this.packageName}-` : undefined | ||
this.monorepoTags | ||
? `${packageBranchPrefix(this.packageName, 'node')}-` | ||
: undefined | ||
); | ||
const commits: Commit[] = await this.commits({ | ||
sha: latestTag ? latestTag.sha : undefined, | ||
|
@@ -68,14 +79,6 @@ export class Node extends ReleasePR { | |
|
||
const updates: Update[] = []; | ||
|
||
// Make an effort to populate packageName from the contents of | ||
// the package.json, rather than forcing this to be set: | ||
const contents: GitHubFileContents = await this.gh.getFileContents( | ||
this.addPath('package.json') | ||
); | ||
const pkg = JSON.parse(contents.parsedContent); | ||
if (pkg.name) this.packageName = pkg.name; | ||
|
||
updates.push( | ||
new PackageJson({ | ||
path: this.addPath('package-lock.json'), | ||
|
@@ -125,11 +128,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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems like some code above (L71) could be DRYed out using this method unless I'm missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nm, I see you need |
||
gh: GitHub, | ||
path?: string | ||
): Promise<string | undefined> { | ||
// Make an effort to populate packageName from the contents of | ||
// the package.json, rather than forcing this to be set: | ||
const contents: GitHubFileContents = await gh.getFileContents( | ||
'package.json' | ||
this.addPathStatic('package.json', path) | ||
); | ||
const pkg = JSON.parse(contents.parsedContent); | ||
if (pkg.name) return pkg.name; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
// 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. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
// map from a packageName to the prefix used in release branch creation. | ||
export function packageBranchPrefix(packageName: string, releaseType?: string) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kept the name more generically related to converting a package name into a branch prefix. I added the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
let branchPrefix: string; | ||
switch (releaseType) { | ||
case 'node': { | ||
branchPrefix = packageName.match(/^@[\w-]+\//) | ||
? packageName.split('/')[1] | ||
: packageName; | ||
break; | ||
} | ||
default: { | ||
branchPrefix = packageName; | ||
} | ||
} | ||
return branchPrefix; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just been specifying package-name in my monorepo implementations, this seems smarter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 setpackage-name
to something different from package.json (or maybe just not setting it at all?).