From f87d5e8b76c4499963cdc9b70d32f237cf76c6fe Mon Sep 17 00:00:00 2001
From: Rhys Arkins <rhys@arkins.net>
Date: Thu, 15 Apr 2021 16:30:02 +0200
Subject: [PATCH] refactor: complex PR automerge result (#9567)
---
lib/workers/branch/index.spec.ts | 12 ++--
lib/workers/branch/index.ts | 4 +-
.../pr/__snapshots__/automerge.spec.ts.snap | 56 ++++++++++++++---
lib/workers/pr/automerge.spec.ts | 8 +++
lib/workers/pr/automerge.ts | 61 +++++++++++++++----
5 files changed, 115 insertions(+), 26 deletions(-)
diff --git a/lib/workers/branch/index.spec.ts b/lib/workers/branch/index.spec.ts
index 28fa8400d3..d24742e00e 100644
--- a/lib/workers/branch/index.spec.ts
+++ b/lib/workers/branch/index.spec.ts
@@ -451,7 +451,7 @@ describe(getName(__filename), () => {
prResult: PrResult.Created,
pr: {},
} as EnsurePrResult);
- prAutomerge.checkAutoMerge.mockResolvedValueOnce(true);
+ prAutomerge.checkAutoMerge.mockResolvedValueOnce({ automerged: true });
commit.commitFilesToBranch.mockResolvedValueOnce(null);
await branchWorker.processBranch({ ...config, automerge: true });
expect(prWorker.ensurePr).toHaveBeenCalledTimes(1);
@@ -472,7 +472,7 @@ describe(getName(__filename), () => {
prResult: PrResult.Created,
pr: {},
} as EnsurePrResult);
- prAutomerge.checkAutoMerge.mockResolvedValueOnce(true);
+ prAutomerge.checkAutoMerge.mockResolvedValueOnce({ automerged: true });
commit.commitFilesToBranch.mockResolvedValueOnce(null);
await branchWorker.processBranch(config);
expect(platform.ensureComment).toHaveBeenCalledTimes(1);
@@ -493,7 +493,7 @@ describe(getName(__filename), () => {
prResult: PrResult.Created,
pr: {},
} as EnsurePrResult);
- prAutomerge.checkAutoMerge.mockResolvedValueOnce(true);
+ prAutomerge.checkAutoMerge.mockResolvedValueOnce({ automerged: true });
config.releaseTimestamp = '2018-04-26T05:15:51.877Z';
commit.commitFilesToBranch.mockResolvedValueOnce(null);
await branchWorker.processBranch(config);
@@ -515,7 +515,7 @@ describe(getName(__filename), () => {
prResult: PrResult.Created,
pr: {},
} as EnsurePrResult);
- prAutomerge.checkAutoMerge.mockResolvedValueOnce(true);
+ prAutomerge.checkAutoMerge.mockResolvedValueOnce({ automerged: true });
config.releaseTimestamp = new Date().toISOString();
commit.commitFilesToBranch.mockResolvedValueOnce(null);
await branchWorker.processBranch(config);
@@ -537,7 +537,7 @@ describe(getName(__filename), () => {
prResult: PrResult.Created,
pr: {},
} as EnsurePrResult);
- prAutomerge.checkAutoMerge.mockResolvedValueOnce(true);
+ prAutomerge.checkAutoMerge.mockResolvedValueOnce({ automerged: true });
config.releaseTimestamp = new Date().toISOString();
await expect(branchWorker.processBranch(config)).rejects.toThrow(
Error(MANAGER_LOCKFILE_ERROR)
@@ -558,7 +558,7 @@ describe(getName(__filename), () => {
prResult: PrResult.Created,
pr: {},
} as EnsurePrResult);
- prAutomerge.checkAutoMerge.mockResolvedValueOnce(true);
+ prAutomerge.checkAutoMerge.mockResolvedValueOnce({ automerged: true });
commit.commitFilesToBranch.mockResolvedValueOnce(null);
await branchWorker.processBranch(config);
expect(platform.ensureComment).toHaveBeenCalledTimes(1);
diff --git a/lib/workers/branch/index.ts b/lib/workers/branch/index.ts
index e84774dcbd..d2b2576b36 100644
--- a/lib/workers/branch/index.ts
+++ b/lib/workers/branch/index.ts
@@ -601,8 +601,8 @@ export async function processBranch(
}
} else if (config.automerge) {
logger.debug('PR is configured for automerge');
- const prAutomerged = await checkAutoMerge(pr, config);
- if (prAutomerged) {
+ const prAutomergeResult = await checkAutoMerge(pr, config);
+ if (prAutomergeResult?.automerged) {
return ProcessBranchResult.Automerged;
}
} else {
diff --git a/lib/workers/pr/__snapshots__/automerge.spec.ts.snap b/lib/workers/pr/__snapshots__/automerge.spec.ts.snap
index 424eda4b08..76721cbc38 100644
--- a/lib/workers/pr/__snapshots__/automerge.spec.ts.snap
+++ b/lib/workers/pr/__snapshots__/automerge.spec.ts.snap
@@ -1,15 +1,57 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
-exports[`workers/pr/automerge checkAutoMerge(pr, config) should automerge comment 1`] = `true`;
+exports[`workers/pr/automerge checkAutoMerge(pr, config) should automerge comment 1`] = `
+Object {
+ "automerged": true,
+ "branchRemoved": false,
+}
+`;
-exports[`workers/pr/automerge checkAutoMerge(pr, config) should automerge if enabled and pr is mergeable 1`] = `true`;
+exports[`workers/pr/automerge checkAutoMerge(pr, config) should automerge if enabled and pr is mergeable 1`] = `
+Object {
+ "automerged": true,
+ "branchRemoved": true,
+}
+`;
-exports[`workers/pr/automerge checkAutoMerge(pr, config) should not automerge if enabled and pr is mergeable but branch status is not success 1`] = `false`;
+exports[`workers/pr/automerge checkAutoMerge(pr, config) should indicate if automerge failed 1`] = `
+Object {
+ "automerged": false,
+ "prAutomergeBlockReason": "PlatformRejection",
+}
+`;
-exports[`workers/pr/automerge checkAutoMerge(pr, config) should not automerge if enabled and pr is mergeable but cannot rebase 1`] = `false`;
+exports[`workers/pr/automerge checkAutoMerge(pr, config) should not automerge if enabled and pr is mergeable but branch status is not success 1`] = `
+Object {
+ "automerged": false,
+ "prAutomergeBlockReason": "BranchNotGreen",
+}
+`;
-exports[`workers/pr/automerge checkAutoMerge(pr, config) should not automerge if enabled and pr is mergeable but unstable 1`] = `false`;
+exports[`workers/pr/automerge checkAutoMerge(pr, config) should not automerge if enabled and pr is mergeable but cannot rebase 1`] = `
+Object {
+ "automerged": false,
+ "prAutomergeBlockReason": "BranchModified",
+}
+`;
-exports[`workers/pr/automerge checkAutoMerge(pr, config) should not automerge if enabled and pr is unmergeable 1`] = `false`;
+exports[`workers/pr/automerge checkAutoMerge(pr, config) should not automerge if enabled and pr is mergeable but unstable 1`] = `
+Object {
+ "automerged": false,
+ "prAutomergeBlockReason": "PlatformNotReady",
+}
+`;
-exports[`workers/pr/automerge checkAutoMerge(pr, config) should remove previous automerge comment when rebasing 1`] = `true`;
+exports[`workers/pr/automerge checkAutoMerge(pr, config) should not automerge if enabled and pr is unmergeable 1`] = `
+Object {
+ "automerged": false,
+ "prAutomergeBlockReason": "Conflicted",
+}
+`;
+
+exports[`workers/pr/automerge checkAutoMerge(pr, config) should remove previous automerge comment when rebasing 1`] = `
+Object {
+ "automerged": true,
+ "branchRemoved": false,
+}
+`;
diff --git a/lib/workers/pr/automerge.spec.ts b/lib/workers/pr/automerge.spec.ts
index 3e4f2d72b1..1343997e39 100644
--- a/lib/workers/pr/automerge.spec.ts
+++ b/lib/workers/pr/automerge.spec.ts
@@ -32,6 +32,14 @@ describe(getName(__filename), () => {
expect(res).toMatchSnapshot();
expect(platform.mergePr).toHaveBeenCalledTimes(1);
});
+ it('should indicate if automerge failed', async () => {
+ config.automerge = true;
+ platform.getBranchStatus.mockResolvedValueOnce(BranchStatus.green);
+ platform.mergePr.mockResolvedValueOnce(false);
+ const res = await prAutomerge.checkAutoMerge(pr, config);
+ expect(res).toMatchSnapshot();
+ expect(platform.mergePr).toHaveBeenCalledTimes(1);
+ });
it('should automerge comment', async () => {
config.automerge = true;
config.automergeType = 'pr-comment';
diff --git a/lib/workers/pr/automerge.ts b/lib/workers/pr/automerge.ts
index 0dabe65dfe..d3d86a4ef0 100644
--- a/lib/workers/pr/automerge.ts
+++ b/lib/workers/pr/automerge.ts
@@ -5,10 +5,25 @@ import { BranchStatus } from '../../types';
import { deleteBranch, isBranchModified } from '../../util/git';
import { BranchConfig } from '../types';
+export enum PrAutomergeBlockReason {
+ BranchModified = 'BranchModified',
+ BranchNotGreen = 'BranchNotGreen',
+ Conflicted = 'Conflicted',
+ DryRun = 'DryRun',
+ PlatformNotReady = 'PlatformNotReady',
+ PlatformRejection = 'PlatformRejection',
+}
+
+export type AutomergePrResult = {
+ automerged: boolean;
+ branchRemoved?: boolean;
+ prAutomergeBlockReason?: PrAutomergeBlockReason;
+};
+
export async function checkAutoMerge(
pr: Pr,
config: BranchConfig
-): Promise<boolean> {
+): Promise<AutomergePrResult> {
logger.trace({ config }, 'checkAutoMerge');
const {
branchName,
@@ -20,15 +35,20 @@ export async function checkAutoMerge(
// Return if PR not ready for automerge
if (pr.isConflicted) {
logger.debug('PR is conflicted');
- logger.debug({ pr });
- return false;
+ return {
+ automerged: false,
+ prAutomergeBlockReason: PrAutomergeBlockReason.Conflicted,
+ };
}
if (requiredStatusChecks && pr.canMerge !== true) {
logger.debug(
{ canMergeReason: pr.canMergeReason },
'PR is not ready for merge'
);
- return false;
+ return {
+ automerged: false,
+ prAutomergeBlockReason: PrAutomergeBlockReason.PlatformNotReady,
+ };
}
const branchStatus = await platform.getBranchStatus(
branchName,
@@ -38,12 +58,18 @@ export async function checkAutoMerge(
logger.debug(
`PR is not ready for merge (branch status is ${branchStatus})`
);
- return false;
+ return {
+ automerged: false,
+ prAutomergeBlockReason: PrAutomergeBlockReason.BranchNotGreen,
+ };
}
// Check if it's been touched
if (await isBranchModified(branchName)) {
logger.debug('PR is ready for automerge but has been modified');
- return false;
+ return {
+ automerged: false,
+ prAutomergeBlockReason: PrAutomergeBlockReason.BranchModified,
+ };
}
if (automergeType === 'pr-comment') {
logger.debug(`Applying automerge comment: ${automergeComment}`);
@@ -52,7 +78,10 @@ export async function checkAutoMerge(
logger.info(
`DRY-RUN: Would add PR automerge comment to PR #${pr.number}`
);
- return false;
+ return {
+ automerged: false,
+ prAutomergeBlockReason: PrAutomergeBlockReason.DryRun,
+ };
}
if (rebaseRequested) {
await platform.ensureCommentRemoval({
@@ -60,27 +89,37 @@ export async function checkAutoMerge(
content: automergeComment,
});
}
- return platform.ensureComment({
+ await platform.ensureComment({
number: pr.number,
topic: null,
content: automergeComment,
});
+ return { automerged: true, branchRemoved: false };
}
// Let's merge this
- logger.debug(`Automerging #${pr.number}`);
// istanbul ignore if
if (getAdminConfig().dryRun) {
logger.info(`DRY-RUN: Would merge PR #${pr.number}`);
- return false;
+ return {
+ automerged: false,
+ prAutomergeBlockReason: PrAutomergeBlockReason.DryRun,
+ };
}
+ logger.debug(`Automerging #${pr.number}`);
const res = await platform.mergePr(pr.number, branchName);
if (res) {
logger.info({ pr: pr.number, prTitle: pr.title }, 'PR automerged');
+ let branchRemoved = false;
try {
await deleteBranch(branchName);
+ branchRemoved = true;
} catch (err) /* istanbul ignore next */ {
logger.warn({ branchName, err }, 'Branch auto-remove failed');
}
+ return { automerged: true, branchRemoved };
}
- return res;
+ return {
+ automerged: false,
+ prAutomergeBlockReason: PrAutomergeBlockReason.PlatformRejection,
+ };
}
--
GitLab