From e0d074e946c482a3cf497b89a883e955928519ca Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Thu, 6 Jun 2024 12:51:52 +0200
Subject: Review

---
 mod/mod-ci-github-gq.cxx | 27 ++++++++++++++++++++++-----
 mod/mod-ci-github.cxx    |  7 +++++--
 mod/mod-ci-github.hxx    |  8 +++++---
 3 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx
index 26b4c1f..df81854 100644
--- a/mod/mod-ci-github-gq.cxx
+++ b/mod/mod-ci-github-gq.cxx
@@ -628,9 +628,13 @@ namespace brep
               {
                 if (ma == "CONFLICTING")
                   value = "";
-                else if (ma == "UNKNOWN")
+                if (ma == "UNKNOWN")
+                  ; // Still being generated; leave value absent.
+                else
                 {
-                  // Still being generated; leave value absent.
+                  error << "unexpected mergeable value '" << ma << "'";
+
+                  // Carry on as if it were UNKNOWN.
                 }
 
                 // Skip the merge commit ID (which should be null).
@@ -697,7 +701,18 @@ namespace brep
   // page) open pull requests with the specified base branch from the
   // repository with the specified node ID.
   //
-  // @@ TMP Should we support more than 100?
+  // @@ TMP Should we support more/less than 100?
+  //
+  //    Doing more (or even 100) could waste a lot of CI resources on
+  //    re-testing stale PRs. Maybe we should create a failed synthetic
+  //    conclusion check run asking the user to re-run the CI manually if/when
+  //    needed.
+  //
+  //    Note that we cannot request more than 100 at a time (will need to
+  //    do multiple requests with paging, etc).
+  //
+  //    Also, maybe we should limit the result to "fresh" PRs, e.g., those
+  //    that have been "touched" in the last week.
   //
   // Example query:
   //
@@ -789,7 +804,7 @@ namespace brep
         {
           using event = json::event;
 
-          gq_parse_response (p, [this] (json::parser& p)
+          auto parse_data = [this] (json::parser& p)
           {
             p.next_expect (event::begin_object);
 
@@ -825,7 +840,9 @@ namespace brep
             }
 
             p.next_expect (event::end_object);
-          });
+          };
+
+          gq_parse_response (p, move (parse_data));
         }
 
         resp () = default;
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 8ea730f..70c1c7d 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -495,7 +495,8 @@ namespace brep
 
     // The merge commits of any open pull requests with this branch as base
     // branch will now be out of date, and thus so will be their CI builds and
-    // associated check runs.
+    // associated check runs (and, no, GitHub does not invalidate those CI
+    // results automatically; see below).
     //
     // Unfortunately GitHub does not provide a webhook for PR base branch
     // updates (as it does for PR head branch updates) so we have to handle it
@@ -512,7 +513,9 @@ namespace brep
     // user would be able to merge a broken PR.
     //
     // Regardless of the nature of the error, we have to let the check suite
-    // handling code proceed so we only issue diagnostics.
+    // handling code proceed so we only issue diagnostics. Note also that we
+    // want to run this code as early as possible to minimize the window of
+    // the user seeing misleading CI results.
     //
     {
       // Fetch open pull requests with the check suite's head branch as base
diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx
index 07295f2..489aac7 100644
--- a/mod/mod-ci-github.hxx
+++ b/mod/mod-ci-github.hxx
@@ -17,6 +17,8 @@
 
 namespace brep
 {
+  struct service_data;
+
   class ci_github: public database_module,
                    private ci_start,
                    public tenant_service_build_unloaded,
@@ -83,8 +85,8 @@ namespace brep
     // true, cancel its existing CI request first.
     //
     // Return true if an unloaded CI request was created. Ignore failure to
-    // cancel because the CI request may already have been cancelled for a
-    // legitimate reason.
+    // cancel because the CI request may already have been cancelled for other
+    // reasons.
     //
     // After this call we will start getting the build_unloaded()
     // notifications until (1) we load the request, (2) we cancel it, or (3)
@@ -94,7 +96,7 @@ namespace brep
     create_pull_request_ci (const basic_mark& error,
                             const basic_mark& warn,
                             const basic_mark& trace,
-                            string service_data,
+                            const service_data&,
                             const string& pull_request_node_id,
                             bool cancel_first) const;
 
-- 
cgit v1.1