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

Only auto-merge after creating and updating PRs #56

Merged
merged 3 commits into from
Nov 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
96 changes: 35 additions & 61 deletions __snapshots__/github.ts.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

132 changes: 60 additions & 72 deletions src/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,6 @@ export type MergeMethod = 'merge' | 'squash' | 'rebase';
interface CreatePullRequestOptions {
fork?: boolean;
draft?: boolean;
reviewers?: string[];
autoMerge?: {
mergeMethod: MergeMethod;
};
/**
* If the number of an existing pull request is passed, its HEAD branch and attributes (title, labels, etc) will be
* updated instead of creating a new pull request.
Expand All @@ -238,10 +234,6 @@ interface CreatePullRequestOptions {
interface UpdatePullRequestOptions {
signoffUser?: string;
fork?: boolean;
reviewers?: string[];
autoMerge?: {
mergeMethod: MergeMethod;
};
pullRequestOverflowHandler?: PullRequestOverflowHandler;
}

Expand Down Expand Up @@ -1338,40 +1330,6 @@ export class GitHub {
pullRequest.labels
);

// attempt to enable auto-merge
let directlyMerged = false;
if (options?.autoMerge) {
try {
const result = await this.enablePullRequestAutoMerge(
pullRequestNumber,
options.autoMerge.mergeMethod
);
directlyMerged = result === 'direct-merged';
} catch (e: unknown) {
this.logger.error(
isOctokitGraphqlResponseError(e) ? e.errors || [] : (e as {}),
'Failed to enable auto merge. Continuing.'
);
}
}

// assign reviewers
if (!directlyMerged && options?.reviewers) {
try {
await this.octokit.pulls.requestReviewers({
owner: this.repository.owner,
repo: this.repository.repo,
pull_number: pullRequestNumber,
reviewers: options.reviewers,
});
} catch (error: unknown) {
this.logger.error(
isOctokitRequestError(error) ? error.message : (error as {}),
'Failed to add reviewers. Continuing.'
);
}
}

return await this.getPullRequest(pullRequestNumber);
}
);
Expand Down Expand Up @@ -1452,9 +1410,7 @@ export class GitHub {
releasePullRequest.updates,
{
fork: options?.fork,
reviewers: options?.reviewers,
existingPrNumber: number,
autoMerge: options?.autoMerge,
}
);

Expand Down Expand Up @@ -2399,42 +2355,74 @@ export class GitHub {
this.logger.debug({response}, 'mutatePullrequestEnableAutoMerge');
}

private async enablePullRequestAutoMerge(
async enablePullRequestAutoMerge(
pullRequestNumber: number,
mergeMethod: MergeMethod
): Promise<'auto-merged' | 'direct-merged'> {
this.logger.debug('Enable PR auto-merge');
const prId = await this.queryPullRequestId(pullRequestNumber);
if (!prId) {
throw new Error(`No id found for pull request ${pullRequestNumber}`);
}
): Promise<'auto-merged' | 'direct-merged' | 'none'> {
try {
await this.mutatePullRequestEnableAutoMerge(prId, mergeMethod);
return 'auto-merged';
this.logger.debug('Enable PR auto-merge');
const prId = await this.queryPullRequestId(pullRequestNumber);
if (!prId) {
throw new Error(`No id found for pull request ${pullRequestNumber}`);
}
try {
await this.mutatePullRequestEnableAutoMerge(prId, mergeMethod);
return 'auto-merged';
} catch (e: unknown) {
if (
isOctokitGraphqlResponseError(e) &&
(e.errors || []).find(
err =>
err.type === 'UNPROCESSABLE' &&
(err.message.includes('Pull request is in clean status') ||
err.message.includes(
'Protected branch rules not configured for this branch'
))
)
) {
this.logger.debug(
'PR can be merged directly, do it instead of via GitHub auto-merge'
);
await this.octokit.pulls.merge({
owner: this.repository.owner,
repo: this.repository.repo,
pull_number: pullRequestNumber,
merge_method: mergeMethod,
});
return 'direct-merged';
} else {
throw e;
}
}
} catch (e: unknown) {
if (
isOctokitGraphqlResponseError(e) &&
(e.errors || []).find(
err =>
err.type === 'UNPROCESSABLE' &&
(err.message.includes('Pull request is in clean status') ||
err.message.includes(
'Protected branch rules not configured for this branch'
))
)
) {
this.logger.debug(
'PR can be merged directly, do it instead of via GitHub auto-merge'
);
await this.octokit.pulls.merge({
this.logger.error(
isOctokitGraphqlResponseError(e) ? e.errors || [] : (e as {}),
'Failed to enable auto merge. Continuing.'
);
return 'none';
}
}

async addPullRequestReviewers({
pullRequestNumber,
reviewers,
}: {
pullRequestNumber: number;
reviewers: string[];
}) {
if (reviewers) {
try {
await this.octokit.pulls.requestReviewers({
owner: this.repository.owner,
repo: this.repository.repo,
pull_number: pullRequestNumber,
merge_method: mergeMethod,
reviewers,
});
return 'direct-merged';
} else {
throw e;
} catch (error: unknown) {
this.logger.error(
isOctokitRequestError(error) ? error.message : (error as {}),
'Failed to add reviewers. Continuing.'
);
}
}
}
Expand Down
57 changes: 52 additions & 5 deletions src/manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1099,11 +1099,26 @@ export class Manifest {
{
fork: this.fork,
draft: pullRequest.draft,
reviewers: this.reviewers,
autoMerge: this.pullRequestAutoMergeOption(pullRequest),
}
);

let directlyMerged = false;
const autoMerge = this.pullRequestAutoMergeOption(pullRequest);
if (autoMerge) {
const result = await this.github.enablePullRequestAutoMerge(
newPullRequest.number,
autoMerge.mergeMethod
);
directlyMerged = result === 'direct-merged';
}

if (!directlyMerged) {
this.github.addPullRequestReviewers({
pullRequestNumber: newPullRequest.number,
reviewers: this.reviewers,
});
}

return newPullRequest;
}

Expand All @@ -1130,13 +1145,28 @@ export class Manifest {
this.changesBranch,
{
fork: this.fork,
reviewers: this.reviewers,
signoffUser: this.signoffUser,
pullRequestOverflowHandler: this.pullRequestOverflowHandler,
autoMerge: this.pullRequestAutoMergeOption(pullRequest),
}
);

let directlyMerged = false;
const autoMerge = this.pullRequestAutoMergeOption(pullRequest);
if (autoMerge) {
const result = await this.github.enablePullRequestAutoMerge(
updatedPullRequest.number,
autoMerge.mergeMethod
);
directlyMerged = result === 'direct-merged';
}

if (!directlyMerged) {
this.github.addPullRequestReviewers({
pullRequestNumber: updatedPullRequest.number,
reviewers: this.reviewers,
});
}

return updatedPullRequest;
}

Expand All @@ -1161,11 +1191,28 @@ export class Manifest {
fork: this.fork,
signoffUser: this.signoffUser,
pullRequestOverflowHandler: this.pullRequestOverflowHandler,
autoMerge: this.pullRequestAutoMergeOption(pullRequest),
}
);
// 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(
updatedPullRequest.number,
autoMerge.mergeMethod
);
directlyMerged = result === 'direct-merged';
}

if (!directlyMerged) {
this.github.addPullRequestReviewers({
pullRequestNumber: updatedPullRequest.number,
reviewers: this.reviewers,
});
}

return updatedPullRequest;
}

Expand Down
Loading