Review

4 files changed, 44 insertions, 59 deletions

diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx
index 17096eb..1909e1f 100644
--- a/mod/mod-ci-github-gq.cxx
+++ b/mod/mod-ci-github-gq.cxx
@@ -241,7 +241,8 @@ namespace brep
       {
         vector<gh_check_run> check_runs; // Received check runs.
 
-        resp (json::parser& p): check_runs (gq_parse_response_check_runs (p)) {}
+        resp (json::parser& p)
+            : check_runs (gq_parse_response_check_runs (p)) {}
 
         resp () = default;
       } rs;
@@ -271,23 +272,23 @@ namespace brep
             // @@ Are we handling the case where the resulting state (built)
             //    differs from what we expect?
             //
-            if (rst != build_state::built && rst != st)
+            // @@@ Does built-to-built transition updates status?
+            //
+            if (rst != st && rst != build_state::built)
             {
               error << "unexpected check_run status: received '" << rcr.status
                     << "' but expected '" << gh_to_status (st) << '\'';
 
               return false; // Fail because something is clearly very wrong.
             }
-            else
-            {
-              check_run& cr (crs[i]);
 
-              if (!cr.node_id)
-                cr.node_id = move (rcr.node_id);
+            check_run& cr (crs[i]);
 
-              cr.state = rst;
-              cr.state_synced = true;
-            }
+            if (!cr.node_id)
+              cr.node_id = move (rcr.node_id);
+
+            cr.state = rst;
+            cr.state_synced = (rst == st);
           }
 
           return true;
diff --git a/mod/mod-ci-github-gq.hxx b/mod/mod-ci-github-gq.hxx
index 3083696..9022fe3 100644
--- a/mod/mod-ci-github-gq.hxx
+++ b/mod/mod-ci-github-gq.hxx
@@ -20,8 +20,8 @@ namespace brep
   //
 
   // Create a new check run on GitHub for each build. Update `check_runs` with
-  // the new states and node IDs. Return false and issue diagnostics if the
-  // request failed.
+  // the new data (node id, state, and state_synced). Return false and issue
+  // diagnostics if the request failed.
   //
   // Note: no details_url yet since there will be no entry in the build result
   // search page until the task starts building.
@@ -40,8 +40,8 @@ namespace brep
                         build_state);
 
   // 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
-  // failed.
+  // data (node id, state, and state_synced). Return false and issue
+  // diagnostics if the request failed.
   //
   // If the details_url is absent GitHub will use the app's homepage.
   //
@@ -69,8 +69,12 @@ namespace brep
   // Update a check run on GitHub.
   //
   // Send a GraphQL request that updates an existing check run. Update `cr`
-  // with the new state. Return false and issue diagnostics if the request
-  // failed.
+  // with the new data (state and state_synced). Return false and issue
+  // diagnostics if the request failed.
+  //
+  // Note that GitHub allows any state transitions except from built (but
+  // built to built is allowed). The latter case is signalled by setting the
+  // check_run state_synced member to false and the state member to built.
   //
   // If the details_url is absent GitHub will use the app's homepage.
   //
diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx
index eb81ae4..0dd52ca 100644
--- a/mod/mod-ci-github-service-data.hxx
+++ b/mod/mod-ci-github-service-data.hxx
@@ -33,7 +33,7 @@ namespace brep
     build_state             state;
     bool                    state_synced;
 
-    optional<result_status> status; // Only present if state is built.
+    optional<result_status> status; // Only if state is built & synced.
 
     string
     state_string () const
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index e7dd7b5..b2e0c41 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -1736,34 +1736,8 @@ namespace brep
 
       if (cr.state_synced)
       {
-        // Check run was created/updated successfully to built.
-        //
-        // @@ TMP Feels like this should also be done inside
-        //    gq_{create,update}_check_run() -- where cr.state is set if the
-        //    create/update succeeds -- but I think we didn't want to pass a
-        //    result_status into a gq_ function because converting to a GitHub
-        //    conclusion/title/summary is reasonably complicated.
-        //
-        //    @@@ We need to redo that code:
-        //
-        //    - Pass the vector of check runs with new state (and status) set.
-        //    - Update synchronized flag inside those functions.
-        //    - Update the state to built if it's already built on GitHub --
-        //      but then what do we set the status to?
-        //
-        //      @@ TMP This scenario can only arise for updates to building.
-        //         For creations a new CR will always be created so the
-        //         returned state will always be what we asked for; and we
-        //         never update to queued.
-        //
-        //         As for updates to building, if GH has already been updated
-        //         to built then the build_built() lambda will soon save the
-        //         built state and valid status so nothing more should need to
-        //         be done. In fact, whenever updating to building we do stop
-        //         immediately if it's already built on GH.
-        //
-        //    - Maybe signal in return value (optional<bool>?) that there is
-        //      a discrepancy.
+        // Check run was created/updated successfully to built (with
+        // status we specified).
         //
         cr.status = b.status;
 
@@ -1833,24 +1807,30 @@ namespace brep
       if (iat)
         sd.installation_access = *iat;
 
-      if (check_run* scr = sd.find_check_run (cr.build_id))
+      // Only update the check_run state in service data if it matches the
+      // state (specifically, status) on GitHub.
+      //
+      if (cr.state_synced)
       {
-        // 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 (check_run* scr = sd.find_check_run (cr.build_id))
         {
-          warn << "check run " << cr.build_id << ": out of order built "
-               << "notification service data update; existing state: "
-               << scr->state_string ();
-        }
+          // 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)
+          {
+            warn << "check run " << cr.build_id << ": out of order built "
+                 << "notification service data update; existing state: "
+                 << scr->state_string ();
+          }
 #endif
-        *scr = cr;
+          *scr = cr;
+        }
+        else
+          sd.check_runs.push_back (cr);
       }
-      else
-        sd.check_runs.push_back (cr);
 
       return sd.json ();
     };