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.

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: add exponential retries on merge errors #480

Merged
merged 23 commits into from
Aug 19, 2020
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e686145
feat: add retry when trying to merge
Jul 30, 2020
8ed6829
chore: rename variable name
Aug 4, 2020
9289fbb
test: update tests
Aug 4, 2020
c97bfc0
docs: add entry in readme file
Aug 4, 2020
d1fdb79
chore: swap definition
Aug 5, 2020
29742be
chore: rename 'trial' to 'retryCount'
Aug 5, 2020
7f874f6
chore: rename `numberOfRetries` to `maximumRetries`
Aug 5, 2020
e849d10
test: add check on the retrying log messages (to ensure the backoff h…
Aug 5, 2020
06ff5e7
Merge branch 'master' into feat/CP-2089_retry_PR_on_merge_errors
ricardoatsouza Aug 6, 2020
e28bc57
test: mock endpoint instead of `merge` function; removes `export` fru…
Aug 10, 2020
a55028f
Merge branch 'master' into feat/CP-2089_retry_PR_on_merge_errors
ricardoatsouza Aug 12, 2020
ef0cedc
feat: add internal parameter for the minimum wait time in merge
Aug 14, 2020
18e4918
fix: send "maximumRetries" parameters in "index.ts"
Aug 14, 2020
7cf6771
feat: retries only on a specific error (and update tests)
Aug 14, 2020
b7d4dd0
Merge branch 'feat/CP-2089_retry_PR_on_merge_errors' of github.com:ri…
Aug 14, 2020
b43e47a
Merge branch 'master' into feat/CP-2089_retry_PR_on_merge_errors
ricardoatsouza Aug 14, 2020
d0b59dc
test: remove mergeWithRetry check
Aug 18, 2020
d5400ab
test: checks if merge function throws when a different error is given
Aug 18, 2020
2032586
test: remove minimum time parameter
Aug 18, 2020
aabf783
test: test backoff by mocking setTimeout
Aug 18, 2020
95c4ed4
Merge branch 'master' into feat/CP-2089_retry_PR_on_merge_errors
ricardoatsouza Aug 18, 2020
020ed6d
fix: lint
Aug 18, 2020
2665e62
Merge branch 'master' into feat/CP-2089_retry_PR_on_merge_errors
ricardoatsouza Aug 18, 2020
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
18 changes: 18 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,24 @@ steps:
MERGE_METHOD: MERGE
```

### Number of retries

In case the merge action fails, by default it will automatically be retried up
to three times using an exponential backoff strategy. This means, the first
retry will happen 1 second after the first failure, while the second will happen
ricardoatsouza marked this conversation as resolved.
Show resolved Hide resolved
4 seconds after the previous, the third 9 seconds, and so on.

It's possible to configure the number of retries by providing a value for
`MAXIMUM_RETRIES` (by default, the value is `3`).

```yaml
steps:
- name: Merge me!
uses: ridedott/merge-me-action@master
with:
MAXIMUM_RETRIES: 2
```

## Getting Started

These instructions will get you a copy of the project up and running on your
Expand Down
1 change: 1 addition & 0 deletions cSpell.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"language": "en-US",
"version": "0.1",
"words": [
"backoff",
"codeowners",
"commitlint",
"coverallsapp",
Expand Down
64 changes: 53 additions & 11 deletions src/common/merge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,56 @@ import {
import { parseInputMergeMethod } from '../utilities/inputParsers';
import { logDebug, logInfo } from '../utilities/log';

export interface PullRequestDetails {
commitHeadline: string;
pullRequestId: string;
reviewEdge: { node: { state: string } } | undefined;
}

const EXPONENTIAL_BACKOFF = 2;
const MINIMUM_WAIT_TIME = 1000;

const delay = async (duration: number): Promise<void> => {
return new Promise((resolve: () => void): void => {
setTimeout((): void => {
resolve();
}, duration);
});
};

/**
* Approves and merges a given Pull Request.
*/
export const merge = async (
const merge = async (
octokit: ReturnType<typeof getOctokit>,
{
commitHeadline,
pullRequestId,
reviewEdge,
}: {
commitHeadline: string;
pullRequestId: string;
reviewEdge: { node: { state: string } } | undefined;
},
pullRequestDetails: PullRequestDetails,
): Promise<void> => {
const mergeMethod = parseInputMergeMethod();

const { commitHeadline, pullRequestId, reviewEdge } = pullRequestDetails;

const mutation =
reviewEdge === undefined
? approveAndMergePullRequestMutation(mergeMethod)
: mergePullRequestMutation(mergeMethod);

await octokit.graphql(mutation, { commitHeadline, pullRequestId });
};

const shouldRetry = (error: Error): boolean =>
error.message.includes('Base branch was modified.');

export const mergeWithRetry = async (
octokit: ReturnType<typeof getOctokit>,
details: PullRequestDetails & {
maximumRetries: number;
retryCount: number;
},
): Promise<void> => {
const { retryCount, maximumRetries } = details;

try {
await octokit.graphql(mutation, { commitHeadline, pullRequestId });
await merge(octokit, details);
} catch (error) {
logInfo(
'An error ocurred while merging the Pull Request. This is usually ' +
Expand All @@ -40,5 +66,21 @@ export const merge = async (
);
/* eslint-disable-next-line @typescript-eslint/no-base-to-string */
logDebug(`Original error: ${(error as Error).toString()}.`);

if (shouldRetry(error) && retryCount <= maximumRetries) {
const nextRetryIn = retryCount ** EXPONENTIAL_BACKOFF * MINIMUM_WAIT_TIME;

logInfo(`Retrying in ${nextRetryIn.toString()}...`);

await delay(nextRetryIn);

await mergeWithRetry(octokit, {
...details,
maximumRetries,
retryCount: retryCount + 1,
});
} else {
throw error;
}
}
};
142 changes: 134 additions & 8 deletions src/eventHandlers/checkSuite/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ import { getOctokit } from '@actions/github';
import { OK } from 'http-status-codes';
import * as nock from 'nock';

import { useSetTimeoutImmediateInvocation } from '../../../test/utilities';
import { mergePullRequestMutation } from '../../graphql/mutations';
import { AllowedMergeMethods } from '../../utilities/inputParsers';
import * as log from '../../utilities/log';
import { checkSuiteHandle } from '.';

/* cspell:disable-next-line */
Expand Down Expand Up @@ -61,7 +63,7 @@ describe('check Suite event handler', (): void => {
});
nock('https://api.github.com').post('/graphql').reply(OK);

await checkSuiteHandle(octokit, 'dependabot-preview[bot]');
await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3);

expect(warningSpy).not.toHaveBeenCalled();
});
Expand Down Expand Up @@ -116,7 +118,7 @@ describe('check Suite event handler', (): void => {
})
.reply(OK);

await checkSuiteHandle(octokit, 'dependabot-preview[bot]');
await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3);
});

it('does not approve pull requests that are not mergeable', async (): Promise<
Expand Down Expand Up @@ -160,7 +162,7 @@ describe('check Suite event handler', (): void => {
},
});

await checkSuiteHandle(octokit, 'dependabot-preview[bot]');
await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3);

expect(infoSpy).toHaveBeenCalledWith(
'Pull request is not in a mergeable state: CONFLICTING.',
Expand Down Expand Up @@ -208,7 +210,7 @@ describe('check Suite event handler', (): void => {
},
});

await checkSuiteHandle(octokit, 'dependabot-preview[bot]');
await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3);

expect(infoSpy).toHaveBeenCalledWith('Pull request is already merged.');
});
Expand Down Expand Up @@ -254,7 +256,7 @@ describe('check Suite event handler', (): void => {
},
});

await checkSuiteHandle(octokit, 'dependabot-preview[bot]');
await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3);

expect(infoSpy).toHaveBeenCalledWith(
'Pull request cannot be merged cleanly. Current state: UNKNOWN.',
Expand Down Expand Up @@ -302,7 +304,7 @@ describe('check Suite event handler', (): void => {
},
});

await checkSuiteHandle(octokit, 'dependabot-preview[bot]');
await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3);

expect(infoSpy).toHaveBeenCalledWith('Pull request is not open: CLOSED.');
});
Expand All @@ -312,7 +314,7 @@ describe('check Suite event handler', (): void => {
> => {
expect.assertions(1);

await checkSuiteHandle(octokit, 'some-other-login');
await checkSuiteHandle(octokit, 'some-other-login', 3);

expect(infoSpy).toHaveBeenCalledWith(
'Pull request created by dependabot-preview[bot], not some-other-login, skipping.',
Expand All @@ -334,8 +336,132 @@ describe('check Suite event handler', (): void => {
},
});

await checkSuiteHandle(octokit, 'dependabot-preview[bot]');
await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 3);

expect(warningSpy).toHaveBeenCalled();
});

it('retries up to two times before failing', async (): Promise<void> => {
JrSchild marked this conversation as resolved.
Show resolved Hide resolved
expect.assertions(6);

nock('https://api.github.com')
.post('/graphql')
.reply(OK, {
data: {
JrSchild marked this conversation as resolved.
Show resolved Hide resolved
repository: {
pullRequest: {
commits: {
edges: [
{
node: {
commit: {
messageHeadline: COMMIT_HEADLINE,
},
},
},
],
},
id: PULL_REQUEST_ID,
mergeStateStatus: 'CLEAN',
mergeable: 'MERGEABLE',
merged: false,
reviews: {
edges: [
{
node: {
state: 'APPROVED',
},
},
],
},
state: 'OPEN',
},
},
},
})
.post('/graphql')
.times(3)
.reply(
403,
'##[error]GraphqlError: Base branch was modified. Review and try the merge again.',
);

const logDebugSpy = jest.spyOn(log, 'logDebug');
const logInfoSpy = jest.spyOn(log, 'logInfo');

useSetTimeoutImmediateInvocation();

try {
await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 2);
} catch (error) {
expect(error).toBeInstanceOf(Error);
expect(error.message).toStrictEqual(
'##[error]GraphqlError: Base branch was modified. Review and try the merge again.',
);
expect(logDebugSpy).toHaveBeenCalledTimes(3);
expect(logInfoSpy.mock.calls[1][0]).toStrictEqual(
'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.',
);
expect(logInfoSpy.mock.calls[2][0]).toStrictEqual('Retrying in 1000...');
expect(logInfoSpy.mock.calls[4][0]).toStrictEqual('Retrying in 4000...');
}
});

it('fails the backoff strategy when the error is not "Base branch was modified"', async (): Promise<
void
> => {
expect.assertions(3);

nock('https://api.github.com')
.post('/graphql')
.reply(OK, {
data: {
repository: {
pullRequest: {
commits: {
edges: [
{
node: {
commit: {
messageHeadline: COMMIT_HEADLINE,
},
},
},
],
},
id: PULL_REQUEST_ID,
mergeStateStatus: 'CLEAN',
mergeable: 'MERGEABLE',
merged: false,
reviews: {
edges: [
{
node: {
state: 'APPROVED',
},
},
],
},
state: 'OPEN',
},
},
},
})
.post('/graphql')
.reply(403, '##[error]GraphqlError: This is a different error.');

const logInfoSpy = jest.spyOn(log, 'logInfo');

try {
await checkSuiteHandle(octokit, 'dependabot-preview[bot]', 2);
} catch (error) {
expect(error).toBeInstanceOf(Error);
expect(error.message).toStrictEqual(
'##[error]GraphqlError: This is a different error.',
);
expect(logInfoSpy.mock.calls[1][0]).toStrictEqual(
'An error ocurred while merging the Pull Request. This is usually caused by the base branch being out of sync with the target branch. In this case, the base branch must be rebased. Some tools, such as Dependabot, do that automatically.',
);
}
});
});
10 changes: 7 additions & 3 deletions src/eventHandlers/checkSuite/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import { context, getOctokit } from '@actions/github';

import { merge } from '../../common/merge';
import { mergeWithRetry } from '../../common/merge';
import { findPullRequestInfo as findPullRequestInformation } from '../../graphql/queries';
import {
MergeableState,
Expand Down Expand Up @@ -91,6 +91,7 @@ const getPullRequestInformation = async (

const tryMerge = async (
octokit: ReturnType<typeof getOctokit>,
maximumRetries: number,
{
commitMessageHeadline,
mergeStateStatus,
Expand Down Expand Up @@ -124,9 +125,11 @@ const tryMerge = async (
} else if (pullRequestState !== 'OPEN') {
logInfo(`Pull request is not open: ${pullRequestState}.`);
} else {
await merge(octokit, {
await mergeWithRetry(octokit, {
commitHeadline: commitMessageHeadline,
maximumRetries,
pullRequestId,
retryCount: 1,
reviewEdge: reviewEdges[0],
});
}
Expand All @@ -135,6 +138,7 @@ const tryMerge = async (
export const checkSuiteHandle = async (
octokit: ReturnType<typeof getOctokit>,
gitHubLogin: string,
maximumRetries: number,
): Promise<void> => {
const pullRequests = context.payload.check_suite.pull_requests as Array<{
number: number;
Expand Down Expand Up @@ -170,7 +174,7 @@ export const checkSuiteHandle = async (
)}.`,
);

await tryMerge(octokit, pullRequestInformation);
await tryMerge(octokit, maximumRetries, pullRequestInformation);
}
}
};
Loading