From d3158fdb496f5d91dac52bbb504a5f523dd63793 Mon Sep 17 00:00:00 2001 From: Michael Beaumont Date: Thu, 9 Jul 2020 16:56:02 +0200 Subject: [PATCH 1/5] Allow setting merge method through labels --- README.md | 3 +++ lib/merge.js | 30 ++++++++++++++++++++++++++++-- test/merge.test.js | 33 +++++++++++++++++++++++++++++++-- 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 85988fbf..159e0389 100644 --- a/README.md +++ b/README.md @@ -96,6 +96,9 @@ The following merge options are supported: or [`squash`](https://help.github.com/en/articles/about-pull-request-merges#squash-and-merge-your-pull-request-commits) (squash all commits into a single commit). The default option is `merge`. + Additionally, labeling a PR with any required label plus the suffix + `.merge`, `.rebase`, `.squash` sets the merge method for that PR. + - `MERGE_COMMIT_MESSAGE`: The commit message to use when merging the pull request into the base branch. Possible values are `automatic` (use GitHub's default message), `pull-request-title` (use the pull request's title), diff --git a/lib/merge.js b/lib/merge.js index 6404b1dd..19bb2767 100644 --- a/lib/merge.js +++ b/lib/merge.js @@ -17,10 +17,11 @@ async function merge(context, pullRequest) { const { octokit, config: { - mergeMethod, + mergeMethod: defaultMergeMethod, mergeCommitMessage, mergeCommitMessageRegex, mergeFilterAuthor, + mergeLabels, mergeRemoveLabels, mergeRetries, mergeRetrySleep @@ -55,6 +56,11 @@ async function merge(context, pullRequest) { } const commitMessage = getCommitMessage(mergeCommitMessage, pullRequest); + const mergeMethod = getMergeMethod( + defaultMergeMethod, + mergeLabels, + pullRequest + ); const merged = await tryMerge( octokit, pullRequest, @@ -175,7 +181,11 @@ function skipPullRequest(context, pullRequest) { } for (const required of mergeLabels.required) { - if (!labels.includes(required)) { + if ( + !["", ".merge", ".rebase", ".squash"].some(suffix => + labels.includes(required + suffix) + ) + ) { logger.info("Skipping PR merge, required label missing:", required); skip = true; } @@ -255,6 +265,22 @@ function tryMerge( ); } +function getMergeMethod(defaultMergeMethod, mergeLabels, pullRequest) { + for (const l of pullRequest.labels) { + for (const ml of mergeLabels.required) { + switch (l.name) { + case `${ml}.merge`: + return "merge"; + case `${ml}.rebase`: + return "rebase"; + case `${ml}.squash`: + return "squash"; + } + } + } + return defaultMergeMethod; +} + function getCommitMessage(mergeCommitMessage, pullRequest) { if (mergeCommitMessage === "automatic") { return undefined; diff --git a/test/merge.test.js b/test/merge.test.js index a32abef7..562c74d9 100644 --- a/test/merge.test.js +++ b/test/merge.test.js @@ -2,12 +2,13 @@ const { merge } = require("../lib/merge"); const { createConfig } = require("../lib/common"); const { pullRequest } = require("./common"); -let octokit; +let octokit, mergeMethod; beforeEach(() => { + mergeMethod = undefined; octokit = { pulls: { - merge: jest.fn() + merge: jest.fn(({ merge_method }) => (mergeMethod = merge_method)) } }; }); @@ -109,3 +110,31 @@ test("MERGE_FILTER_AUTHOR when set but do not match current author should not me // WHEN expect(await merge({ config, octokit }, pr)).toEqual(false); }); + +test("Merge method can be set by env variable", async () => { + // GIVEN + const pr = pullRequest(); + + const config = createConfig({ + MERGE_METHOD: "rebase" + }); + + // WHEN + expect(await merge({ config, octokit }, pr)).toEqual(true); + expect(mergeMethod).toEqual("rebase"); +}); + +test("Merge method can be set by any required label", async () => { + // GIVEN + const pr = pullRequest(); + pr.labels = [{ name: "automerge.squash" }, { name: "reallyautomerge" }]; + + const config = createConfig({ + MERGE_METHOD: "merge", + MERGE_LABELS: "automerge,reallyautomerge" + }); + + // WHEN + expect(await merge({ config, octokit }, pr)).toEqual(true); + expect(mergeMethod).toEqual("squash"); +}); From 707aa0ee374fa78b0f6f7b04ba4e68e30a96ecda Mon Sep 17 00:00:00 2001 From: Michael Beaumont Date: Tue, 14 Jul 2020 11:27:05 +0200 Subject: [PATCH 2/5] Put merge method labels behind flag --- README.md | 7 +++++-- lib/common.js | 2 ++ lib/merge.js | 21 ++++++++++----------- test/common.test.js | 1 + test/merge.test.js | 1 + 5 files changed, 19 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 159e0389..902905fa 100644 --- a/README.md +++ b/README.md @@ -96,8 +96,11 @@ The following merge options are supported: or [`squash`](https://help.github.com/en/articles/about-pull-request-merges#squash-and-merge-your-pull-request-commits) (squash all commits into a single commit). The default option is `merge`. - Additionally, labeling a PR with any required label plus the suffix - `.merge`, `.rebase`, `.squash` sets the merge method for that PR. +- `MERGE_USE_METHOD_LABELS`: If `"true"`, labeling a PR with any required label plus the suffix + `.merge`, `.rebase`, `.squash` sets the merge method for that PR. Otherwise + `MERGE_METHOD` is used. + + Note that a required label with a merge method suffix also satisfies the required label check. - `MERGE_COMMIT_MESSAGE`: The commit message to use when merging the pull request into the base branch. Possible values are `automatic` (use GitHub's diff --git a/lib/common.js b/lib/common.js index 8b4ee230..0af48d29 100644 --- a/lib/common.js +++ b/lib/common.js @@ -97,6 +97,7 @@ function createConfig(env = {}) { const mergeRetries = parsePositiveInt("MERGE_RETRIES", 6); const mergeRetrySleep = parsePositiveInt("MERGE_RETRY_SLEEP", 5000); const mergeDeleteBranch = env.MERGE_DELETE_BRANCH === "true"; + const mergeUseMethodLabels = env.MERGE_USE_METHOD_LABELS === "true"; const updateLabels = parseMergeLabels(env.UPDATE_LABELS, "automerge"); const updateMethod = env.UPDATE_METHOD || "merge"; @@ -107,6 +108,7 @@ function createConfig(env = {}) { mergeLabels, mergeRemoveLabels, mergeMethod, + mergeUseMethodLabels, mergeForks, mergeCommitMessage, mergeCommitMessageRegex, diff --git a/lib/merge.js b/lib/merge.js index 19bb2767..c002232e 100644 --- a/lib/merge.js +++ b/lib/merge.js @@ -18,6 +18,7 @@ async function merge(context, pullRequest) { octokit, config: { mergeMethod: defaultMergeMethod, + mergeUseMethodLabels, mergeCommitMessage, mergeCommitMessageRegex, mergeFilterAuthor, @@ -56,11 +57,9 @@ async function merge(context, pullRequest) { } const commitMessage = getCommitMessage(mergeCommitMessage, pullRequest); - const mergeMethod = getMergeMethod( - defaultMergeMethod, - mergeLabels, - pullRequest - ); + const mergeMethod = mergeUseMethodLabels + ? getMergeMethod(defaultMergeMethod, mergeLabels, pullRequest) + : defaultMergeMethod; const merged = await tryMerge( octokit, pullRequest, @@ -149,7 +148,7 @@ async function deleteBranch(octokit, pullRequest) { function skipPullRequest(context, pullRequest) { const { - config: { mergeForks, mergeLabels } + config: { mergeForks, mergeLabels, mergeUseMethodLabels } } = context; let skip = false; @@ -171,6 +170,10 @@ function skipPullRequest(context, pullRequest) { } } + const allowedSuffixes = mergeUseMethodLabels + ? ["", ".merge", ".rebase", ".squash"] + : [""]; + const labels = pullRequest.labels.map(label => label.name); for (const label of pullRequest.labels) { @@ -181,11 +184,7 @@ function skipPullRequest(context, pullRequest) { } for (const required of mergeLabels.required) { - if ( - !["", ".merge", ".rebase", ".squash"].some(suffix => - labels.includes(required + suffix) - ) - ) { + if (!allowedSuffixes.some(suffix => labels.includes(required + suffix))) { logger.info("Skipping PR merge, required label missing:", required); skip = true; } diff --git a/test/common.test.js b/test/common.test.js index 8ff04f41..482be78d 100644 --- a/test/common.test.js +++ b/test/common.test.js @@ -13,6 +13,7 @@ test("createConfig", () => { blocking: [], required: [] }, + mergeUseMethodLabels: false, mergeForks: true, mergeCommitMessage: "automatic", mergeCommitMessageRegex: "", diff --git a/test/merge.test.js b/test/merge.test.js index 562c74d9..f4478c4c 100644 --- a/test/merge.test.js +++ b/test/merge.test.js @@ -130,6 +130,7 @@ test("Merge method can be set by any required label", async () => { pr.labels = [{ name: "automerge.squash" }, { name: "reallyautomerge" }]; const config = createConfig({ + MERGE_USE_METHOD_LABELS: "true", MERGE_METHOD: "merge", MERGE_LABELS: "automerge,reallyautomerge" }); From 855755754c3eece2c07208502251f28e77687f3b Mon Sep 17 00:00:00 2001 From: Michael Beaumont Date: Wed, 22 Jul 2020 18:01:49 +0200 Subject: [PATCH 3/5] Add specifying merge method labels --- README.md | 7 +++---- lib/common.js | 17 +++++++++++++++-- lib/merge.js | 40 +++++++++++++++++++--------------------- test/common.test.js | 2 +- test/merge.test.js | 8 ++++---- 5 files changed, 42 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 902905fa..c3bcf756 100644 --- a/README.md +++ b/README.md @@ -96,11 +96,10 @@ The following merge options are supported: or [`squash`](https://help.github.com/en/articles/about-pull-request-merges#squash-and-merge-your-pull-request-commits) (squash all commits into a single commit). The default option is `merge`. -- `MERGE_USE_METHOD_LABELS`: If `"true"`, labeling a PR with any required label plus the suffix - `.merge`, `.rebase`, `.squash` sets the merge method for that PR. Otherwise - `MERGE_METHOD` is used. +- `MERGE_METHOD_LABELS`: Set to allow labels to determine the merge method. + For example, `automerge=merge,autosquash=squash,autorebase=rebase`. - Note that a required label with a merge method suffix also satisfies the required label check. + If no such label is present, use `MERGE_METHOD`. - `MERGE_COMMIT_MESSAGE`: The commit message to use when merging the pull request into the base branch. Possible values are `automatic` (use GitHub's diff --git a/lib/common.js b/lib/common.js index 0af48d29..5d24761c 100644 --- a/lib/common.js +++ b/lib/common.js @@ -69,6 +69,19 @@ function createConfig(env = {}) { }; } + function parseMergeMethodLabels(str) { + if (!str) { + return []; + } + return str.split(",").map(lm => { + const [label, method] = lm.split("="); + if (!label || !method) { + throw new Error(`Couldn't parse ${lm} as label=method`); + } + return { label, method }; + }); + } + function parseMergeRemoveLabels(str, defaultValue) { return (str == null ? defaultValue : str).split(",").map(s => s.trim()); } @@ -97,7 +110,7 @@ function createConfig(env = {}) { const mergeRetries = parsePositiveInt("MERGE_RETRIES", 6); const mergeRetrySleep = parsePositiveInt("MERGE_RETRY_SLEEP", 5000); const mergeDeleteBranch = env.MERGE_DELETE_BRANCH === "true"; - const mergeUseMethodLabels = env.MERGE_USE_METHOD_LABELS === "true"; + const mergeMethodLabels = parseMergeMethodLabels(env.MERGE_METHOD_LABELS); const updateLabels = parseMergeLabels(env.UPDATE_LABELS, "automerge"); const updateMethod = env.UPDATE_METHOD || "merge"; @@ -108,7 +121,7 @@ function createConfig(env = {}) { mergeLabels, mergeRemoveLabels, mergeMethod, - mergeUseMethodLabels, + mergeMethodLabels, mergeForks, mergeCommitMessage, mergeCommitMessageRegex, diff --git a/lib/merge.js b/lib/merge.js index c002232e..442dc720 100644 --- a/lib/merge.js +++ b/lib/merge.js @@ -18,7 +18,7 @@ async function merge(context, pullRequest) { octokit, config: { mergeMethod: defaultMergeMethod, - mergeUseMethodLabels, + mergeMethodLabels, mergeCommitMessage, mergeCommitMessageRegex, mergeFilterAuthor, @@ -57,9 +57,7 @@ async function merge(context, pullRequest) { } const commitMessage = getCommitMessage(mergeCommitMessage, pullRequest); - const mergeMethod = mergeUseMethodLabels - ? getMergeMethod(defaultMergeMethod, mergeLabels, pullRequest) - : defaultMergeMethod; + const mergeMethod = getMergeMethod(defaultMergeMethod, mergeMethodLabels, pullRequest); const merged = await tryMerge( octokit, pullRequest, @@ -148,7 +146,7 @@ async function deleteBranch(octokit, pullRequest) { function skipPullRequest(context, pullRequest) { const { - config: { mergeForks, mergeLabels, mergeUseMethodLabels } + config: { mergeForks, mergeLabels } } = context; let skip = false; @@ -170,10 +168,6 @@ function skipPullRequest(context, pullRequest) { } } - const allowedSuffixes = mergeUseMethodLabels - ? ["", ".merge", ".rebase", ".squash"] - : [""]; - const labels = pullRequest.labels.map(label => label.name); for (const label of pullRequest.labels) { @@ -184,7 +178,7 @@ function skipPullRequest(context, pullRequest) { } for (const required of mergeLabels.required) { - if (!allowedSuffixes.some(suffix => labels.includes(required + suffix))) { + if (!labels.includes(required)) { logger.info("Skipping PR merge, required label missing:", required); skip = true; } @@ -264,18 +258,22 @@ function tryMerge( ); } -function getMergeMethod(defaultMergeMethod, mergeLabels, pullRequest) { - for (const l of pullRequest.labels) { - for (const ml of mergeLabels.required) { - switch (l.name) { - case `${ml}.merge`: - return "merge"; - case `${ml}.rebase`: - return "rebase"; - case `${ml}.squash`: - return "squash"; - } +function getMergeMethod(defaultMergeMethod, mergeMethodLabels, pullRequest) { + const foundMergeMethodLabels = pullRequest.labels.flatMap(l => + mergeMethodLabels.filter(ml => ml.label === l.name) + ); + if (foundMergeMethodLabels.length > 0) { + const first = foundMergeMethodLabels[0]; + if (foundMergeMethodLabels.length > 1) { + logger.info( + `Discovered multiple merge method labels, will merge with method ${first.method}` + ); + } else { + logger.info( + `Discovered ${first.label}, will merge with method ${first.method}` + ); } + return first.method; } return defaultMergeMethod; } diff --git a/test/common.test.js b/test/common.test.js index 482be78d..b54b6293 100644 --- a/test/common.test.js +++ b/test/common.test.js @@ -13,7 +13,7 @@ test("createConfig", () => { blocking: [], required: [] }, - mergeUseMethodLabels: false, + mergeMethodLabels: [], mergeForks: true, mergeCommitMessage: "automatic", mergeCommitMessageRegex: "", diff --git a/test/merge.test.js b/test/merge.test.js index f4478c4c..c1380fbc 100644 --- a/test/merge.test.js +++ b/test/merge.test.js @@ -124,15 +124,15 @@ test("Merge method can be set by env variable", async () => { expect(mergeMethod).toEqual("rebase"); }); -test("Merge method can be set by any required label", async () => { +test("Merge method can be set by a merge method label", async () => { // GIVEN const pr = pullRequest(); - pr.labels = [{ name: "automerge.squash" }, { name: "reallyautomerge" }]; + pr.labels = [{ name: "autosquash" }, { name: "reallyautomerge" }]; const config = createConfig({ - MERGE_USE_METHOD_LABELS: "true", + MERGE_METHOD_LABELS: "automerge=merge,autosquash=squash,autorebase=rebase", MERGE_METHOD: "merge", - MERGE_LABELS: "automerge,reallyautomerge" + MERGE_LABELS: "reallyautomerge" }); // WHEN From ee67a979209c7881d2e6cbbd0e702756c80392dd Mon Sep 17 00:00:00 2001 From: Michael Beaumont Date: Wed, 22 Jul 2020 18:23:00 +0200 Subject: [PATCH 4/5] Add MERGE_METHOD_LABEL_REQUIRED option --- README.md | 3 +++ lib/common.js | 2 ++ lib/merge.js | 22 +++++++++++++++++--- test/common.test.js | 1 + test/merge.test.js | 50 +++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 75 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index c3bcf756..f7f0a7ce 100644 --- a/README.md +++ b/README.md @@ -101,6 +101,9 @@ The following merge options are supported: If no such label is present, use `MERGE_METHOD`. +- `MERGE_METHOD_LABEL_REQUIRED`: Set to `"true"` to require one of the + `MERGE_METHOD_LABELS` to be set. + - `MERGE_COMMIT_MESSAGE`: The commit message to use when merging the pull request into the base branch. Possible values are `automatic` (use GitHub's default message), `pull-request-title` (use the pull request's title), diff --git a/lib/common.js b/lib/common.js index 5d24761c..f5b2cecb 100644 --- a/lib/common.js +++ b/lib/common.js @@ -111,6 +111,7 @@ function createConfig(env = {}) { const mergeRetrySleep = parsePositiveInt("MERGE_RETRY_SLEEP", 5000); const mergeDeleteBranch = env.MERGE_DELETE_BRANCH === "true"; const mergeMethodLabels = parseMergeMethodLabels(env.MERGE_METHOD_LABELS); + const mergeMethodLabelRequired = env.MERGE_METHOD_LABEL_REQUIRED === "true"; const updateLabels = parseMergeLabels(env.UPDATE_LABELS, "automerge"); const updateMethod = env.UPDATE_METHOD || "merge"; @@ -122,6 +123,7 @@ function createConfig(env = {}) { mergeRemoveLabels, mergeMethod, mergeMethodLabels, + mergeMethodLabelRequired, mergeForks, mergeCommitMessage, mergeCommitMessageRegex, diff --git a/lib/merge.js b/lib/merge.js index 442dc720..d3713738 100644 --- a/lib/merge.js +++ b/lib/merge.js @@ -22,7 +22,6 @@ async function merge(context, pullRequest) { mergeCommitMessage, mergeCommitMessageRegex, mergeFilterAuthor, - mergeLabels, mergeRemoveLabels, mergeRetries, mergeRetrySleep @@ -57,7 +56,11 @@ async function merge(context, pullRequest) { } const commitMessage = getCommitMessage(mergeCommitMessage, pullRequest); - const mergeMethod = getMergeMethod(defaultMergeMethod, mergeMethodLabels, pullRequest); + const mergeMethod = getMergeMethod( + defaultMergeMethod, + mergeMethodLabels, + pullRequest + ); const merged = await tryMerge( octokit, pullRequest, @@ -146,7 +149,12 @@ async function deleteBranch(octokit, pullRequest) { function skipPullRequest(context, pullRequest) { const { - config: { mergeForks, mergeLabels } + config: { + mergeForks, + mergeLabels, + mergeMethodLabelRequired, + mergeMethodLabels + } } = context; let skip = false; @@ -184,6 +192,14 @@ function skipPullRequest(context, pullRequest) { } } + const numberMethodLabelsFound = mergeMethodLabels + .map(lm => labels.includes(lm.label)) + .filter(x => x).length; + if (mergeMethodLabelRequired && numberMethodLabelsFound === 0) { + logger.info("Skipping PR merge, required merge method label missing"); + skip = true; + } + return skip; } diff --git a/test/common.test.js b/test/common.test.js index b54b6293..aac4bf7d 100644 --- a/test/common.test.js +++ b/test/common.test.js @@ -14,6 +14,7 @@ test("createConfig", () => { required: [] }, mergeMethodLabels: [], + mergeMethodLabelRequired: false, mergeForks: true, mergeCommitMessage: "automatic", mergeCommitMessageRegex: "", diff --git a/test/merge.test.js b/test/merge.test.js index c1380fbc..ea6ae38c 100644 --- a/test/merge.test.js +++ b/test/merge.test.js @@ -139,3 +139,53 @@ test("Merge method can be set by a merge method label", async () => { expect(await merge({ config, octokit }, pr)).toEqual(true); expect(mergeMethod).toEqual("squash"); }); + +test("Merge method can be required", async () => { + // GIVEN + const pr = pullRequest(); + pr.labels = [{ name: "autosquash" }]; + + const config = createConfig({ + MERGE_METHOD_LABELS: "automerge=merge,autosquash=squash,autorebase=rebase", + MERGE_METHOD_LABEL_REQUIRED: "true", + MERGE_METHOD: "merge", + MERGE_LABELS: "" + }); + + // WHEN + expect(await merge({ config, octokit }, pr)).toEqual(true); + expect(mergeMethod).toEqual("squash"); +}); + +test("Missing require merge method skips PR", async () => { + // GIVEN + const pr = pullRequest(); + pr.labels = [{ name: "mergeme" }]; + + const config = createConfig({ + MERGE_METHOD_LABELS: "automerge=merge,autosquash=squash,autorebase=rebase", + MERGE_METHOD_LABEL_REQUIRED: "true", + MERGE_METHOD: "merge", + MERGE_LABELS: "mergeme" + }); + + // WHEN + expect(await merge({ config, octokit }, pr)).toEqual(false); +}); + +test("Merge method doesn't have to be required", async () => { + // GIVEN + const pr = pullRequest(); + pr.labels = [{ name: "mergeme" }]; + + const config = createConfig({ + MERGE_METHOD_LABELS: "automerge=merge,autosquash=squash,autorebase=rebase", + MERGE_METHOD_LABEL_REQUIRED: "false", + MERGE_METHOD: "merge", + MERGE_LABELS: "mergeme" + }); + + // WHEN + expect(await merge({ config, octokit }, pr)).toEqual(true); + expect(mergeMethod).toEqual("merge"); +}); From 13e7be504243c65e4f7f183c1c78096f1c4c80be Mon Sep 17 00:00:00 2001 From: Michael Beaumont Date: Mon, 27 Jul 2020 10:08:08 +0200 Subject: [PATCH 5/5] Review comments --- README.md | 8 +++++--- lib/common.js | 4 +++- lib/merge.js | 4 ++-- test/merge.test.js | 15 +++++++++++++++ 4 files changed, 25 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index f7f0a7ce..60d70cc4 100644 --- a/README.md +++ b/README.md @@ -96,10 +96,12 @@ The following merge options are supported: or [`squash`](https://help.github.com/en/articles/about-pull-request-merges#squash-and-merge-your-pull-request-commits) (squash all commits into a single commit). The default option is `merge`. -- `MERGE_METHOD_LABELS`: Set to allow labels to determine the merge method. - For example, `automerge=merge,autosquash=squash,autorebase=rebase`. +- `MERGE_METHOD_LABELS`: Set to allow labels to determine the merge method + (see `MERGE_METHOD` for possible values). + For example, `automerge=merge,autosquash=squash`. If no such label is present, + the method set by `MERGE_METHOD` will be used. - If no such label is present, use `MERGE_METHOD`. + The default value is `""`. - `MERGE_METHOD_LABEL_REQUIRED`: Set to `"true"` to require one of the `MERGE_METHOD_LABELS` to be set. diff --git a/lib/common.js b/lib/common.js index f5b2cecb..673abf40 100644 --- a/lib/common.js +++ b/lib/common.js @@ -76,7 +76,9 @@ function createConfig(env = {}) { return str.split(",").map(lm => { const [label, method] = lm.split("="); if (!label || !method) { - throw new Error(`Couldn't parse ${lm} as label=method`); + throw new Error( + `Couldn't parse "${lm}" as "