From 09de8fb9053435795c33ba1c177b8571c2f60ba9 Mon Sep 17 00:00:00 2001 From: Chinthaka Godawita Date: Sat, 28 Aug 2021 12:21:40 +1000 Subject: [PATCH 1/4] chore: update ubuntu version in README --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 0f895b5b..2f639467 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,7 @@ on: jobs: autoupdate: name: autoupdate - runs-on: ubuntu-18.04 + runs-on: ubuntu-20.04 steps: - uses: docker://chinthakagodawita/autoupdate-action:v1 env: From e2c285feab4a0e6e93dca2ef3b865b1dedd30b4c Mon Sep 17 00:00:00 2001 From: Chinthaka Godawita Date: Sat, 28 Aug 2021 12:56:48 +1000 Subject: [PATCH 2/4] feat: warn if autoupdate fails on a fork --- src/autoupdater.ts | 44 ++++++++++++++++++----- test/autoupdate.test.ts | 77 ++++++++++++++++++++++++++++++++++------- 2 files changed, 100 insertions(+), 21 deletions(-) diff --git a/src/autoupdater.ts b/src/autoupdater.ts index 6fa718d5..19dacd0e 100644 --- a/src/autoupdater.ts +++ b/src/autoupdater.ts @@ -9,7 +9,7 @@ import { WorkflowRunEvent, } from '@octokit/webhooks-definitions/schema'; import { ConfigLoader } from './config-loader'; -import { Endpoints } from '@octokit/types'; +import { Endpoints, RequestError } from '@octokit/types'; type PullRequestResponse = Endpoints['GET /repos/{owner}/{repo}/pulls/{pull_number}']['response']; @@ -50,7 +50,10 @@ export class AutoUpdater { ghCore.info(`Handling pull_request event triggered by action '${action}'`); - const isUpdated = await this.update(pull_request); + const isUpdated = await this.update( + pull_request.head.repo.owner.login, + pull_request, + ); if (isUpdated) { ghCore.info( 'Auto update complete, pull request branch was updated with changes from the base branch.', @@ -152,7 +155,7 @@ export class AutoUpdater { let pull: PullRequestResponse['data']; for (pull of pullsPage.data) { ghCore.startGroup(`PR-${pull.number}`); - const isUpdated = await this.update(pull); + const isUpdated = await this.update(owner, pull); ghCore.endGroup(); if (isUpdated) { @@ -168,7 +171,7 @@ export class AutoUpdater { return updated; } - async update(pull: PullRequest): Promise { + async update(sourceEventOwner: string, pull: PullRequest): Promise { const { ref } = pull.head; ghCore.info(`Evaluating pull request #${pull.number}...`); @@ -211,7 +214,7 @@ export class AutoUpdater { } try { - await this.merge(mergeOpts); + return await this.merge(sourceEventOwner, pull.number, mergeOpts); } catch (e: unknown) { if (e instanceof Error) { ghCore.error( @@ -221,8 +224,6 @@ export class AutoUpdater { } return false; } - - return true; } async prNeedsUpdate(pull: PullRequest): Promise { @@ -365,7 +366,11 @@ export class AutoUpdater { return true; } - async merge(mergeOpts: MergeParameters): Promise { + async merge( + sourceEventOwner: string, + prNumber: number, + mergeOpts: MergeParameters, + ): Promise { const sleep = (timeMs: number) => { return new Promise((resolve) => { setTimeout(resolve, timeMs); @@ -404,13 +409,33 @@ export class AutoUpdater { break; } catch (e: unknown) { if (e instanceof Error) { + /** + * If this update was against a fork and we got a 403 then it's + * probably because we don't have access to it. + */ + if ( + 'status' in e && + (e as RequestError).status === 403 && + sourceEventOwner !== mergeOpts.owner + ) { + ghCore.error( + `Could not update pull request #${prNumber} due to an authorisation error. This is probably because this pull request is from a fork and the current token does not have write access to the forked repository.`, + ); + + return false; + } + + // Ignore conflicts if configured to do so. if ( e.message === 'Merge conflict' && mergeConflictAction === 'ignore' ) { ghCore.info('Merge conflict detected, skipping update.'); - break; + + return false; } + + // Else, throw an error so we don't continue retrying. if (e.message === 'Merge conflict') { ghCore.error('Merge conflict error trying to update branch'); throw e; @@ -431,6 +456,7 @@ export class AutoUpdater { } } } + return true; } } diff --git a/test/autoupdate.test.ts b/test/autoupdate.test.ts index cb973255..bc68072b 100644 --- a/test/autoupdate.test.ts +++ b/test/autoupdate.test.ts @@ -860,7 +860,7 @@ describe('test `update`', () => { const updateSpy = jest .spyOn(updater, 'prNeedsUpdate') .mockResolvedValue(false); - const needsUpdate = await updater.update(validPull); + const needsUpdate = await updater.update(owner, validPull); expect(needsUpdate).toEqual(false); expect(updateSpy).toHaveBeenCalledTimes(1); }); @@ -872,7 +872,7 @@ describe('test `update`', () => { .spyOn(updater, 'prNeedsUpdate') .mockResolvedValue(true); const mergeSpy = jest.spyOn(updater, 'merge'); - const needsUpdate = await updater.update(validPull); + const needsUpdate = await updater.update(owner, validPull); expect(needsUpdate).toEqual(true); expect(updateSpy).toHaveBeenCalledTimes(1); @@ -894,7 +894,7 @@ describe('test `update`', () => { }, }; - const needsUpdate = await updater.update(pull); + const needsUpdate = await updater.update(owner, pull); expect(needsUpdate).toEqual(false); expect(updateSpy).toHaveBeenCalledTimes(1); @@ -910,7 +910,7 @@ describe('test `update`', () => { .spyOn(updater, 'prNeedsUpdate') .mockResolvedValue(true); const mergeSpy = jest.spyOn(updater, 'merge').mockResolvedValue(true); - const needsUpdate = await updater.update(validPull); + const needsUpdate = await updater.update(owner, validPull); const expectedMergeOpts = { owner: validPull.head.repo.owner.login, @@ -922,7 +922,11 @@ describe('test `update`', () => { expect(needsUpdate).toEqual(true); expect(updateSpy).toHaveBeenCalledTimes(1); - expect(mergeSpy).toHaveBeenCalledWith(expectedMergeOpts); + expect(mergeSpy).toHaveBeenCalledWith( + owner, + validPull.number, + expectedMergeOpts, + ); }); test('merge with no message', async () => { @@ -933,7 +937,7 @@ describe('test `update`', () => { .spyOn(updater, 'prNeedsUpdate') .mockResolvedValue(true); const mergeSpy = jest.spyOn(updater, 'merge').mockResolvedValue(true); - const needsUpdate = await updater.update(validPull); + const needsUpdate = await updater.update(owner, validPull); const expectedMergeOpts = { owner: validPull.head.repo.owner.login, @@ -944,7 +948,11 @@ describe('test `update`', () => { expect(needsUpdate).toEqual(true); expect(updateSpy).toHaveBeenCalledTimes(1); - expect(mergeSpy).toHaveBeenCalledWith(expectedMergeOpts); + expect(mergeSpy).toHaveBeenCalledWith( + owner, + validPull.number, + expectedMergeOpts, + ); }); }); @@ -973,6 +981,11 @@ describe('test `merge`', () => { description: 'merge error', success: false, }, + { + code: 403, + description: 'authorisation error', + success: false, + }, ]; for (const responseTest of responseCodeTests) { @@ -991,9 +1004,9 @@ describe('test `merge`', () => { .reply(responseTest.code); if (responseTest.success) { - await updater.merge(mergeOpts); + await updater.merge(owner, 1, mergeOpts); } else { - await expect(updater.merge(mergeOpts)).rejects.toThrowError(); + await expect(updater.merge(owner, 1, mergeOpts)).rejects.toThrowError(); } expect(scope.isDone()).toEqual(true); @@ -1018,13 +1031,53 @@ describe('test `merge`', () => { scopes.push(scope); } - await expect(updater.merge(mergeOpts)).rejects.toThrowError(); + await expect(updater.merge(owner, 1, mergeOpts)).rejects.toThrowError(); for (const scope of scopes) { expect(scope.isDone()).toEqual(true); } }); + test('handles fork authorisation errors', async () => { + (config.retryCount as jest.Mock).mockReturnValue(0); + const updater = new AutoUpdater(config, emptyEvent); + + const scope = nock('https://api.github.com:443') + .post(`/repos/${owner}/${repo}/merges`, { + commit_message: mergeOpts.commit_message, + base: mergeOpts.base, + head: mergeOpts.head, + }) + .reply(403, { + message: 'Must have admin rights to Repository.', + }); + + await updater.merge('some-other-owner', 1, mergeOpts); + + expect(scope.isDone()).toEqual(true); + }); + + test('throws an error on non-fork authorisation errors', async () => { + (config.retryCount as jest.Mock).mockReturnValue(0); + const updater = new AutoUpdater(config, emptyEvent); + + const scope = nock('https://api.github.com:443') + .post(`/repos/${owner}/${repo}/merges`, { + commit_message: mergeOpts.commit_message, + base: mergeOpts.base, + head: mergeOpts.head, + }) + .reply(403, { + message: 'Must have admin rights to Repository.', + }); + + await expect(updater.merge(owner, 1, mergeOpts)).rejects.toThrowError( + 'Must have admin rights to Repository.', + ); + + expect(scope.isDone()).toEqual(true); + }); + test('ignore merge conflicts', async () => { (config.retryCount as jest.Mock).mockReturnValue(0); (config.mergeConflictAction as jest.Mock).mockReturnValue('ignore'); @@ -1040,7 +1093,7 @@ describe('test `merge`', () => { message: 'Merge conflict', }); - await updater.merge(mergeOpts); + await updater.merge(owner, 1, mergeOpts); expect(scope.isDone()).toEqual(true); }); @@ -1060,7 +1113,7 @@ describe('test `merge`', () => { message: 'Merge conflict', }); - await expect(updater.merge(mergeOpts)).rejects.toThrowError( + await expect(updater.merge(owner, 1, mergeOpts)).rejects.toThrowError( 'Merge conflict', ); From 9729501aaff9b0b4d5b8218bef7b8ff27275f92c Mon Sep 17 00:00:00 2001 From: Chinthaka Godawita Date: Mon, 30 Aug 2021 18:03:56 +1000 Subject: [PATCH 3/4] feat: display error message in fork warning --- src/autoupdater.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/autoupdater.ts b/src/autoupdater.ts index 19dacd0e..eae35c30 100644 --- a/src/autoupdater.ts +++ b/src/autoupdater.ts @@ -418,8 +418,10 @@ export class AutoUpdater { (e as RequestError).status === 403 && sourceEventOwner !== mergeOpts.owner ) { + const error = e as Error; + ghCore.error( - `Could not update pull request #${prNumber} due to an authorisation error. This is probably because this pull request is from a fork and the current token does not have write access to the forked repository.`, + `Could not update pull request #${prNumber} due to an authorisation error. This is probably because this pull request is from a fork and the current token does not have write access to the forked repository. Error was: ${error.message}`, ); return false; From 4ea966d2d26d020fdd1893e769e74f6c3fa63f5a Mon Sep 17 00:00:00 2001 From: Chinthaka Godawita Date: Tue, 31 Aug 2021 19:49:02 +1000 Subject: [PATCH 4/4] fix: update merge status code check The Github API now returns a HTTP 201 on successful merge. --- src/autoupdater.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/autoupdater.ts b/src/autoupdater.ts index eae35c30..53c5108a 100644 --- a/src/autoupdater.ts +++ b/src/autoupdater.ts @@ -383,7 +383,7 @@ export class AutoUpdater { // See https://developer.github.com/v3/repos/merging/#perform-a-merge const { status } = mergeResp; - if (status === 200) { + if (status === 200 || status === 201) { ghCore.info( `Branch update successful, new branch HEAD: ${mergeResp.data.sha}.`, );