Plan and prepare for upcoming restructuring

7 files changed, 203 insertions, 53 deletions

diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx
index bcf9e55..1e9af74 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 (but all other transitions are allowed).
+            //
+            // @@ 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.
@@ -739,6 +747,15 @@ namespace brep
   //     }
   //   }
   //
+  //
+  // pullRequests (last:50 states:[OPEN]
+  //               orderBy:{field:UPDATED_AT direction:ASC}
+  //               baseRefName:"master") {
+  //
+  // updatedAt field changes on PR close, conversion to draft PR, merge
+  // conflict resolution, etc. So seems like it changes with any kind of PR
+  // update.
+  //
   static string
   gq_query_fetch_open_pull_requests (const string& rid, const string& br)
   {
diff --git a/mod/mod-ci-github-gq.hxx b/mod/mod-ci-github-gq.hxx
index 439f7b7..ad9797a 100644
--- a/mod/mod-ci-github-gq.hxx
+++ b/mod/mod-ci-github-gq.hxx
@@ -83,24 +83,6 @@ namespace brep
                        build_state,
                        optional<gq_built_result> = nullopt);
 
-  // Fetch a pull request's mergeability from GitHub and return it in first,
-  // or absent if the merge commit is still being generated.
-  //
-  // Return false in second and issue diagnostics if the request failed.
-  //
-  struct gq_pr_mergeability
-  {
-    // True if the pull request is auto-mergeable; false if it would create
-    // conflicts.
-    //
-    bool mergeable;
-
-    // The ID of the test merge commit. Empty if mergeable is false.
-    //
-    string merge_commit_id;
-  };
-
-
   // Fetch a pull request's mergeability from GitHub. Return absent value if
   // the merge commit is still being generated. Return empty string if the
   // pull request is not auto-mergeable. Otherwise return the test merge
@@ -110,7 +92,8 @@ namespace brep
   // will be treated by the caller as still being generated).
   //
   // Note that the first request causes GitHub to start preparing the test
-  // merge commit.
+  // merge commit. (For details see
+  // https://docs.github.com/rest/guides/getting-started-with-the-git-database-api#checking-mergeability-of-pull-requests.)
   //
   optional<string>
   gq_pull_request_mergeable (const basic_mark& error,
diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx
index e2ed904..0f6e593 100644
--- a/mod/mod-ci-github-service-data.cxx
+++ b/mod/mod-ci-github-service-data.cxx
@@ -74,7 +74,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);
@@ -185,7 +188,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-service-data.hxx b/mod/mod-ci-github-service-data.hxx
index 5573d90..89fcaab 100644
--- a/mod/mod-ci-github-service-data.hxx
+++ b/mod/mod-ci-github-service-data.hxx
@@ -43,12 +43,33 @@ 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):
+  //
+  // 1. Branch push (via check_suite) plus zero or more local PRs (via
+  //    pull_request) that share the same head commit id.
+  //
+  // 2. One or more remote PRs (via pull_request) that share the same head
+  //    commit id (from a repository in another organization).
+  //
+  // Plus, for PRs, the service data may be in the pre-check phase while we
+  // are in the process of requesting the test merge commit and making sure it
+  // can be created and is not behind base. We do all this before we actually
+  // create the CI tenant.
+  //
   struct service_data
   {
     // The data schema version. Note: must be first member in the object.
     //
     uint64_t version = 1;
 
+    // Kind and phase.
+    //
+    enum {local, remote /*, queue */} kind;
+    bool pre_check;
+    bool re_request; // Re-requested (rebuild).
+
     // Check suite settings.
     //
     bool warning_success; // See gh_to_conclusion().
@@ -73,9 +94,16 @@ namespace brep
     //
     optional<string> merge_node_id;
 
-    // The commit ID the check suite or pull request (and its check runs) are
+    // The commit ID the branch push or pull request (and its check runs) are
+    // building. This will be the head commit for the branch push as well as
+    // local pull requests and the test merge commit for remote pull requests.
+    //
+    string check_sha;
+
+    // The commit ID the branch push or pull request (and its check runs) are
     // reporting to. Note that in the case of a pull request this will be the
-    // head commit (`pull_request.head.sha`) as opposed to the merge commit.
+    // head commit (`pull_request.head.sha`) as opposed to the test merge
+    // commit.
     //
     string report_sha;
 
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 0ddc75d..ce191ca 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -29,6 +29,8 @@
 //      - check_run (action: rerequested): received when user re-runs a
 //        specific check or all failed checks.
 //
+//        @@ TMP I have confirmed that the above is accurate.
+//
 //      Will need to extract a few more fields from check_runs, but the layout
 //      is very similar to that of check_suite.
 //
@@ -275,8 +277,8 @@ namespace brep
     // Note: "GitHub continues to add new event types and new actions to
     // existing event types." As a result we ignore known actions that we are
     // not interested in and log and ignore unknown actions. The thinking here
-    // is that we want be "notified" of new actions at which point we can decide
-    // whether to ignore them or to handle.
+    // is that we want be "notified" of new actions at which point we can
+    // decide whether to ignore them or to handle.
     //
     // @@ There is also check_run even (re-requested by user, either
     //    individual check run or all the failed check runs).
@@ -360,7 +362,7 @@ namespace brep
         //   A pull request's head branch was updated from the base branch or
         //   new commits were pushed to the head branch. (Note that there is
         //   no equivalent event for the base branch. That case gets handled
-        //   in handle_check_suite_request() instead.)
+        //   in handle_check_suite_request() instead. @@ Not anymore.)
         //
         // Note that both cases are handled the same: we start a new CI
         // request which will be reported on the new commit id.
@@ -372,6 +374,8 @@ namespace brep
         // Ignore the remaining actions by sending a 200 response with empty
         // body.
         //
+        // @@ Ignore known but log unknown, as in check_suite above?
+        //
         return true;
       }
     }
@@ -487,6 +491,8 @@ namespace brep
       }
     }
 
+    // @@ Not anymore (and may not need separate create_pull_request_ci()).
+    //
     // 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 (and, no, GitHub does not invalidate those CI
@@ -526,7 +532,7 @@ namespace brep
       {
         // Recreate each PR's CI request.
         //
-        for (gh_pull_request& pr: *prs)
+        for (const gh_pull_request& pr: *prs)
         {
           service_data prsd (sd.warning_success,
                              sd.installation_access.token,
@@ -567,6 +573,8 @@ namespace brep
         error << "check suite " << nid << " (re-requested): unable to cancel";
     }
 
+    // @@@ Use repo+head ad service id.
+
     // Start CI for the check suite.
     //
     repository_location rl (cs.repository.clone_url + '#' +
@@ -613,6 +621,8 @@ namespace brep
                                build_state::built,
                                move (br)))
       {
+        assert (cr.state == build_state::built);
+
         l3 ([&]{trace << "updated check_run { " << cr << " }";});
       }
       else
@@ -673,34 +683,66 @@ namespace brep
   //   from within the same repository or from a forked repository. The merge
   //   and head commits will be at refs/pull/<PR-number>/{merge,head}.
   //
-  // - New commits are added to PR head branch
+  // - @@ TODO Shared repo model problems
+  //
+  //      The root of all of these problems is that, in the shared repository
+  //      model, for every PR we also receive a check_suite for the commit to
+  //      the head branch. The situation is exacerbated by the fact that the
+  //      PR and CS can arrive in any order.
+  //
+  //       - There will be two CIs running concurrently, building different
+  //         commits: the head commit (check_suite) vs merge commit
+  //         (pull_request).
+  //
+  //       - The CS and PR check_runs will all be added to the same commit
+  //         SHA: pull_request.head.sha, check_suite.head_sha (see above for
+  //         the reasons we don't put the check_runs on the merge
+  //         commit).
+  //
+  //       - Recall that creating a check_run named `A` will effectively
+  //         replace any existing check_runs with that name. They will still
+  //         exist on the GitHub servers but GitHub will only consider the
+  //         latest one (for display in the UI or in determining the
+  //         mergeability of a PR).
+  //
+  //       - Some CS CRs may finish after the corresponding PR CRs, thus
+  //         potentially inverting the true status of a PR (e.g., allow the
+  //         merge of a PR with a bad merge commit).
+  //
+  //   Thus we need a way to prevent any CS check_runs from being updated
+  //   after having received a PR.
   //
-  //   @@ TODO In this case we will end up creating two sets of check runs on
-  //      the same commit (pull_request.head.sha and
-  //      check_suite.head_sha). It's not obvious which to prefer but I'm
-  //      thinking the pull request is more important because in most
-  //      development models it represents something that is more likely to
-  //      end up in an important branch (as opposed to the head of a feature
-  //      branch).
+  //   Problem 1: Create PR from feature branch
   //
-  //      Note that in these two cases we are building different commit (the
-  //      head commit vs merge commit). So it's not clear how we can have
-  //      a single check_suite result represent both?
+  //     - Receive check_suite for commit to feature branch
+  //     - Receive pull_request
   //
-  //   Possible solution: ignore all check_suites with non-empty
-  //   pull_requests[].
+  //     Solution: When receive a PR, fetch all check_suites with that head
+  //     SHA (curl REPO/commits/SHA/check-suites) and cancel their CI
+  //     jobs.
   //
-  //   => check_suite(requested, PR_head) [only in shared repo model]
+  //     Thus there will be no more CS check_run updates. Note however that in
+  //     most cases the PR will be received long enough after the CS for the
+  //     latter's check_runs to all have completed already.
   //
-  //      Note: check_suite.pull_requests[] will contain all PRs with this
-  //      branch as head.
+  //     Note that there will not be a merge CR on the head yet so the PR will
+  //     never be green.
   //
-  //      Note: check_suite.pull_requests[i].head.sha will be the new, updated
-  //      PR head sha.
+  //   Problem 2: New commits are added to PR head branch
   //
-  //   => pull_request(synchronize)
+  //     Note: The check_suite and pull_request can arrive in any order.
   //
-  //   Note: The check_suite and pull_request can arrive in any order.
+  //     - check_suite(requested, PR_head)
+  //
+  //       Note: check_suite.pull_requests[] will contain all PRs with this
+  //       branch as head.
+  //
+  //       Note: check_suite.pull_requests[i].head.sha will be the new,
+  //       updated PR head sha.
+  //
+  //     - pull_request(synchronize)
+  //
+  //     Solution: Ignore all check_suites with non-empty pull_requests[].
   //
   // - New commits are added to PR base branch
   //
@@ -713,6 +755,9 @@ namespace brep
   //
   // - PR closed @@ TODO
   //
+  //   Also received if base branch is deleted. (And presumably same for head
+  //   branch.)
+  //
   //   => pull_request(closed)
   //
   //   Cancel CI?
@@ -779,8 +824,6 @@ namespace brep
     return true;
   }
 
-  // Note: only handles pull requests (not check suites).
-  //
   function<optional<string> (const tenant_service&)> ci_github::
   build_unloaded (tenant_service&& ts,
                   const diag_epilogue& log_writer) const noexcept
@@ -798,6 +841,42 @@ namespace brep
       return nullptr;
     }
 
+    return sd.pre_check
+             ? build_unloaded_pre_check (move (ts), move (sd), log_writer)
+             : build_unloaded_load (move (ts), move (sd), log_writer);
+  }
+
+  function<optional<string> (const tenant_service&)> ci_github::
+  build_unloaded_pre_check (
+    tenant_service&&,
+    service_data&&,
+    const diag_epilogue& log_writer) const noexcept
+  {
+    NOTIFICATION_DIAG (log_writer);
+
+    // Note: PR only (but both local and remove).
+    //
+    // - Ask for test merge commit.
+    // - If not ready, get called again.
+    // - If not mergeable, behind, of different head, cancel itself and ignore.
+    // - Otherwise, create unloaded CI tenant (with proper duplicate mode
+    //   based on re_request) and cancel itself.
+
+    return nullptr;
+  }
+
+  function<optional<string> (const tenant_service&)> ci_github::
+  build_unloaded_load (
+    tenant_service&& ts,
+    service_data&& sd,
+    const diag_epilogue& log_writer) const noexcept
+  {
+    NOTIFICATION_DIAG (log_writer);
+
+    // @@@ TODO: load the tenant: should be the same for both branch push and
+    // PR.
+    //
+
     // Get a new installation access token if the current one has expired.
     //
     const gh_installation_access_token* iat (nullptr);
@@ -907,6 +986,8 @@ namespace brep
                                build_state::built,
                                move (br)))
       {
+        assert (cr.state == build_state::built);
+
         return cr;
       }
       else
@@ -1579,7 +1660,10 @@ namespace brep
           if (cr.state == build_state::built)
           {
             if (conclusion)
+            {
+              assert (cr.status);
               *conclusion |= *cr.status;
+            }
           }
           else
             conclusion = nullopt;
@@ -1602,8 +1686,6 @@ namespace brep
         if (scr->state == build_state::built)
           return nullptr;
 
-        // Don't move from scr because we search sd.check_runs below.
-        //
         cr = move (*scr);
       }
       else
@@ -1788,7 +1870,7 @@ namespace brep
         }
       }
 
-      if (cr.state == build_state::built)
+      if (cr.state_synced)
       {
         // Check run was created/updated successfully to built.
         //
@@ -1798,6 +1880,27 @@ 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?
+        //
+        //      @@ 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.
+        //
         cr.status = b.status;
 
         // Update the conclusion check run if all check runs are now built.
@@ -1831,6 +1934,8 @@ namespace brep
                                    build_state::built,
                                    move (br)))
           {
+            assert (cr.state == build_state::built);
+
             l3 ([&]{trace << "updated check_run { " << cr << " }";});
           }
           else
@@ -1918,7 +2023,7 @@ namespace brep
                    *build_db_, move (ts),
                    chrono::seconds (30) /* interval */,
                    chrono::seconds (0)  /* delay */)
-      .has_value ();
+      .first.has_value (); // @@ TODO HACKED AROUND
   }
 
   string ci_github::
diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx
index 489aac7..e919065 100644
--- a/mod/mod-ci-github.hxx
+++ b/mod/mod-ci-github.hxx
@@ -46,6 +46,16 @@ namespace brep
     build_unloaded (tenant_service&&,
                     const diag_epilogue& log_writer) const noexcept override;
 
+    function<optional<string> (const tenant_service&)>
+    build_unloaded_pre_check (tenant_service&&,
+                              service_data&&,
+                              const diag_epilogue&) const noexcept;
+
+    function<optional<string> (const tenant_service&)>
+    build_unloaded_load (tenant_service&&,
+                         service_data&&,
+                         const diag_epilogue&) const noexcept;
+
     virtual function<optional<string> (const tenant_service&)>
     build_queued (const tenant_service&,
                   const vector<build>&,
diff --git a/mod/tenant-service.hxx b/mod/tenant-service.hxx
index c46cb7b..6a982ea 100644
--- a/mod/tenant-service.hxx
+++ b/mod/tenant-service.hxx
@@ -127,7 +127,8 @@ namespace brep
 
   // This notification is only made on unloaded CI requests created with the
   // ci_start::create() call and until they are loaded with ci_start::load()
-  // or, alternatively, abandoned with ci_start::abandon().
+  // or, alternatively, abandoned with ci_start::cancel() (in which case the
+  // returned callback should be NULL).
   //
   // Note: make sure the implementation of this notification does not take
   // too long (currently 40 seconds) to avoid nested notifications. Note