Merge pull request #158 from wilzbach/improve-merge

Avoid any action on 'mergeable: null'

Author: wilzbach

.travis.yml

@@ -24,5 +24,6 @@ matrix:
       after_success:
         - bash <(curl -s https://codecov.io/bash)
   allow_failures:
+   - d: dmd
    - d: dmd-nightly
    - d: dmd-beta

source/dlangbot/app.d

@@ -139,9 +139,8 @@ void githubHook(HTTPServerRequest req, HTTPServerResponse res)
 
 //==============================================================================
 
-void cronDaily(string[] repositories, bool simulate)
+void cronDaily(string[] repositories, CronConfig config)
 {
-    CronConfig config = {simulate: simulate};
     auto actions = [
         &detectStalledPR,
         &detectInactiveStablePR,
@@ -285,7 +284,10 @@ else void main(string[] args)
     }
 
     if (cronRepositories)
-        return cronDaily(cronRepositories, runDailyCronSimulation);
+    {
+        CronConfig config = {simulate: runDailyCronSimulation};
+        return cronDaily(cronRepositories, config);
+    }
 
     startServer(settings);
     lowerPrivileges();

source/dlangbot/cron.d

@@ -20,6 +20,7 @@ struct CronConfig
     Duration stalledPRs = 90.days; // PRs with no activity within X days will get flagged
     Duration oldStablePRs = 3.days; // PRs targeting stable which should trigger a warning
     bool simulate;
+    Duration waitAfterMergeNullState = 750.msecs; // Time to wait before refetching a PR with mergeable: null
 }
 
 alias PRTuple = Tuple!(PullRequest, "pr", GHComment[], "comments", GHComment[], "reviewComments", CronConfig, "config");
@@ -73,8 +74,51 @@ auto detectInactiveStablePR(PRTuple t)
 
 auto detectPRWithMergeConflicts(PRTuple t)
 {
+    if (t.pr.mergeable.isNull)
+    {
+        logInfo("[cron-daily/%s/%d]: detectMerge - mergeable is null.", t.pr.repoSlug, t.pr.number);
+        // repeat request to receive computed mergeable information
+        foreach (i; 0 .. 4)
+        {
+            import vibe.core.core : sleep;
+            t.config.waitAfterMergeNullState.sleep;
+            logInfo("[cron-daily/%s/%d]: detectMerge - repeating request", t.pr.repoSlug, t.pr.number);
+            t.pr = t.pr.refresh;
+            if (!t.pr.mergeable.isNull)
+                goto mergable;
+        }
+        return LabelResponse(LabelAction.none, "");
+    }
+mergable:
+
+    bool isMergeable;
+    if (!t.pr.mergeableState.isNull)
+    {
+        logInfo("[cron-daily/%s/%d]: mergableState = %s", t.pr.repoSlug, t.pr.number, t.pr.mergeableState.get);
+        with (PullRequest.MergeableState)
+        final switch(t.pr.mergeableState)
+        {
+            case unknown, checking:
+                // should only be set if mergeable is null
+                return LabelResponse(LabelAction.none, "");
+            case clean, unstable:
+                isMergeable = true;
+                break;
+            case dirty:
+                isMergeable = false;
+                break;
+            // a reviewer has blocked the merge (not observed yet)
+            case blocked:
+                return LabelResponse(LabelAction.none, "");
+        }
+    }
+    else
+    {
+        logInfo("[cron-daily/%s/%d]: mergable = %s", t.pr.repoSlug, t.pr.number, t.pr.mergeable.get);
+        isMergeable = t.pr.mergeable.get;
+    }
+
     // "needs rebase" gets automatically removed on a new push
-    bool isMergeable = !t.pr.mergeable.isNull && t.pr.mergeable.get;
     with(LabelAction)
     return LabelResponse(!isMergeable ? add : remove, "needs rebase");
 }

source/dlangbot/github_api.d

@@ -136,11 +136,13 @@ struct PullRequest
         Nullable!Repo repo;
     }
     Branch base, head;
-    enum MergeableState { clean, dirty, unstable, blocked, unknown }
+    enum MergeableState { checking, clean, dirty, unstable, blocked, unknown }
     @byName GHState state;
     uint number;
+    string url;
     string title;
     @optional Nullable!bool mergeable;
+    // https://platform.github.community/t/documentation-about-mergeable-state/4259
     @optional @byName @name("mergeable_state") Nullable!MergeableState mergeableState;
     @name("created_at") SysTime createdAt;
     @name("updated_at") SysTime updatedAt;
@@ -223,6 +225,12 @@ struct PullRequest
             req.writeJsonBody(merge);
         }, mergeURL);
     }
+
+    typeof(this) refresh() {
+        return ghGetRequest(url)
+                .readJson
+                .deserializeJson!(typeof(this));
+    }
 }
 
 static struct GHUser

test/cronjob.d

@@ -84,3 +84,58 @@ unittest
 
     testCronDaily(repositories);
 }
+
+// test that no label updates are sent if no activity was found
+unittest
+{
+    import std.datetime : Clock, days;
+    setAPIExpectations(
+        "/github/repos/dlang/phobos/issues?state=open&sort=updated&direction=asc", (ref Json j) {
+            // only test one pull request
+            j = Json([j[0]]);
+        },
+        "/github/repos/dlang/phobos/pulls/2526", (ref Json j) {
+            j["mergeable"] = false;
+        },
+        "/github/repos/dlang/phobos/status/a04acd6a2813fb344d3e47369cf7fd64523ece44",
+        "/github/repos/dlang/phobos/issues/2526/comments", (ref Json j) {
+            j[$ - 1]["created_at"] = (Clock.currTime - 2.days).toISOExtString;
+            j[$ - 1]["updated_at"] = (Clock.currTime - 2.days).toISOExtString;
+        },
+        "/github/repos/dlang/phobos/pulls/2526/comments",
+    );
+
+    testCronDaily(repositories);
+}
+
+// test that the merge state gets refreshed
+unittest
+{
+    import std.datetime : Clock, days;
+    setAPIExpectations(
+        "/github/repos/dlang/phobos/issues?state=open&sort=updated&direction=asc", (ref Json j) {
+            // only test one pull request
+            j = Json([j[0]]);
+        },
+        "/github/repos/dlang/phobos/pulls/2526", (ref Json j) {
+            j["mergeable"] = null;
+        },
+        "/github/repos/dlang/phobos/pulls/2526", (ref Json j) {
+            j["mergeable"] = false;
+            j["mergeable_state"] = "dirty";
+        },
+        "/github/repos/dlang/phobos/status/a04acd6a2813fb344d3e47369cf7fd64523ece44",
+        "/github/repos/dlang/phobos/issues/2526/comments", (ref Json j) {
+            j[$ - 1]["created_at"] = (Clock.currTime - 2.days).toISOExtString;
+            j[$ - 1]["updated_at"] = (Clock.currTime - 2.days).toISOExtString;
+        },
+        "/github/repos/dlang/phobos/pulls/2526/comments",
+        "/github/repos/dlang/phobos/issues/2526/labels",
+        (scope HTTPServerRequest req, scope HTTPServerResponse res){
+            assert(req.method == HTTPMethod.PUT);
+            assert(req.json[].map!(e => e.get!string).equal(["blocked", "needs rebase"]));
+        },
+    );
+
+    testCronDaily(repositories);
+}

test/utils.d

@@ -321,10 +321,14 @@ void openUrl(string url, string expectedResponse,
 void testCronDaily(string[] repositories, int line = __LINE__, string file = __FILE__)
 {
     import dlangbot.app : cronDaily;
-    bool simulate = false;
+    import dlangbot.cron : CronConfig;
 
     logInfo("Starting cron test in %s:%d", file, line);
 
-    cronDaily(repositories, simulate);
+    CronConfig config = {
+        simulate: false,
+        waitAfterMergeNullState: 1.msecs,
+    };
+    cronDaily(repositories, config);
     checkAPIExpectations;
 }