diff options
| -rw-r--r-- | mod/mod-ci-github-service-data.cxx | 5 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.hxx | 19 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 106 | ||||
| -rw-r--r-- | mod/mod-ci-github.hxx | 19 |
4 files changed, 81 insertions, 68 deletions
diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx index c5bf6a4..3812bf7 100644 --- a/mod/mod-ci-github-service-data.cxx +++ b/mod/mod-ci-github-service-data.cxx @@ -102,14 +102,15 @@ namespace brep timestamp iat_ea, uint64_t iid, string rid, - string rs, + string hs, bool rr) : kind (local), pre_check (false), re_request (rr), warning_success (ws), installation_access (move (iat_tok), iat_ea), installation_id (iid), repository_node_id (move (rid)), - report_sha (move (rs)) + check_sha (hs), + report_sha (move (hs)) { } diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx index 462e7f7..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. @@ -66,6 +74,8 @@ namespace brep // Kind and phase. // + // @@ TODO Serialize these fields. + // enum {local, remote /*, queue */} kind; bool pre_check; bool re_request; // Re-requested (rebuild). @@ -129,12 +139,15 @@ namespace brep // The check_suite constructor. // + // Note that check_sha and report_sha are both the SHA of the + // check_suite's head commit. + // service_data (bool warning_success, string iat_token, timestamp iat_expires_at, uint64_t installation_id, string repository_node_id, - string report_sha, + string head_sha, bool re_request); // The pull_request constructor. diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index 84633fd..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, @@ -493,8 +495,15 @@ namespace brep // Create an unloaded CI request. // + // Note: use no delay since we need to (re)create the synthetic conclusion + // check run as soon as possible. + // // Note that we use the create() API instead of start() since duplicate - // management is no available in start(). + // management is not available in start(). + // + // After this call we will start getting the build_unloaded() + // notifications until (1) we load the request, (2) we cancel it, or (3) + // it gets archived after some timeout. // auto pr (create (error, warn, @@ -508,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 && @@ -524,6 +533,8 @@ namespace brep // High-level description of pull request (PR) handling // + // @@ TODO Update these comments. + // // - Some GitHub pull request terminology: // // - Fork and pull model: Pull requests are created in a forked @@ -681,30 +692,61 @@ namespace brep l3 ([&]{trace << "installation_access_token { " << *iat << " }";}); + // 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. Let's do. + + // @@ TODO: what happens when the entire PR build is re-requested? + + // @@ TODO Serialize the new service_data fields! + // + // Note that check_sha will be set later, in build_unloaded_pre_check(). + // service_data sd (warning_success, move (iat->token), iat->expires_at, pr.installation.id, move (pr.repository.node_id), - pr.pull_request.head_sha, - pr.repository.clone_url, + move (pr.pull_request.head_sha) /* report_sha */, + move (pr.repository.clone_url), pr.pull_request.number); - // Create unloaded CI request. Cancel the existing CI request first if the - // head branch has been updated (action is `synchronize`). + 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). // - // After this call we will start getting the build_unloaded() - // notifications until (1) we load the request, (2) we cancel it, or (3) - // it gets archived after some timeout. + // 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() or as another PR). // - bool cancel_first (pr.action == "synchronize"); + tenant_service ts ("", "ci-github", sd.json ()); - if (!create_pull_request_ci (error, warn, trace, - sd, pr.pull_request.node_id, - cancel_first)) + // 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() -- + // until we cancel the request or it gets archived after some + // timeout. (Note that we never actually load this request, we always + // cancel it; see build_unloaded_pre_check() for details.) + // + if (!create (error, + warn, + &trace, + *build_db_, + move (ts), + chrono::seconds (30) /* interval */, + 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; @@ -749,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; } @@ -1881,36 +1929,6 @@ namespace brep }; } - bool ci_github:: - create_pull_request_ci (const basic_mark& error, - const basic_mark& warn, - const basic_mark& trace, - const service_data& sd, - const string& nid, - bool cf) const - { - // Cancel the existing CI request if asked to do so. Ignore failure - // because the request may already have been cancelled for other reasons. - // - if (cf) - { - if (!cancel (error, warn, &trace, *build_db_, "ci-github", nid)) - l3 ([&] {trace << "unable to cancel CI for pull request " << nid;}); - } - - // Create a new unloaded CI request. - // - tenant_service ts (nid, "ci-github", sd.json ()); - - // Note: use no delay since we need to (re)create the synthetic merge - // check run as soon as possible. - // - return create (error, warn, &trace, - *build_db_, move (ts), - chrono::seconds (30) /* interval */, - chrono::seconds (0) /* delay */).has_value (); - } - string ci_github:: details_url (const build& b) const { diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx index e919065..8284f7f 100644 --- a/mod/mod-ci-github.hxx +++ b/mod/mod-ci-github.hxx @@ -91,25 +91,6 @@ namespace brep bool handle_pull_request (gh_pull_request_event, bool warning_success); - // Create an unloaded CI request for a pull request. If `cancel_first` is - // 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 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) - // it gets archived after some timeout. - // - bool - create_pull_request_ci (const basic_mark& error, - const basic_mark& warn, - const basic_mark& trace, - const service_data&, - const string& pull_request_node_id, - bool cancel_first) const; - // Build a check run details_url for a build. // string |