Skip to content
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.

Sign up for GitHub

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

feat: warn if autoupdate fails on a fork #219

Merged
merged 4 commits into from
Sep 12, 2021
Merged

feat: warn if autoupdate fails on a fork #219

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
09de8fb
chore: update ubuntu version in README
chinthakagodawita Aug 28, 2021
e2c285f
feat: warn if autoupdate fails on a fork
chinthakagodawita Aug 28, 2021
9729501
feat: display error message in fork warning
chinthakagodawita Aug 30, 2021
4ea966d
fix: update merge status code check
chinthakagodawita Aug 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
44 changes: 35 additions & 9 deletions src/autoupdater.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
Expand Down Expand Up @@ -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.',
Expand Down Expand Up @@ -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) {
Expand All @@ -168,7 +171,7 @@ export class AutoUpdater {
return updated;
}

async update(pull: PullRequest): Promise<boolean> {
async update(sourceEventOwner: string, pull: PullRequest): Promise<boolean> {
const { ref } = pull.head;
ghCore.info(`Evaluating pull request #${pull.number}...`);

Expand Down Expand Up @@ -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(
Expand All @@ -221,8 +224,6 @@ export class AutoUpdater {
}
return false;
}

return true;
}

async prNeedsUpdate(pull: PullRequest): Promise<boolean> {
Expand Down Expand Up @@ -365,7 +366,11 @@ export class AutoUpdater {
return true;
}

async merge(mergeOpts: MergeParameters): Promise<boolean> {
async merge(
sourceEventOwner: string,
prNumber: number,
mergeOpts: MergeParameters,
): Promise<boolean> {
const sleep = (timeMs: number) => {
return new Promise((resolve) => {
setTimeout(resolve, timeMs);
Expand Down Expand Up @@ -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(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think it would be useful to include github's actual error message here as well?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be too verbose for the annotation? Though, I'll add it in as a debug message, might be helpful for figuring out issues

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not too bad tbh, what do you think? https://github.com/chinthakagodawita/autoupdate-test/actions/runs/1181370405

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that's good

`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;
Expand All @@ -431,6 +456,7 @@ export class AutoUpdater {
}
}
}

return true;
}
}
77 changes: 65 additions & 12 deletions test/autoupdate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -860,7 +860,7 @@ describe('test `update`', () => {
const updateSpy = jest
.spyOn(updater, 'prNeedsUpdate')
.mockResolvedValue(false);
const needsUpdate = await updater.update(<any>validPull);
const needsUpdate = await updater.update(owner, <any>validPull);
expect(needsUpdate).toEqual(false);
expect(updateSpy).toHaveBeenCalledTimes(1);
});
Expand All @@ -872,7 +872,7 @@ describe('test `update`', () => {
.spyOn(updater, 'prNeedsUpdate')
.mockResolvedValue(true);
const mergeSpy = jest.spyOn(updater, 'merge');
const needsUpdate = await updater.update(<any>validPull);
const needsUpdate = await updater.update(owner, <any>validPull);

expect(needsUpdate).toEqual(true);
expect(updateSpy).toHaveBeenCalledTimes(1);
Expand All @@ -894,7 +894,7 @@ describe('test `update`', () => {
},
};

const needsUpdate = await updater.update(<any>pull);
const needsUpdate = await updater.update(owner, <any>pull);

expect(needsUpdate).toEqual(false);
expect(updateSpy).toHaveBeenCalledTimes(1);
Expand All @@ -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(<any>validPull);
const needsUpdate = await updater.update(owner, <any>validPull);

const expectedMergeOpts = {
owner: validPull.head.repo.owner.login,
Expand All @@ -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 () => {
Expand All @@ -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(<any>validPull);
const needsUpdate = await updater.update(owner, <any>validPull);

const expectedMergeOpts = {
owner: validPull.head.repo.owner.login,
Expand All @@ -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,
);
});
});

Expand Down Expand Up @@ -973,6 +981,11 @@ describe('test `merge`', () => {
description: 'merge error',
success: false,
},
{
code: 403,
description: 'authorisation error',
success: false,
},
];

for (const responseTest of responseCodeTests) {
Expand All @@ -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);
Expand All @@ -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');
Expand All @@ -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);
});
Expand All @@ -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',
);

Expand Down