From 0a7f176c21075cafac7862226304e2cd0142fa4b Mon Sep 17 00:00:00 2001
From: Francois Kritzinger <francois@codesynthesis.com>
Date: Wed, 3 Apr 2024 09:37:06 +0200
Subject: Post-review changes

---
 mod/mod-ci-github.cxx | 100 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 72 insertions(+), 28 deletions(-)

diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 2e2d160..b402af6 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -333,6 +333,12 @@ namespace brep
       string                build_id; // Full build id.
       optional<string>      node_id;  // GitHub id.
       optional<build_state> state;
+
+      string
+      state_string () const
+      {
+        return state ? to_string (*state) : "null";
+      }
     };
     vector<check_run> check_runs;
 
@@ -363,6 +369,9 @@ namespace brep
     json () const;
   };
 
+  ostream&
+  operator<< (ostream&, const service_data::check_run&);
+
   bool ci_github::
   handle_check_suite_request (check_suite_event cs)
   {
@@ -930,14 +939,6 @@ namespace brep
     //
     for (const build& b: builds)
     {
-      // Include this build if this is a legitimate transition back to queued
-      // from another state or it doesn't already have a stored check run.
-      // Note: never go back on the built state.
-      //
-      string bid (check_run_name (b)); // Full Build ID.
-
-      const service_data::check_run* cr (sd.find_check_run (bid));
-
       // To keep things simple we are going to queue/create a new check run
       // only if we have no corresponding state (which means we haven't yet
       // done anything about this check run).
@@ -947,22 +948,31 @@ namespace brep
       // building, which is probably not a big deal. Also, this sidesteps
       // various "absent state" corner.
       //
-      if (cr == nullptr)
-      {
-        if (cr != nullptr)
-          crs.emplace_back (move (*cr)).state = nullopt;
-        else
-          crs.emplace_back (move (bid), nullopt, nullopt);
+      // Note: never go back on the built state.
+      //
+      string bid (check_run_name (b)); // Full Build ID.
 
+      const service_data::check_run* scr (sd.find_check_run (bid));
+
+      if (scr == nullptr)
+      {
+        crs.emplace_back (move (bid), nullopt, nullopt);
         bs.push_back (b);
       }
-      else if (!cr->state)
+      else if (!scr->state)
         ; // Ignore network issue.
       else if (istate && *istate == build_state::building)
         ; // Ignore interrupted.
       else
       {
-        // @@ TODO warn with some information
+        // Out of order queued notification or a rebuild (not allowed).
+        //
+        warn << *scr << ": "
+             << "unexpected transition from "
+             << (istate ? to_string (*istate) : "null") << " to "
+             << to_string (build_state::queued)
+             << "; previously recorded check_run state: "
+             << scr->state_string ();
       }
     }
 
@@ -992,7 +1002,6 @@ namespace brep
         new_iat = obtain_installation_access_token (sd.installation_id,
                                                     move (*jwt),
                                                     error);
-
         if (new_iat)
           iat = &*new_iat;
       }
@@ -1084,7 +1093,9 @@ namespace brep
 
     return [bs = move (bs),
             iat = move (new_iat),
-            crs = move (crs)] (const tenant_service& ts) -> optional<string>
+            crs = move (crs),
+            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.
@@ -1096,8 +1107,7 @@ namespace brep
       }
       catch (const invalid_argument& e)
       {
-        // @@ TODO: try to move error into this lambda?
-        // error << "failed to parse service data: " << e;
+        error << "failed to parse service data: " << e;
         return nullopt;
       }
 
@@ -1118,8 +1128,9 @@ namespace brep
         //
         if (service_data::check_run* scr = sd.find_check_run (cr.build_id))
         {
-          // @@ TODO: let's warn about this out of order case to see how
-          //          often we get it.
+          warn << cr << " state " << scr->state_string ()
+               << " was stored before notified state " << cr.state_string ()
+               << " could be stored";
         }
         else
           sd.check_runs.push_back (cr);
@@ -1327,10 +1338,23 @@ namespace brep
     p.next_expect_member_array ("check_runs");
     while (p.next_expect (event::begin_object, event::end_array))
     {
-      check_runs.emplace_back (
-          p.next_expect_member_string ("build_id"),
-          p.next_expect_member_string ("node_id"),
-          to_build_state (p.next_expect_member_string ("state")));
+      string bid (p.next_expect_member_string ("build_id"));
+
+      optional<string> nid;
+      {
+        string* v (p.next_expect_member_string_null ("node_id"));
+        if (v != nullptr)
+          nid = *v;
+      }
+
+      optional<build_state> s;
+      {
+        string* v (p.next_expect_member_string_null ("state"));
+        if (v != nullptr)
+          s = to_build_state (*v);
+      }
+
+      check_runs.emplace_back (move (bid), move (nid), s);
 
       p.next_expect (event::end_object);
     }
@@ -1377,8 +1401,19 @@ namespace brep
     {
       s.begin_object ();
       s.member ("build_id", cr.build_id);
-      s.member ("node_id", *cr.node_id);
-      s.member ("state", to_string (*cr.state));
+
+      s.member_name ("node_id");
+      if (cr.node_id)
+        s.value (*cr.node_id);
+      else
+        s.value (nullptr);
+
+      s.member_name ("state");
+      if (cr.state)
+        s.value (to_string (*cr.state));
+      else
+        s.value (nullptr);
+
       s.end_object ();
     }
     s.end_array ();
@@ -1399,6 +1434,15 @@ namespace brep
     return nullptr;
   }
 
+  ostream&
+  operator<< (ostream& os, const service_data::check_run& cr)
+  {
+    os << "check_run { node_id: " << cr.node_id.value_or ("null")
+       << ", build_id: " << cr.build_id << " }";
+
+    return os;
+  }
+
   // The rest is GitHub request/response type parsing and printing.
   //
 
-- 
cgit v1.1