Review

6 files changed, 87 insertions, 79 deletions

diff --git a/mod/mod-ci-github-gh.cxx b/mod/mod-ci-github-gh.cxx
index 5c4809c..c97f938 100644
--- a/mod/mod-ci-github-gh.cxx
+++ b/mod/mod-ci-github-gh.cxx
@@ -20,9 +20,6 @@ namespace brep
     case build_state::queued:   return "QUEUED";
     case build_state::building: return "IN_PROGRESS";
     case build_state::built:    return "COMPLETED";
-    default:
-      throw invalid_argument ("invalid build_state value: " +
-                              to_string (static_cast<int> (st)));
     }
   }
 
@@ -36,19 +33,21 @@ namespace brep
     else if (s == "IN_PROGRESS") return build_state::building;
     else if (s == "COMPLETED")   return build_state::built;
     else
-      throw invalid_argument ("invalid GitHub check run status: '" + s +
+      throw invalid_argument ("unexpected GitHub check run status: '" + s +
                               '\'');
   }
 
   string
-  gh_to_conclusion (result_status rs)
+  gh_to_conclusion (result_status rs, bool warning_success)
   {
     switch (rs)
     {
     case result_status::success:
-    case result_status::warning:
       return "SUCCESS";
 
+    case result_status::warning:
+      return warning_success ? "SUCCESS" : "FAILURE";
+
     case result_status::error:
     case result_status::abort:
     case result_status::abnormal:
@@ -60,12 +59,6 @@ namespace brep
     case result_status::interrupt:
       throw invalid_argument ("unexpected result_status value: " +
                               to_string (rs));
-
-      // Invalid value.
-      //
-    default:
-      throw invalid_argument ("invalid result_status value: " +
-                              to_string (static_cast<int> (rs)));
     }
   }
 
diff --git a/mod/mod-ci-github-gh.hxx b/mod/mod-ci-github-gh.hxx
index 45b5359..7e46cbb 100644
--- a/mod/mod-ci-github-gh.hxx
+++ b/mod/mod-ci-github-gh.hxx
@@ -81,8 +81,11 @@ namespace brep
   build_state
   gh_from_status (const string&);
 
+  // If warning_success is true, then map result_status::warning to SUCCESS
+  // and to FAILURE otherwise.
+  //
   string
-  gh_to_conclusion (result_status);
+  gh_to_conclusion (result_status, bool warning_success);
 
   // Create a check_run name from a build. If the second argument is not
   // NULL, return an abbreviated id if possible.
diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx
index 4e13199..bedf301 100644
--- a/mod/mod-ci-github-gq.cxx
+++ b/mod/mod-ci-github-gq.cxx
@@ -324,7 +324,7 @@ namespace brep
   gq_mutation_create_check_runs (
     const string& ri, // Repository ID
     const string& hs, // Head SHA
-    const vector<reference_wrapper<const build>>& bs,
+    const vector<reference_wrapper<const build>>& bs, // Pass build ids.
     build_state st, optional<result_status> rs,
     const build_queued_hints* bh)
   {
diff --git a/mod/mod-ci-github-gq.hxx b/mod/mod-ci-github-gq.hxx
index 9331b2b..16117ea 100644
--- a/mod/mod-ci-github-gq.hxx
+++ b/mod/mod-ci-github-gq.hxx
@@ -24,14 +24,14 @@ namespace brep
   // request failed.
   //
   bool
-  gq_create_check_runs (vector<check_run>& check_runs,
+  gq_create_check_runs (const basic_mark& error,
+                        vector<check_run>& check_runs,
                         const string& installation_access_token,
                         const string& repository_id,
                         const string& head_sha,
                         const vector<reference_wrapper<const build>>&,
                         build_state,
-                        const build_queued_hints&,
-                        const basic_mark& error);
+                        const build_queued_hints&);
 
   // Create a new check run on GitHub for a build. Update `cr` with the new
   // state and the node ID. Return false and issue diagnostics if the request
@@ -43,15 +43,15 @@ namespace brep
   // @@ TODO Support output (title, summary, text).
   //
   bool
-  gq_create_check_run (check_run& cr,
+  gq_create_check_run (const basic_mark& error,
+                       check_run& cr,
                        const string& installation_access_token,
                        const string& repository_id,
                        const string& head_sha,
                        const build&,
                        build_state,
-                       optional<result_status>,
-                       const build_queued_hints&,
-                       const basic_mark& error);
+                       optional<result_status> = nullopt,
+                       const build_queued_hints&);
 
   // Update a check run on GitHub.
   //
@@ -65,13 +65,13 @@ namespace brep
   // @@ TODO Support output (title, summary, text).
   //
   bool
-  gq_update_check_run (check_run& cr,
+  gq_update_check_run (const basic_mark& error,
+                       check_run& cr,
                        const string& installation_access_token,
                        const string& repository_id,
                        const string& node_id,
                        build_state,
-                       optional<result_status>,
-                       const basic_mark& error);
+                       optional<result_status> = nullopt);
 }
 
 #endif // MOD_MOD_CI_GITHUB_GQ_HXX
diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx
index d4a47d5..99c50ae 100644
--- a/mod/mod-ci-github-service-data.hxx
+++ b/mod/mod-ci-github-service-data.hxx
@@ -25,6 +25,7 @@ namespace brep
   struct check_run
   {
     string                build_id; // Full build id.
+    string                name;     // Potentially shortened build id.
     optional<string>      node_id;  // GitHub id.
 
     build_state           state;
@@ -46,6 +47,10 @@ namespace brep
     //
     uint64_t version = 1;
 
+    // Check suite settings.
+    //
+    bool warning_success; // See gh_to_conclusion().
+
     // Check suite-global data.
     //
     gh_installation_access_token installation_access;
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 9449f97..3e6803e 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -411,8 +411,8 @@ namespace brep
   //    building  Skip if there is no check run in service data or it's
   //              not in the queued state, otherwise update.
   //
-  //    built     Update if there is check run in service data and its
-  //              state is not built, otherwise create new.
+  //    built     Update if there is check run in service data unless its
+  //              state is built, otherwise create new.
   //
   //    The rationale for this semantics is as follows: the building
   //    notification is a "nice to have" and can be skipped if things are not
@@ -603,7 +603,8 @@ namespace brep
   }
 
   function<optional<string> (const tenant_service&)> ci_github::
-  build_building (const tenant_service& ts, const build& b,
+  build_building (const tenant_service& ts,
+                  const build& b,
                   const diag_epilogue& log_writer) const noexcept
   {
     NOTIFICATION_DIAG (log_writer);
@@ -743,7 +744,8 @@ namespace brep
   }
 
   function<optional<string> (const tenant_service&)> ci_github::
-  build_built (const tenant_service& ts, const build& b,
+  build_built (const tenant_service& ts,
+               const build& b,
                const diag_epilogue& log_writer) const noexcept
   {
     NOTIFICATION_DIAG (log_writer);
@@ -760,32 +762,35 @@ namespace brep
     }
 
     check_run cr; // Updated check run.
-    string bid (gh_check_run_name (b)); // Full Build ID.
-
-    if (check_run* scr = sd.find_check_run (bid))
     {
-      if (scr->state != build_state::building)
+      string bid (gh_check_run_name (b)); // Full Build ID.
+
+      if (check_run* scr = sd.find_check_run (bid))
       {
-        warn << "check run " << bid << ": out of order built "
-             << "notification; existing state: " << scr->state_string ();
-      }
+        if (scr->state != build_state::building)
+        {
+          warn << "check run " << bid << ": out of order built notification; "
+               << "existing state: " << scr->state_string ();
+        }
 
-      // Do nothing if already built.
-      //
-      if (scr->state == build_state::built)
-        return nullptr;
+        // Do nothing if already built (e.g., rebuild).
+        //
+        if (scr->state == build_state::built)
+          return nullptr;
 
-      cr = move (*scr);
-    }
-    else
-    {
-      warn << "check run " << bid << ": out of order built "
-           << "notification; no check run state in service data";
+        cr = move (*scr);
+      }
+      else
+      {
+        warn << "check run " << bid << ": out of order built notification; "
+             << "no check run state in service data";
 
-      cr.build_id = move (bid);
-    }
+        cr.build_id = move (bid);
+        cr.name = cr.build_id;
+      }
 
-    cr.state_synced = false;
+      cr.state_synced = false;
+    }
 
     // Get a new installation access token if the current one has expired.
     //
@@ -838,9 +843,6 @@ namespace brep
         // check run to the service data it will create another check run with
         // the shortened name which will never get to the built state.
         //
-        //   @@ TMP If build_queued() runs but fails to create we could store
-        //          the build hints and use them now.
-        //
         if (gq_create_check_run (cr,
                                  iat->token,
                                  sd.repository_id,
@@ -861,40 +863,45 @@ namespace brep
             cr = move (cr),
             error = move (error),
             warn = move (warn)] (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 captures.
+    {
+      // 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;
-        }
+      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 (iat)
+        sd.installation_access = *iat;
 
-        if (check_run* scr = sd.find_check_run (cr.build_id))
+      if (check_run* scr = sd.find_check_run (cr.build_id))
+      {
+        // This will most commonly generate a duplicate warning (see above).
+        // We could save the old state and only warn if it differs but let's
+        // not complicate things for now.
+        //
+#if 0
+        if (scr->state != build_state::building)
         {
-          if (scr->state != build_state::building)
-          {
-            warn << "check run " << cr.build_id << ": out of order built "
-                 << "notification service data update; existing state: "
-                 << scr->state_string ();
-          }
-
-          *scr = cr;
+          warn << "check run " << cr.build_id << ": out of order built "
+               << "notification service data update; existing state: "
+               << scr->state_string ();
         }
-        else
-          sd.check_runs.push_back (cr);
+#endif
+        *scr = cr;
+      }
+      else
+        sd.check_runs.push_back (cr);
 
-        return sd.json ();
-      };
+      return sd.json ();
+    };
   }
 
   optional<string> ci_github::