From f24e48fef5be97f53225ce3a115f35a419b3cdbd Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Fri, 7 Jun 2024 10:17:30 +0200
Subject: Review

---
 mod/mod-ci-github-gq.cxx           | 10 +++++++++-
 mod/mod-ci-github-service-data.cxx |  6 ++++++
 mod/mod-ci-github.cxx              | 12 ++++++++++++
 3 files changed, 27 insertions(+), 1 deletion(-)

(limited to 'mod')

diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx
index bcf9e55..1b967e5 100644
--- a/mod/mod-ci-github-gq.cxx
+++ b/mod/mod-ci-github-gq.cxx
@@ -265,6 +265,12 @@ namespace brep
 
             build_state rst (gh_from_status (rcr.status)); // Received state.
 
+            // Note that GitHub won't allow us to change a built check run
+            // to any other state.
+            //
+            // @@ Are we handling the case where the resulting state (built)
+            //    differs from what we expect?
+            //
             if (rst != build_state::built && rst != st)
             {
               error << "unexpected check_run status: received '" << rcr.status
@@ -279,7 +285,7 @@ namespace brep
               if (!cr.node_id)
                 cr.node_id = move (rcr.node_id);
 
-              cr.state = gh_from_status (rcr.status);
+              cr.state = rst;
               cr.state_synced = true;
             }
           }
@@ -636,6 +642,8 @@ namespace brep
                   ; // Still being generated; leave value absent.
                 else
                 {
+                  // @@ Let's throw invalid_json.
+                  //
                   parse_error = "unexpected mergeable value '" + ma + '\'';
 
                   // Carry on as if it were UNKNOWN.
diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx
index 7ae0b4f..6db32fd 100644
--- a/mod/mod-ci-github-service-data.cxx
+++ b/mod/mod-ci-github-service-data.cxx
@@ -77,7 +77,10 @@ namespace brep
       {
         string* v (p.next_expect_member_string_null ("status"));
         if (v != nullptr)
+        {
           rs = bbot::to_result_status (*v);
+          assert (s == build_state::built);
+        }
       }
 
       check_runs.emplace_back (move (bid), move (nm), move (nid), s, ss, rs);
@@ -188,7 +191,10 @@ namespace brep
 
       s.member_name ("status");
       if (cr.status)
+      {
+        assert (cr.state == build_state::built);
         s.value (to_string (*cr.status));
+      }
       else
         s.value (nullptr);
 
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 0ddc75d..d829ff6 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -1579,7 +1579,10 @@ namespace brep
           if (cr.state == build_state::built)
           {
             if (conclusion)
+            {
+              assert (cr.status);
               *conclusion |= *cr.status;
+            }
           }
           else
             conclusion = nullopt;
@@ -1798,6 +1801,15 @@ namespace brep
         //    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?
+        //    - Maybe signal in return value (optional<bool>?) that there is
+        //      a discrepancy.
+        //
         cr.status = b.status;
 
         // Update the conclusion check run if all check runs are now built.
-- 
cgit v1.1