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

Jump to bottom

feat(github): Detect GitHub automerge setting #12398

Merged
merged 4 commits into from Oct 29, 2021
Merged

feat(github): Detect GitHub automerge setting #12398

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6fbf3bb
feat(github): Detect GitHub automerge setting
Oct 29, 2021
463c56c
Fix snapshot
Oct 29, 2021
cdc9d02
Fix coverage
Oct 29, 2021
94eb0bf
Merge branch 'main' into feat/github-automerge-detect-setting
rarkins Oct 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/platform/github/graphql.ts
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@ query($owner: String!, $name: String!) {
isFork
isArchived
nameWithOwner
autoMergeAllowed
mergeCommitAllowed
rebaseMergeAllowed
squashMergeAllowed
Expand Down
103 changes: 9 additions & 94 deletions lib/platform/github/index.spec.ts
View file
Open in desktop
Expand Up @@ -7,7 +7,6 @@ import {
} from '../../constants/error-messages';
import { BranchStatus, PrState, VulnerabilityAlert } from '../../types';
import * as _repoCache from '../../util/cache/repository';
import { Cache } from '../../util/cache/repository/types';
import * as _git from '../../util/git';
import type { CreatePRConfig, Platform } from '../types';

Expand Down Expand Up @@ -162,6 +161,7 @@ describe('platform/github/index', () => {
isFork: false,
isArchived: false,
nameWithOwner: repository,
autoMergeAllowed: true,
mergeCommitAllowed: true,
rebaseMergeAllowed: true,
squashMergeAllowed: true,
Expand Down Expand Up @@ -1963,12 +1963,6 @@ describe('platform/github/index', () => {
},
};

let cache: Cache;
beforeEach(() => {
cache = {};
repoCache.getCache.mockReturnValue(cache);
});

it('should set automatic merge', async () => {
const scope = await mockScope();
scope.post('/graphql').reply(200, graphqlAutomergeResp);
Expand All @@ -1984,107 +1978,28 @@ describe('platform/github/index', () => {
]);
});

it('should stop trying after GraphQL error', async () => {
it('should handle automatic merge', async () => {
const scope = await mockScope();
scope
.post('/graphql')
.reply(200, graphqlAutomergeErrorResp)
.post('/repos/some/repo/pulls')
.reply(200, createdPrResp)
.post('/repos/some/repo/issues/123/labels')
.reply(200, []);

await github.createPr(prConfig);
await github.createPr(prConfig);

scope.post('/graphql').reply(200, graphqlAutomergeResp);
const pr = await github.createPr(prConfig);
expect(pr).toMatchObject({ number: 123 });
expect(httpMock.getTrace()).toMatchObject([
graphqlGetRepo,
restCreatePr,
restAddLabels,
graphqlAutomerge,
restCreatePr,
restAddLabels,
]);
});

it('should retry 24 hours after GraphQL error', async () => {
const scope = await mockScope();
scope
.post('/graphql')
.reply(200, graphqlAutomergeErrorResp)
.post('/repos/some/repo/pulls')
.reply(200, createdPrResp)
.post('/repos/some/repo/issues/123/labels')
.reply(200, [])
.post('/repos/some/repo/pulls')
.reply(200, createdPrResp)
.post('/repos/some/repo/issues/123/labels')
.reply(200, [])
.post('/graphql')
.reply(200, graphqlAutomergeResp);

// Error occured
const t1 = DateTime.local().toMillis();
await github.createPr(prConfig);
const t2 = DateTime.local().toMillis();

expect(cache.lastPlatformAutomergeFailure).toBeString();

let failedAt = DateTime.fromISO(cache.lastPlatformAutomergeFailure);

expect(failedAt.toMillis()).toBeGreaterThanOrEqual(t1);
expect(failedAt.toMillis()).toBeLessThanOrEqual(t2);

// Too early
failedAt = failedAt.minus({ hours: 12 });
cache.lastPlatformAutomergeFailure = failedAt.toISO();
await github.createPr(prConfig);
expect(cache.lastPlatformAutomergeFailure).toEqual(failedAt.toISO());

// Now should retry
failedAt = failedAt.minus({ hours: 12 });
cache.lastPlatformAutomergeFailure = failedAt.toISO();
await github.createPr(prConfig);

expect(httpMock.getTrace()).toMatchObject([
// 1
graphqlGetRepo,
restCreatePr,
restAddLabels,
graphqlAutomerge, // error
// 2
restCreatePr,
restAddLabels,
// 3
restCreatePr,
restAddLabels,
graphqlAutomerge, // retry
]);
});

it('should keep trying after HTTP error', async () => {
it('should handle automerge errors', async () => {
const scope = await mockScope();
scope
.post('/graphql')
.reply(500)
.post('/repos/some/repo/pulls')
.reply(200, createdPrResp)
.post('/repos/some/repo/issues/123/labels')
.reply(200, [])
.post('/graphql')
.reply(200, graphqlAutomergeResp);

await github.createPr(prConfig);
await github.createPr(prConfig);

scope.post('/graphql').reply(200, graphqlAutomergeErrorResp);
const pr = await github.createPr(prConfig);
expect(pr).toMatchObject({ number: 123 });
expect(httpMock.getTrace()).toMatchObject([
graphqlGetRepo,
restCreatePr,
restAddLabels,
graphqlAutomerge,
restCreatePr,
restAddLabels,
graphqlAutomerge,
]);
});
});
Expand Down
52 changes: 15 additions & 37 deletions lib/platform/github/index.ts
View file
Open in desktop
Expand Up @@ -19,7 +19,6 @@ import {
import { logger } from '../../logger';
import { BranchStatus, PrState, VulnerabilityAlert } from '../../types';
import { ExternalHostError } from '../../types/errors/external-host-error';
import { getCache } from '../../util/cache/repository';
import * as git from '../../util/git';
import * as hostRules from '../../util/host-rules';
import * as githubHttp from '../../util/http/github';
Expand Down Expand Up @@ -246,6 +245,7 @@ export async function initRepo({
// This happens if we don't have Administrator read access, it is not a critical error
logger.debug('Could not find allowed merge methods for repo');
}
config.autoMergeAllowed = repo.autoMergeAllowed;
} catch (err) /* istanbul ignore next */ {
logger.debug({ err }, 'Caught initRepo error');
if (
Expand Down Expand Up @@ -1386,59 +1386,37 @@ async function tryPrAutomerge(
prNumber: number,
prNodeId: string,
platformOptions: PlatformPrOptions
): Promise<boolean> {
): Promise<void> {
if (!platformOptions?.usePlatformAutomerge) {
return;
}

const repoCache = getCache();
const { lastPlatformAutomergeFailure } = repoCache;
if (lastPlatformAutomergeFailure) {
const lastFailedAt = DateTime.fromISO(lastPlatformAutomergeFailure);
const now = DateTime.local();
if (now < lastFailedAt.plus({ hours: 24 })) {
logger.debug(
{ prNumber },
'GitHub-native automerge: skipping attempt due to earlier failure'
);
return;
}
delete repoCache.lastPlatformAutomergeFailure;
if (!config.autoMergeAllowed) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should separate errors from disabled repositories and help us to focus on interesting failure cases.

logger.debug(
{ prNumber },
'GitHub-native automerge: not enabled in repo settings'
);
return;
}

try {
const mergeMethod = config.mergeMethod?.toUpperCase() || 'MERGE';
const variables = { pullRequestId: prNodeId, mergeMethod };
const queryOptions = { variables };

const { errors } = await githubApi.requestGraphql<GhAutomergeResponse>(
enableAutoMergeMutation,
queryOptions
);
if (errors) {
const disabledByPlatform = errors.find(
({ type, message }) =>
type === 'UNPROCESSABLE' &&
message ===
'Pull request is not in the correct state to enable auto-merge'
);

// istanbul ignore else
if (disabledByPlatform) {
logger.debug(
{ prNumber },
'GitHub automerge is not enabled for this repository'
);

const now = DateTime.local();
repoCache.lastPlatformAutomergeFailure = now.toISO();
} else {
logger.debug({ prNumber, errors }, 'GitHub automerge unknown error');
}
} else {
logger.debug('GitHub-native PR automerge enabled');
if (errors) {
logger.debug({ prNumber, errors }, 'GitHub-native automerge: fail');
return;
}

logger.debug({ prNumber }, 'GitHub-native automerge: success');
} catch (err) {
logger.warn({ prNumber, err }, 'GitHub automerge: HTTP request error');
logger.warn({ prNumber, err }, 'GitHub-native automerge: REST API error');
}
}

Expand Down
2 changes: 2 additions & 0 deletions lib/platform/github/types.ts
View file
Open in desktop
Expand Up @@ -75,6 +75,7 @@ export interface LocalRepoConfig {
productLinks: any;
ignorePrAuthor: boolean;
branchPrs: Pr[];
autoMergeAllowed: boolean;
}

export type BranchProtection = any;
Expand All @@ -84,6 +85,7 @@ export interface GhRepo {
isFork: boolean;
isArchived: boolean;
nameWithOwner: string;
autoMergeAllowed: boolean;
mergeCommitAllowed: boolean;
rebaseMergeAllowed: boolean;
squashMergeAllowed: boolean;
Expand Down