Post-review changes

3 files changed, 218 insertions, 191 deletions

diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx
index 54822ae..d8a1a0e 100644
--- a/mod/mod-ci-github-gq.cxx
+++ b/mod/mod-ci-github-gq.cxx
@@ -575,7 +575,7 @@ namespace brep
     return os.str ();
   }
 
-  pair<optional<gq_pr_mergeability>, bool>
+  optional<string>
   gq_pull_request_mergeable (const basic_mark& error,
                              const string& iat,
                              const string& nid)
@@ -595,7 +595,7 @@ namespace brep
         // The response value. Absent if the merge commit is still being
         // generated.
         //
-        optional<gq_pr_mergeability> value;
+        optional<string> value;
 
         resp (json::parser& p)
         {
@@ -617,11 +617,11 @@ namespace brep
                 string oid (p.next_expect_member_string ("oid"));
                 p.next_expect (event::end_object);
 
-                value = {true, move (oid)};
+                value = move (oid);
               }
               else if (ma == "CONFLICTING")
               {
-                value = {false, ""};
+                value = "";
               }
               else if (ma == "UNKNOWN")
               {
@@ -648,7 +648,7 @@ namespace brep
         if (!rs.found)
           error << "pull request '" << nid << "' not found";
 
-        return {move (rs.value), rs.found};
+        return move (rs.value);
       }
       else
         error << "failed to fetch pull request: error HTTP response status "
@@ -679,7 +679,7 @@ namespace brep
       error << "unable to fetch pull request: " << e;
     }
 
-    return {nullopt, false};
+    return nullopt;
   }
 
   // bool
diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx
index 8c6970c..7fd554d 100644
--- a/mod/mod-ci-github-service-data.cxx
+++ b/mod/mod-ci-github-service-data.cxx
@@ -43,12 +43,18 @@ namespace brep
 
     {
       string* s (p.next_expect_member_string_null ("repository_clone_url"));
-      if (s)
+      if (s != nullptr)
         repository_clone_url = *s;
     }
 
     pr_number = p.next_expect_member_number_null<uint32_t> ("pr_number");
 
+    {
+      string* s (p.next_expect_member_string_null ("merge_node_id"));
+      if (s != nullptr)
+        merge_node_id = *s;
+    }
+
     head_sha = p.next_expect_member_string ("head_sha");
 
     p.next_expect_member_array ("check_runs");
@@ -72,6 +78,12 @@ namespace brep
       p.next_expect (event::end_object);
     }
 
+    {
+      string* s (p.next_expect_member_string_null ("conclusion_node_id"));
+      if (s != nullptr)
+        conclusion_node_id = *s;
+    }
+
     p.next_expect (event::end_object);
   }
 
@@ -81,9 +93,24 @@ namespace brep
                 timestamp iat_ea,
                 uint64_t iid,
                 string rid,
+                string hs)
+      : warning_success (ws),
+        installation_access (move (iat_tok), iat_ea),
+        installation_id (iid),
+        repository_node_id (move (rid)),
+        head_sha (move (hs))
+  {
+  }
+
+  service_data::
+  service_data (bool ws,
+                string iat_tok,
+                timestamp iat_ea,
+                uint64_t iid,
+                string rid,
                 string hs,
-                optional<string> rcu,
-                optional<uint32_t> prn)
+                string rcu,
+                uint32_t prn)
       : warning_success (ws),
         installation_access (move (iat_tok), iat_ea),
         installation_id (iid),
@@ -128,6 +155,12 @@ namespace brep
     else
       s.value (nullptr);
 
+    s.member_name ("merge_node_id");
+    if (merge_node_id)
+      s.value (*merge_node_id);
+    else
+      s.value (nullptr);
+
     s.member ("head_sha", head_sha);
 
     s.member_begin_array ("check_runs");
@@ -150,6 +183,12 @@ namespace brep
     }
     s.end_array ();
 
+    s.member_name ("conclusion_node_id");
+    if (conclusion_node_id)
+      s.value (*conclusion_node_id);
+    else
+      s.value (nullptr);
+
     s.end_object ();
 
     return b;
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index f9fd453..be9daa9 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -650,15 +650,89 @@ namespace brep
     if (iat == nullptr)
       return nullptr;
 
-    bool first (...); // @@ TODO
+    // Synthetic merge check run node ID. Absent if the check run was created
+    // in a previous call (and has already been stored in the service data).
+    //
+    optional<string> mni;
+
+    // Create an in-progress check run. Return the check run on success or
+    // nullopt on failure.
+    //
+    auto create_cr = [iat, &sd, &error] (string name) -> optional<check_run>
+    {
+      check_run cr;
+      cr.name = move (name);
+
+      if (gq_create_check_run (error,
+                               cr,
+                               iat->token,
+                               sd.repository_node_id,
+                               sd.head_sha,
+                               // @@ TODO What details URL to use?
+                               "https://build2.org", // details URL.
+                               build_state::building))
+      {
+        return cr;
+      }
+      else
+        return nullopt;
+    };
+
+    // Update the merge check run with success or failure. Return the check
+    // run on success or nullopt on failure.
+    //
+    auto update_merge_cr = [iat, &sd, &mni, &error] (result_status rs,
+                                                     const string& summary)
+      -> optional<check_run>
+    {
+      optional<gq_built_result> br (
+        gq_built_result (gh_to_conclusion (rs, sd.warning_success),
+                         circle (rs) + ' ' + ucase (to_string (rs)),
+                         move (summary)));
+
+      check_run cr;
+      cr.name = "merge-commit"; // For display purposes only.
+
+      const string& ni (mni ? *mni : *sd.merge_node_id); // Node ID.
+
+      if (gq_update_check_run (error,
+                               cr,
+                               iat->token,
+                               sd.repository_node_id,
+                               ni,
+                               "https://build2.org", // details URL.
+                               build_state::built,
+                               move (br)))
+      {
+        return cr;
+      }
+      else
+        return nullopt;
+    };
+
+    // True if this is the first call (or the merge commit couldn't be created
+    // on the first call).
+    //
+    bool first (!sd.merge_node_id);
 
     // If this is the first call, (re)create the synthetic merge check run as
     // soon as possible to make sure the previous check suite, if any, is no
     // longer completed.
     //
+    // @@ TMP There is still a window between receipt of the pull_request
+    //    event and the first bot/worker asking for a task. That could be
+    //    minutes, right?
+    //
     if (first)
     {
-      // TODO: in-progress
+      if (auto cr = create_cr ("merge-commit"))
+      {
+        l3 ([&]{trace << "created check_run { " << *cr << " }";});
+
+        mni = move (cr->node_id);
+      }
+      else
+        return nullptr; // Try again on the next call.
     }
 
     // Start/check PR mergeability.
@@ -668,7 +742,7 @@ namespace brep
 
     if (!mc) // No merge commit yet.
     {
-      // If this is a subseqent notification and there is no merge commit,
+      // If this is a subsequent notification and there is no merge commit,
       // then there is nothing to do.
       //
       if (!first)
@@ -679,217 +753,139 @@ namespace brep
       //
       return [&error,
               iat = move (new_iat),
-              cr = move (cr),
-              ccr = move (ccr)] (const tenant_service& ts) -> optional<string>
+              mni = move (mni)] (const tenant_service& ts) -> optional<string>
       {
-        // @@ TODO
-      }
+        // NOTE: this lambda may be called repeatedly (e.g., due to
+        // transaction being aborted) and so should not move out of its
+        // captures.
+
+        service_data sd;
+        try
+        {
+          sd = service_data (*ts.data);
+        }
+        catch (const invalid_argument& e)
+        {
+          error << "failed to parse service data: " << e;
+          return nullopt;
+        }
+
+        if (iat)
+          sd.installation_access = *iat;
+
+        if (mni)
+          sd.merge_node_id = *mni;
+
+        return sd.json ();
+      };
     }
-    else if (mc->empty ()) // Not meargable.
+    else if (mc->empty ()) // Not mergeable.
     {
-      // If the commit is not mergable, cancel the CI request and fail the
+      // If the commit is not mergeable, cancel the CI request and fail the
       // merge check run.
       //
       // Note that it feels like in this case we don't really need to create a
       // failed synthetic conclusion check run since the PR cannot be merged
       // anyway.
 
-      // @@ TODO: cancel CI request
-      // @@ TODO: fail merge check run and update in service data.
+      if (auto cr = update_merge_cr (result_status::error,
+                                     "merge would create conflicts"))
+      {
+        l3 ([&]{trace << "updated check_run { " << *cr << " }";});
+
+        // @@ TODO: cancel CI request
+      }
+      // Don't cancel the CI request if the merge check run update failed so
+      // that we can try again on the next call.
 
       return [&error,
               iat = move (new_iat),
-              cr = move (cr),
-              ccr = move (ccr)] (const tenant_service& ts) -> optional<string>
+              mni = move (mni)] (const tenant_service& ts) -> optional<string>
       {
-        // @@ TODO
-      }
-    }
-
-    // If we are here, then it means we have a merge commit that we can load.
-    //
-
-    // As a first step, (re)create the synthetic conclusion check run and then
-    // change the merge check run state to success. Do it in this order so
-    // that the check suite does not become completed.
-
-    // @@ TODO: create synthetic conclusion check run (in progress)
-    // @@ TODO: update synthetic merge checj run (success)
-    // @@ TODO: will need to update conclusion check run node_id in service data
+        // NOTE: this lambda may be called repeatedly (e.g., due to
+        // transaction being aborted) and so should not move out of its
+        // captures.
 
-    // @@ TODO: load CI request.
+        service_data sd;
+        try
+        {
+          sd = service_data (*ts.data);
+        }
+        catch (const invalid_argument& e)
+        {
+          error << "failed to parse service data: " << e;
+          return nullopt;
+        }
 
-    {
-      pair<optional<gq_pr_mergeability>, bool> pr (
-        gq_pull_request_mergeable (error, iat->token, ts.id));
+        if (iat)
+          sd.installation_access = *iat;
 
-      if (pr.second) // No errors.
-      {
-        if (pr.first) // Merge commit has been generated.
-          ma = move (pr.first);
-      }
-      else
-      {
-        error << "failed to get pull request " << ts.id << " mergeability";
+        if (mni)
+          sd.merge_node_id = mni;
 
-        return nullptr;
-      }
+        return sd.json ();
+      };
     }
 
-    // The merge commit's build state. Note that the PR fetch above initiated
-    // its generation on GitHub.
-    //
-    build_state bs;
-
-    // The check run result to use if the merge commit has been calculated, or
-    // absent if it has not.
+    // If we are here, then it means we have a merge commit that we can load.
     //
-    optional<gq_built_result> br;
-
-    if (ma)
-    {
-      bs = build_state::built;
-
-      result_status rs;
-      string sm; // Summary.
-
-      if (ma->mergeable)
-      {
-        rs = result_status::success;
-        sm = "merge would succeed";
-      }
-      else
-      {
-        rs = result_status::error;
-        sm = "merge would create conflicts";
-      }
-
-      br = gq_built_result (gh_to_conclusion (rs, sd.warning_success),
-                            circle (rs) + ' ' + ucase (to_string (rs)),
-                            move (sm));
-    }
-    else
-      bs = build_state::building;
 
-    // The stored merge commit check run.
-    //
-    check_run* scr (sd.find_check_run ("merge-commit"));
+    // As a first step, (re)create the synthetic conclusion check run and then
+    // change the merge check run state to success. Do it in this order so
+    // that the check suite does not become completed.
 
-    // The updated merge commit check run.
+    // Conclusion check run node ID. Absent if the check run was created in a
+    // previous call (and has already been stored in the service data).
     //
-    check_run cr;
+    optional<string> cni;
 
-    if (scr == nullptr || !scr->node_id)
+    if (!sd.conclusion_node_id)
     {
-      // Create the check run because the merge commit check run has not
-      // previously been stored, or we failed to create it last time we
-      // tried.
-      //
-      // @@ TMP There is still a window between receipt of the pull_request
-      //        event and the first bot/worker asking for a task. Shouldn't we
-      //        rather create the merge CR when we create the unloaded CI
-      //        build (in the pull_request handler)?
-      //
-      if (scr != nullptr)
-        cr = move (*scr);
-      else
+      if (auto cr = create_cr ("conclusion"))
       {
-        cr.build_id = "merge-commit";
-        cr.name = cr.build_id;
-      }
-      cr.state_synced = false;
+        l3 ([&]{trace << "created check_run { " << *cr << " }";});
 
-      if (gq_create_check_run (error,
-                               cr,
-                               iat->token,
-                               sd.repository_node_id,
-                               sd.head_sha,
-                               // @@ TODO What details URL to use?
-                               "https://build2.org", // details URL.
-                               bs,
-                               move (br)))
-      {
-        l3 ([&]{trace << "created check_run { " << cr << " }";});
+        cni = move (cr->node_id);
       }
     }
-    else if (ma)
-    {
-      // Update because the merge commit check run was previously stored and
-      // the merge commit result has become available.
-      //
-      cr = move (*scr);
-      cr.state_synced = false;
 
-      if (gq_update_check_run (error,
-                               cr,
-                               iat->token,
-                               sd.repository_node_id,
-                               *cr.node_id,
-                               "https://build2.org", // details URL.
-                               bs,
-                               move (br)))
-      {
-        l3 ([&]{trace << "updated check_run { " << cr << " }";});
-      }
-    }
-    else
+    if (cni || sd.conclusion_node_id)
     {
-      // Do nothing because nothing has changed.
+      // Update merge check run to successful.
       //
-      return nullptr;
-    }
-
-    // Create the conclusion check run and load the CI request.
-    //
-    optional<check_run> ccr; // Conclusion check run.
-
-    if (ma && ma->mergeable)
-    {
-      // Create the conclusion check run if the merge commit shows the PR is
-      // mergeable.
+      // @@ TMP If the CI load below failed on the previous call we probably
+      //    don't want to update the CR again but we can't tell the difference
+      //    (no state for the merge CR). However it should be a no-op due to
+      //    the GitHub API's semantics so it's probably OK?
       //
-      // @@ TMP We could do this later if we prefer: if the branch protection
-      //    rule requires the conclusion check run then the PR will not go
-      //    green until the conclusion check run is successful.
-      //
-      ccr = check_run ();
-      ccr->build_id = "conclusion";
-      ccr->name = ccr->build_id;
-      ccr->state_synced = false;
-
-      if (gq_create_check_run (error,
-                               *ccr,
-                               iat->token,
-                               sd.repository_node_id,
-                               sd.head_sha,
-                               // @@ TODO What details URL to use?
-                               "https://build2.org", // details URL.
-                               build_state::queued))
+      if (auto cr = update_merge_cr (result_status::success,
+                                     "merge would succeed"))
       {
-        l3 ([&]{trace << "created check_run { " << *ccr << " }";});
-      }
+        l3 ([&]{trace << "updated check_run { " << *cr << " }";});
 
-      // Load the CI request.
-      //
-      repository_location rl (*sd.repository_clone_url + "#refs/pull/" +
+        // Load the CI request.
+        //
+        repository_location rl (*sd.repository_clone_url + "#refs/pull/" +
                                 to_string (*sd.pr_number) + "/merge",
-                              repository_type::git);
+                                repository_type::git);
 
-      optional<start_result> r (
-        load (error, warn, &trace, *build_db_, move (ts), rl));
+        optional<start_result> r (
+          load (error, warn, &trace, *build_db_, move (ts), rl));
 
-      if (!r)
-      {
-        error << "unable to load CI request";
+        if (!r)
+        {
+          error << "unable to load CI request ";
 
-        // @@ TODO Handle this. Will get called again and in those cases do
-        //    nothing except load the CI request.
+          // Try again next call.
+        }
       }
+      // Don't load the CI request if the merge check run update failed so
+      // that we can try again on the next call.
     }
 
-    return
-      [&error, iat = move (new_iat), cr = move (cr), ccr = move (ccr)] (
-        const tenant_service& ts) -> optional<string>
+    return [&error,
+            iat = move (new_iat),
+            cni = move (cni)] (const tenant_service& ts) -> optional<string>
     {
       // NOTE: this lambda may be called repeatedly (e.g., due to
       // transaction being aborted) and so should not move out of its
@@ -909,16 +905,8 @@ namespace brep
       if (iat)
         sd.installation_access = *iat;
 
-      if (check_run* scr = sd.find_check_run (cr.build_id))
-        *scr = cr;
-      else
-        sd.check_runs.push_back (cr);
-
-      if (ccr)
-      {
-        assert (sd.find_check_run (ccr->build_id) == nullptr);
-        sd.check_runs.push_back (*ccr);
-      }
+      if (cni)
+        sd.conclusion_node_id = cni;
 
       return sd.json ();
     };