From 188096b222c53120062c742ddfbad6e8a39f62c0 Mon Sep 17 00:00:00 2001 From: meorphis <108296353+meorphis@users.noreply.github.com> Date: Wed, 24 Jul 2024 17:06:53 -0400 Subject: [PATCH] do not merge directly if we cannot enable auto-merge (#165) --- __snapshots__/github.ts.js | 4 ++-- src/github.ts | 12 +++-------- src/manifest.ts | 42 ++++++++++++++------------------------ test/github.ts | 18 +++++----------- test/manifest.ts | 15 ++++++-------- 5 files changed, 31 insertions(+), 60 deletions(-) diff --git a/__snapshots__/github.ts.js b/__snapshots__/github.ts.js index 478b00365..89b1ee076 100644 --- a/__snapshots__/github.ts.js +++ b/__snapshots__/github.ts.js @@ -451,7 +451,7 @@ exports['GitHub createRelease should raise a RequestError for other validation e "target_commitish": "abc123" } -exports['GitHub enablePullRequestAutoMerge merges release PR directly when an auto-merge given but "protected branch rules not configured for this branch" 1'] = { +exports['GitHub enablePullRequestAutoMerge does nothing when an auto-merge given but PR in "clean status" 1'] = { "query": "query pullRequestId($owner: String!, $repo: String!, $pullRequestNumber: Int!) {\n repository(name: $repo, owner: $owner) {\n pullRequest(number: $pullRequestNumber) {\n id\n }\n }\n }", "variables": { "owner": "fake", @@ -460,7 +460,7 @@ exports['GitHub enablePullRequestAutoMerge merges release PR directly when an au } } -exports['GitHub enablePullRequestAutoMerge merges release PR directly when an auto-merge given but PR in "clean status" 1'] = { +exports['GitHub enablePullRequestAutoMerge merges release PR directly when an auto-merge given but "protected branch rules not configured for this branch" 1'] = { "query": "query pullRequestId($owner: String!, $repo: String!, $pullRequestNumber: Int!) {\n repository(name: $repo, owner: $owner) {\n pullRequest(number: $pullRequestNumber) {\n id\n }\n }\n }", "variables": { "owner": "fake", diff --git a/src/github.ts b/src/github.ts index adc587813..0d0952717 100644 --- a/src/github.ts +++ b/src/github.ts @@ -2358,7 +2358,7 @@ export class GitHub { async enablePullRequestAutoMerge( pullRequestNumber: number, mergeMethod: MergeMethod - ): Promise<'auto-merged' | 'direct-merged' | 'none'> { + ): Promise<'auto-merged' | 'none'> { try { this.logger.debug('Enable PR auto-merge'); const prId = await this.queryPullRequestId(pullRequestNumber); @@ -2381,15 +2381,9 @@ export class GitHub { ) ) { this.logger.debug( - 'PR can be merged directly, do it instead of via GitHub auto-merge' + 'Auto-merge cannot be enabled - user probably has auto-merge disabled for their repo' ); - await this.octokit.pulls.merge({ - owner: this.repository.owner, - repo: this.repository.repo, - pull_number: pullRequestNumber, - merge_method: mergeMethod, - }); - return 'direct-merged'; + return 'none'; } else { throw e; } diff --git a/src/manifest.ts b/src/manifest.ts index 6e4a1e3f9..2a22b7e2b 100644 --- a/src/manifest.ts +++ b/src/manifest.ts @@ -1121,22 +1121,18 @@ export class Manifest { } ); - let directlyMerged = false; const autoMerge = this.pullRequestAutoMergeOption(pullRequest); if (autoMerge) { - const result = await this.github.enablePullRequestAutoMerge( + await this.github.enablePullRequestAutoMerge( newPullRequest.number, autoMerge.mergeMethod ); - directlyMerged = result === 'direct-merged'; } - if (!directlyMerged) { - this.github.addPullRequestReviewers({ - pullRequestNumber: newPullRequest.number, - reviewers: this.reviewers, - }); - } + this.github.addPullRequestReviewers({ + pullRequestNumber: newPullRequest.number, + reviewers: this.reviewers, + }); return newPullRequest; } @@ -1169,22 +1165,18 @@ export class Manifest { } ); - let directlyMerged = false; const autoMerge = this.pullRequestAutoMergeOption(pullRequest); if (autoMerge) { - const result = await this.github.enablePullRequestAutoMerge( + await this.github.enablePullRequestAutoMerge( updatedPullRequest.number, autoMerge.mergeMethod ); - directlyMerged = result === 'direct-merged'; } - if (!directlyMerged) { - this.github.addPullRequestReviewers({ - pullRequestNumber: updatedPullRequest.number, - reviewers: this.reviewers, - }); - } + this.github.addPullRequestReviewers({ + pullRequestNumber: updatedPullRequest.number, + reviewers: this.reviewers, + }); return updatedPullRequest; } @@ -1215,22 +1207,18 @@ export class Manifest { // TODO: consider leaving the snooze label await this.github.removeIssueLabels([SNOOZE_LABEL], snoozed.number); - let directlyMerged = false; const autoMerge = this.pullRequestAutoMergeOption(pullRequest); if (autoMerge) { - const result = await this.github.enablePullRequestAutoMerge( + await this.github.enablePullRequestAutoMerge( updatedPullRequest.number, autoMerge.mergeMethod ); - directlyMerged = result === 'direct-merged'; } - if (!directlyMerged) { - this.github.addPullRequestReviewers({ - pullRequestNumber: updatedPullRequest.number, - reviewers: this.reviewers, - }); - } + this.github.addPullRequestReviewers({ + pullRequestNumber: updatedPullRequest.number, + reviewers: this.reviewers, + }); return updatedPullRequest; } diff --git a/test/github.ts b/test/github.ts index b810be6ae..f501b073d 100644 --- a/test/github.ts +++ b/test/github.ts @@ -1271,7 +1271,7 @@ describe('GitHub', () => { req.done(); }); - it('merges release PR directly when an auto-merge given but PR in "clean status"', async () => { + it('does nothing when an auto-merge given but PR in "clean status"', async () => { const mutatePullRequestEnableAutoMergeStub = sandbox .stub(github, 'mutatePullRequestEnableAutoMerge') // eslint-disable-line @typescript-eslint/no-explicit-any .throws( @@ -1312,14 +1312,10 @@ describe('GitHub', () => { }, }, }, - }) - .put('/repos/fake/fake/pulls/123/merge', { - merge_method: 'rebase', - }) - .reply(200); + }); const result = await github.enablePullRequestAutoMerge(123, 'rebase'); - expect(result).to.equal('direct-merged'); + expect(result).to.equal('none'); sinon.assert.calledOnce(mutatePullRequestEnableAutoMergeStub); req.done(); }); @@ -1366,14 +1362,10 @@ describe('GitHub', () => { }, }, }, - }) - .put('/repos/fake/fake/pulls/123/merge', { - merge_method: 'rebase', - }) - .reply(200); + }); const result = await github.enablePullRequestAutoMerge(123, 'rebase'); - expect(result).to.equal('direct-merged'); + expect(result).to.equal('none'); sinon.assert.calledOnce(mutatePullRequestEnableAutoMergeStub); req.done(); }); diff --git a/test/manifest.ts b/test/manifest.ts index 1bea7cd11..c57ff6bf7 100644 --- a/test/manifest.ts +++ b/test/manifest.ts @@ -4995,7 +4995,7 @@ version = "3.0.0" }); const enablePullRequestAutoMergeStub = sandbox .stub(github, 'enablePullRequestAutoMerge') - .resolves('direct-merged'); + .resolves('none'); const addPullRequestReviewersStub = sandbox .stub(github, 'addPullRequestReviewers') .resolves(); @@ -5228,8 +5228,7 @@ version = "3.0.0" expect(enablePullRequestAutoMergeStub.callCount).to.equal(1); - // only called when not auto-merged - expect(addPullRequestReviewersStub.callCount).to.equal(5); + expect(addPullRequestReviewersStub.callCount).to.equal(6); }); it('enables auto-merge when filters are provided (filters: only commit type, match-all)', async () => { @@ -5246,7 +5245,7 @@ version = "3.0.0" }); const enablePullRequestAutoMergeStub = sandbox .stub(github, 'enablePullRequestAutoMerge') - .resolves('direct-merged'); + .resolves('none'); const addPullRequestReviewersStub = sandbox .stub(github, 'addPullRequestReviewers') .resolves(); @@ -5418,8 +5417,7 @@ version = "3.0.0" ); expect(enablePullRequestAutoMergeStub.callCount).to.equal(3); - // only called when not auto-merged - expect(addPullRequestReviewersStub.callCount).to.equal(1); + expect(addPullRequestReviewersStub.callCount).to.equal(4); }); it('enables auto-merge when filters are provided (filters: build-patch-minor version bump, commit filters, match-at-least-one)', async () => { @@ -5436,7 +5434,7 @@ version = "3.0.0" }); const enablePullRequestAutoMergeStub = sandbox .stub(github, 'enablePullRequestAutoMerge') - .resolves('direct-merged'); + .resolves('none'); const addPullRequestReviewersStub = sandbox .stub(github, 'addPullRequestReviewers') .resolves(); @@ -5755,8 +5753,7 @@ version = "3.0.0" ); expect(enablePullRequestAutoMergeStub.callCount).to.equal(4); - // only called when not auto-merged - expect(addPullRequestReviewersStub.callCount).to.equal(3); + expect(addPullRequestReviewersStub.callCount).to.equal(7); }); it('updates an existing pull request', async () => {