From b0db15fadc04c2e5d85db1ed4f8a6982a39dbb4b Mon Sep 17 00:00:00 2001 From: "Benjamin E. Coe" Date: Mon, 20 May 2019 13:43:48 -0700 Subject: [PATCH] fix: force update branch rather than closing and reopening PRs (#152) fixes: #141, #128 --- package-lock.json | 2 +- src/github.ts | 114 +++++++++++++++++++++++++++--------------- src/release-pr.ts | 3 +- system-test/github.ts | 3 +- 4 files changed, 80 insertions(+), 42 deletions(-) diff --git a/package-lock.json b/package-lock.json index 8e6412ed8..23b6bd474 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "release-please", - "version": "1.5.1", + "version": "1.6.0", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/src/github.ts b/src/github.ts index 8ef18bd3b..db6ef2e61 100644 --- a/src/github.ts +++ b/src/github.ts @@ -49,6 +49,7 @@ interface GitHubPR { body: string; sha: string; updates: Update[]; + labels: string[]; } export class GitHub { @@ -267,26 +268,58 @@ export class GitHub { async openPR(options: GitHubPR): Promise { let refName = await this.refByBranchName(options.branch); + let openReleasePR: PullsListResponseItem|undefined; // If the branch exists, we delete it and create a new branch // with the same name; this results in the existing PR being closed. if (!refName) { refName = `refs/heads/${options.branch}`; + + // the branch didn't yet exist, so make it. + try { + checkpoint( + `creating branch ${chalk.green(options.branch)}`, + CheckpointType.Success); + await this.octokit.git.createRef({ + owner: this.owner, + repo: this.repo, + ref: refName, + sha: options.sha + }); + } catch (err) { + if (err.status === 404) { + // the most likely cause of a 404 during this step is actually + // that the user does not have access to the repo: + throw new AuthError(); + } else { + throw err; + } + } + } else { try { checkpoint( `branch ${chalk.red(options.branch)} already exists`, CheckpointType.Failure); - await this.octokit.git.deleteRef({ + + // check if there's an existing PR, so that we can opt to update it + // rather than creating a new PR. + (await this.findOpenReleasePRs(options.labels)) + .forEach((releasePR: PullsListResponseItem) => { + if (refName && refName.indexOf(releasePR.head.ref) !== -1) { + openReleasePR = releasePR; + } + }); + + await this.octokit.git.updateRef({ owner: this.owner, repo: this.repo, - // DELETE requires that the refs/ prefix used during - // creation be removed; this seems like a bug we should log. - ref: refName.replace('refs/', '') + // TODO: remove the replace logic depending on the outcome of: + // https://github.com/octokit/rest.js/issues/1039. + ref: refName.replace('refs/', ''), + sha: options.sha, + force: true }); - checkpoint( - `branch ${chalk.red(options.branch)} deleted`, - CheckpointType.Success); } catch (err) { if (err.status === 404) { // the most likely cause of a 404 during this step is actually @@ -298,39 +331,41 @@ export class GitHub { } } - // always create the branch (it was potentially deleted in the prior step). - try { + await this.updateFiles(options.updates, options.branch, refName); + + if (openReleasePR) { + // TODO: dig into why `updateRef` closes an issue attached + // to the branch being updated: + // https://github.com/octokit/rest.js/issues/1373 checkpoint( - `creating branch ${chalk.green(options.branch)}`, + `update pull-request #${openReleasePR.number}: ${ + chalk.yellow(options.title)}`, CheckpointType.Success); - await this.octokit.git.createRef( - {owner: this.owner, repo: this.repo, ref: refName, sha: options.sha}); - } catch (err) { - if (err.status === 404) { - // the most likely cause of a 404 during this step is actually - // that the user does not have access to the repo: - throw new AuthError(); - } else { - throw err; - } + await this.octokit.pulls.update({ + pull_number: openReleasePR.number, + owner: this.owner, + repo: this.repo, + title: options.title, + body: options.body, + state: 'open', + base: 'master' + }); + return openReleasePR.number; + } else { + checkpoint( + `open pull-request: ${chalk.yellow(options.title)}`, + CheckpointType.Success); + const resp: Response = + await this.octokit.pulls.create({ + owner: this.owner, + repo: this.repo, + title: options.title, + body: options.body, + head: options.branch, + base: 'master' + }); + return resp.data.number; } - - await this.updateFiles(options.updates, options.branch, refName); - - checkpoint( - `open pull-request: ${chalk.yellow(options.title)}`, - CheckpointType.Success); - const resp: Response = - await this.octokit.pulls.create({ - owner: this.owner, - repo: this.repo, - title: options.title, - body: options.body, - head: options.branch, - base: 'master' - }); - - return resp.data.number; } async updateFiles(updates: Update[], branch: string, refName: string) { @@ -436,7 +471,8 @@ export class GitHub { } async removeLabels(labels: string[], prNumber: number) { - await labels.forEach(async (label) => { + for (let i = 0, label; i < labels.length; i++) { + label = labels[i]; checkpoint( `removing label ${chalk.green(label)} from ${ chalk.green('' + prNumber)}`, @@ -447,7 +483,7 @@ export class GitHub { issue_number: prNumber, name: label }); - }); + } } } diff --git a/src/release-pr.ts b/src/release-pr.ts index 5f60e8e03..6086ac117 100644 --- a/src/release-pr.ts +++ b/src/release-pr.ts @@ -152,7 +152,8 @@ export class ReleasePR { sha, updates, title, - body + body, + labels: this.labels }); await this.gh.addLabels(pr, this.labels); await this.closeStaleReleasePRs(pr); diff --git a/system-test/github.ts b/system-test/github.ts index 524261416..0b8c6897b 100644 --- a/system-test/github.ts +++ b/system-test/github.ts @@ -125,7 +125,8 @@ describe('GitHub', () => { version: '1.3.0', title: 'version 1.3.0', body: 'my PR body', - updates: [] + updates: [], + labels: [] }) .catch(err => { nbr.nockDone();