Don't retry merge on state dirty

bin/automerge.js

@@ -6,7 +6,7 @@ const fse = require("fs-extra");
 const { ArgumentParser } = require("argparse");
 const Octokit = require("@octokit/rest");
 
-const { ClientError, logger } = require("../lib/common");
+const { ClientError, logger, createConfig } = require("../lib/common");
 const { executeLocally, executeGitHubAction } = require("../lib/api");
 
 const pkg = require("../package.json");
@@ -59,27 +59,7 @@ async function main() {
     userAgent: "pascalgn/automerge-action"
   });
 
-  const mergeLabels = parseLabels(process.env.MERGE_LABELS, "automerge");
-  const mergeMethod = process.env.MERGE_METHOD || "merge";
-  const mergeForks = process.env.MERGE_FORKS !== "false";
-  const mergeCommitMessage = process.env.MERGE_COMMIT_MESSAGE || "automatic";
-  const mergeRetries = parsePositiveInt("MERGE_RETRIES", 6);
-  const mergeRetrySleep = parsePositiveInt("MERGE_RETRY_SLEEP", 10000);
-
-  const updateLabels = parseLabels(process.env.UPDATE_LABELS, "automerge");
-  const updateMethod = process.env.UPDATE_METHOD || "merge";
-
-  const config = {
-    mergeLabels,
-    mergeMethod,
-    mergeForks,
-    mergeCommitMessage,
-    mergeRetries,
-    mergeRetrySleep,
-    updateLabels,
-    updateMethod
-  };
-
+  const config = createConfig(process.env);
   logger.debug("Configuration:", config);
 
   const context = { token, octokit, config };
@@ -124,30 +104,6 @@ function env(name) {
   return val;
 }
 
-function parseLabels(str, defaultValue) {
-  const arr = (str == null ? defaultValue : str).split(",").map(s => s.trim());
-  return {
-    required: arr.filter(s => !s.startsWith("!") && s.length > 0),
-    blocking: arr
-      .filter(s => s.startsWith("!") && s.length > 1)
-      .map(s => s.substr(1))
-  };
-}
-
-function parsePositiveInt(name, defaultValue) {
-  const val = process.env[name];
-  if (val == null || val === "") {
-    return defaultValue;
-  } else {
-    const number = parseInt(val);
-    if (isNaN(number) || number < 0) {
-      throw new ClientError(`Not a positive integer: ${val}`);
-    } else {
-      return number;
-    }
-  }
-}
-
 if (require.main === module) {
   main().catch(e => {
     if (e instanceof ClientError) {

it/it.js

@@ -1,6 +1,7 @@
 const Octokit = require("@octokit/rest");
 
 const { executeLocally } = require("../lib/api");
+const { createConfig } = require("../lib/common");
 
 async function main() {
   require("dotenv").config();
@@ -12,26 +13,12 @@ async function main() {
     userAgent: "pascalgn/automerge-action-it"
   });
 
-  const mergeLabels = { required: ["it-merge"], blocking: [] };
-  const mergeMethod = "merge";
-  const mergeForks = false;
-  const mergeCommitMessage = "automatic";
-  const mergeRetries = 3;
-  const mergeRetrySleep = 1000;
-
-  const updateLabels = { required: ["it-update"], blocking: [] };
-  const updateMethod = "merge";
-
-  const config = {
-    mergeLabels,
-    mergeMethod,
-    mergeForks,
-    mergeCommitMessage,
-    mergeRetries,
-    mergeRetrySleep,
-    updateLabels,
-    updateMethod
-  };
+  const config = createConfig({
+    UPDATE_LABELS: "it-update",
+    MERGE_LABELS: "it-merge",
+    MERGE_RETRIES: "3",
+    MERGE_RETRY_SLEEP: "2000"
+  });
 
   const context = { token, octokit, config };
 

lib/common.js

@@ -55,6 +55,56 @@ function inspect(obj) {
   return util.inspect(obj, false, null, true);
 }
 
+function createConfig(env = {}) {
+  function parseLabels(str, defaultValue) {
+    const arr = (str == null ? defaultValue : str)
+      .split(",")
+      .map(s => s.trim());
+    return {
+      required: arr.filter(s => !s.startsWith("!") && s.length > 0),
+      blocking: arr
+        .filter(s => s.startsWith("!"))
+        .map(s => s.substr(1).trim())
+        .filter(s => s.length > 0)
+    };
+  }
+
+  function parsePositiveInt(name, defaultValue) {
+    const val = env[name];
+    if (val == null || val === "") {
+      return defaultValue;
+    } else {
+      const number = parseInt(val);
+      if (isNaN(number) || number < 0) {
+        throw new ClientError(`Not a positive integer: ${val}`);
+      } else {
+        return number;
+      }
+    }
+  }
+
+  const mergeLabels = parseLabels(env.MERGE_LABELS, "automerge");
+  const mergeMethod = env.MERGE_METHOD || "merge";
+  const mergeForks = env.MERGE_FORKS !== "false";
+  const mergeCommitMessage = env.MERGE_COMMIT_MESSAGE || "automatic";
+  const mergeRetries = parsePositiveInt("MERGE_RETRIES", 6);
+  const mergeRetrySleep = parsePositiveInt("MERGE_RETRY_SLEEP", 10000);
+
+  const updateLabels = parseLabels(env.UPDATE_LABELS, "automerge");
+  const updateMethod = env.UPDATE_METHOD || "merge";
+
+  return {
+    mergeLabels,
+    mergeMethod,
+    mergeForks,
+    mergeCommitMessage,
+    mergeRetries,
+    mergeRetrySleep,
+    updateLabels,
+    updateMethod
+  };
+}
+
 function tmpdir(callback) {
   async function handle(path) {
     try {
@@ -75,8 +125,13 @@ function tmpdir(callback) {
 }
 
 async function retry(retries, sleep, doInitial, doRetry, doFailed) {
-  if (await doInitial()) {
+  const initialResult = await doInitial();
+  if (initialResult === "success") {
     return true;
+  } else if (initialResult === "failure") {
+    return false;
+  } else if (initialResult !== "retry") {
+    throw new Error(`invalid return value: ${initialResult}`);
   }
 
   for (let run = 1; run <= retries; run++) {
@@ -87,8 +142,13 @@ async function retry(retries, sleep, doInitial, doRetry, doFailed) {
       await doSleep(sleep);
     }
 
-    if (await doRetry()) {
+    const retryResult = await doRetry();
+    if (retryResult === "success") {
       return true;
+    } else if (retryResult === "failure") {
+      return false;
+    } else if (retryResult !== "retry") {
+      throw new Error(`invalid return value: ${initialResult}`);
     }
   }
 
@@ -104,6 +164,7 @@ module.exports = {
   ClientError,
   TimeoutError,
   logger,
+  createConfig,
   tmpdir,
   inspect,
   retry

lib/merge.js

@@ -107,13 +107,13 @@ function checkReady(pullRequest) {
   const { mergeable_state } = pullRequest;
   if (mergeable_state == null || MAYBE_READY.includes(mergeable_state)) {
     logger.info("PR is probably ready: mergeable_state:", mergeable_state);
-    return true;
+    return "success";
   } else if (NOT_READY.includes(mergeable_state)) {
     logger.info("PR not ready: mergeable_state:", mergeable_state);
-    return false;
+    return "failure";
   } else {
     logger.info("Current PR status: mergeable_state:", mergeable_state);
-    return false;
+    return "retry";
   }
 }
 
@@ -180,10 +180,10 @@ async function mergePullRequest(
       sha: head,
       merge_method: mergeMethod
     });
-    return true;
+    return "success";
   } catch (e) {
     logger.info("Failed to merge PR:", e.message);
-    return false;
+    return "retry";
   }
 }
 

test/common.test.js

@@ -0,0 +1,26 @@
+const { createConfig } = require("../lib/common");
+
+test("createConfig", () => {
+  const config = createConfig({
+    UPDATE_LABELS: " required1,! block1, ! ,required2, !block2 ",
+    MERGE_LABELS: "",
+    MERGE_RETRIES: "3"
+  });
+  const expected = {
+    mergeMethod: "merge",
+    mergeLabels: {
+      blocking: [],
+      required: []
+    },
+    mergeForks: true,
+    mergeCommitMessage: "automatic",
+    mergeRetries: 3,
+    mergeRetrySleep: 10000,
+    updateMethod: "merge",
+    updateLabels: {
+      blocking: ["block1", "block2"],
+      required: ["required1", "required2"]
+    }
+  };
+  expect(config).toEqual(expected);
+});

test/update.test.js

@@ -1,7 +1,9 @@
 const { update } = require("../lib/update");
+const { createConfig } = require("../lib/common");
 const { pullRequest } = require("./common");
 
-test("update will only run when a label is set", async () => {
+test("update will only run when the label matches", async () => {
   const pr = pullRequest();
-  expect(await update({ config: { updateLabel: "" } }, pr)).toEqual(false);
+  const config = createConfig({ UPDATE_LABELS: "none" });
+  expect(await update({ config }, pr)).toEqual(false);
 });