diff options
| -rw-r--r-- | mod/ci-common.cxx | 1 | ||||
| -rw-r--r-- | mod/ci-common.hxx | 6 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.hxx | 12 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 34 |
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; } |