Review

4 files changed, 36 insertions, 17 deletions

diff --git a/mod/ci-common.cxx b/mod/ci-common.cxx
index e9b1b07..5191d46 100644
--- a/mod/ci-common.cxx
+++ b/mod/ci-common.cxx
@@ -546,6 +546,7 @@ namespace brep
   {
     using namespace odb::core;
 
+    assert (mode == duplicate_tenant_mode::fail || !service.id.empty ());
     assert (!transaction::has_current ());
 
     duplicate_tenant_result r (duplicate_tenant_result::created);
diff --git a/mod/ci-common.hxx b/mod/ci-common.hxx
index c03abf8..df580e4 100644
--- a/mod/ci-common.hxx
+++ b/mod/ci-common.hxx
@@ -99,10 +99,10 @@ namespace brep
     // same as separate calls to cancel() and then to create() since the
     // latter would happen in two separate transactions and will thus be racy.
     //
-    // Note: should be called out of the database transaction.
+    // Finally note that only duplicate_tenant_mode::fail can be used if the
+    // service id is empty.
     //
-    // @@ TMP Shouldn't the comments mention that if tenant_service.id is an
-    //    empty string then the generated tenant id will be used?
+    // Note: should be called out of the database transaction.
     //
     enum class duplicate_tenant_mode {fail, ignore, replace, replace_archived};
     enum class duplicate_tenant_result {created, ignored, replaced};
diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx
index c9218f3..a6cb742 100644
--- a/mod/mod-ci-github-service-data.hxx
+++ b/mod/mod-ci-github-service-data.hxx
@@ -11,6 +11,8 @@
 
 namespace brep
 {
+  // @@@ Check is any data members are unused.
+
   // Service data associated with the tenant (corresponds to GH check suite).
   //
   // It is always a top-level JSON object and the first member is always the
@@ -44,8 +46,7 @@ namespace brep
   };
 
   // We have two kinds of service data that correspond to the following two
-  // scenarios (those are the only possible ones, until/unless we add support
-  // for merge queues):
+  // typical scenarios (until/unless we add support for merge queues):
   //
   // 1. Branch push (via check_suite) plus zero or more local PRs (via
   //    pull_request) that share the same head commit id.
@@ -58,6 +59,13 @@ namespace brep
   // can be created and is not behind base. We do all this before we actually
   // create the CI tenant.
   //
+  // Note that the above two cases are typical but not the only possible
+  // scenarios. Specifically, it is possible to have a mixture of all three
+  // kinds (branch push, local PR, and remote PR) since the same head commit
+  // id can be present in both local and remote branches. There is no way to
+  // handle this case perfectly and we do the best we can (see
+  // build_unloaded_pre_check() for details).
+  //
   struct service_data
   {
     // The data schema version. Note: must be first member in the object.
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 6c0bb72..6289d8b 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -473,6 +473,8 @@ namespace brep
     //
     string sid (cs.repository.node_id + ":" + cs.check_suite.head_sha);
 
+    // @@ Let's pass the "state" arguments explicitly in both constructors.
+    //
     service_data sd (warning_success,
                      iat->token,
                      iat->expires_at,
@@ -515,7 +517,7 @@ namespace brep
     if (!pr)
     {
       fail << "check suite " << cs.check_suite.node_id
-           << ": unable to create unloaded CI request";
+           << ": unable to create unloaded CI tenant";
     }
 
     if (dtm == duplicate_tenant_mode::replace &&
@@ -690,14 +692,16 @@ namespace brep
 
     l3 ([&]{trace << "installation_access_token { " << *iat << " }";});
 
-    // @@ TMP Regarding pushes to PR head branches ("synchronize"): If a
-    //    remote PR head branch is not behind base, wouldn't we want to cancel
-    //    existing CIs and restart? This is the only event we'll see when the
-    //    user clicks the "update PR head branch with base" button.
-    //
+    // Note that similar to the branch push case above, while it would have
+    // been nice to cancel the previous CI job once the PR head moves (the
+    // "synchronize" event), due to the head sharing problem the previous CI
+    // job might actually still be relevant (in both local and remote PR
+    // cases).
 
     // @@ TMP We can already determine whether the PR is local or remote so we
-    //    could pass `kind` to the service_data constructor.
+    //    could pass `kind` to the service_data constructor. Let's do.
+
+    // @@ TODO: what happens when the entire PR build is re-requested?
 
     // @@ TODO Serialize the new service_data fields!
     //
@@ -712,7 +716,7 @@ namespace brep
                      move (pr.repository.clone_url),
                      pr.pull_request.number);
 
-    assert (sd.pre_check);
+    assert (sd.pre_check); // @@ Get rid (once ctor changed).
 
     // Create an unloaded CI request for the pre-check phase (during which we
     // wait for the PR's merge commit and behindness to become available).
@@ -720,12 +724,12 @@ namespace brep
     // Create with an empty service id so that the generated tenant id is used
     // instead during the pre-check phase (so as not to clash with a proper
     // service id for this head commit, potentially created in
-    // handle_check_suite()).
+    // handle_check_suite() or as another PR).
     //
     tenant_service ts ("", "ci-github", sd.json ());
 
-    // Note: use no delay since we need to (re)create the synthetic conclusion
-    // check run as soon as possible.
+    // Note: use no delay since we need to start the actual CI (which in turn
+    // (re)creates the synthetic conclusion check run) as soon as possible.
     //
     // After this call we will start getting the build_unloaded()
     // notifications -- which will be routed to build_unloaded_pre_check() --
@@ -742,7 +746,7 @@ namespace brep
                  chrono::seconds (0) /* delay */))
     {
       fail << "pull request " << pr.pull_request.node_id
-           << ": unable to create unloaded CI request";
+           << ": unable to create unloaded pre-check tenant";
     }
 
     return true;
@@ -787,6 +791,12 @@ namespace brep
     // - Otherwise, create unloaded CI tenant (with proper duplicate mode
     //   based on re_request) and cancel itself.
 
+    // Note that in case of a mixed local/remote case, whether we CI the head
+    // commit or test merge commit will be racy and there is nothing we can do
+    // about (the purely local case can get "upgraded" to mixed after we have
+    // started the CI job).
+    //
+
     return nullptr;
   }