diff options
| -rw-r--r-- | mod/mod-ci-github-gq.hxx | 21 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.hxx | 5 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 105 |
3 files changed, 73 insertions, 58 deletions
diff --git a/mod/mod-ci-github-gq.hxx b/mod/mod-ci-github-gq.hxx index f52299c..2c1704f 100644 --- a/mod/mod-ci-github-gq.hxx +++ b/mod/mod-ci-github-gq.hxx @@ -90,7 +90,7 @@ namespace brep // // Return absent value if the merge commit is still being generated (which // means PR head branch behindness is not yet known either). See the - // gq_pr_pre_check struct's field comments for non-absent return value + // gq_pr_pre_check struct's member comments for non-absent return value // semantics. // // Issue diagnostics and return absent if the request failed (which means it @@ -100,9 +100,9 @@ namespace brep // merge commit. (For details see // https://docs.github.com/rest/guides/getting-started-with-the-git-database-api#checking-mergeability-of-pull-requests.) // - struct gq_pr_pre_check + struct gq_pr_pre_check_info { - // The PR head commit ID. + // The PR head commit id. // string head_sha; @@ -110,16 +110,17 @@ namespace brep // bool behind; - // The commit ID of the test merge commit. Empty if behind or the PR is + // The commit id of the test merge commit. Absent if behind or the PR is // not auto-mergeable. // - string merge_commit_sha; + optional<string> merge_commit_sha; }; - optional<gq_pr_pre_check> - gq_pull_request_pre_check_info (const basic_mark& error, - const string& installation_access_token, - const string& node_id); + optional<gq_pr_pre_check_info> + gq_fetch_pull_request_pre_check_info ( + const basic_mark& error, + const string& installation_access_token, + const string& node_id); // Fetch the last 100 open pull requests with the specified base branch from // the repository with the specified node ID. @@ -127,6 +128,8 @@ namespace brep // Issue diagnostics and return nullopt if the repository was not found or // an error occurred. // + // @@@ Looks like not needed anymore. + // optional<vector<gh_pull_request>> gq_fetch_open_pull_requests (const basic_mark& error, const string& installation_access_token, diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx index 27d4791..c321edc 100644 --- a/mod/mod-ci-github-service-data.hxx +++ b/mod/mod-ci-github-service-data.hxx @@ -92,14 +92,13 @@ namespace brep string repository_node_id; // GitHub-internal opaque repository id. - string event_node_id; // check_suite/pull_request node id. - // The following two are only used for pull requests. // // @@ TODO/LATER: maybe put them in a struct? // - optional<string> repository_clone_url; + optional<string> pr_node_id; optional<uint32_t> pr_number; + optional<string> pr_repository_clone_url; // The GitHub ID of the synthetic PR merge check run or absent if it // hasn't been created yet. diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index 0a9b68a..7da694b 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -469,7 +469,7 @@ namespace brep // protection rule for head behind is not enabled. In this case, it would // be good to complete the CI. So maybe/later. - // Service id that uniquely identifies the CI request. + // Service id that uniquely identifies the CI tenant. // string sid (cs.repository.node_id + ":" + cs.check_suite.head_sha); @@ -483,6 +483,12 @@ namespace brep // In this case we need to load the existing service data for this head // commit, extract the test merge commit, and restart the CI for that. // + // Note that it's possible the base branch has moved in the meantime and + // ideally we would want to re-request the test merge commit, etc. + // However, this will only be necessary if the user does not follow our + // recommendation of enabling the head-behind-base protection. And it + // seems all this extra complexity would not be warranted. + // string check_sha; service_data::kind_type kind; @@ -530,8 +536,8 @@ namespace brep move (check_sha), move (cs.check_suite.head_sha) /* report_sha */); - // If this check suite is being re-run, replace the existing CI request if - // it exists; otherwise create a new one, doing nothing if a request + // If this check suite is being re-run, replace the existing CI tenant if + // it exists; otherwise create a new one, doing nothing if a tenant // already exists (which could've been created by handle_pull_request()). // // Note that GitHub UI does not allow re-running the entire check suite @@ -541,7 +547,7 @@ namespace brep ? duplicate_tenant_mode::replace : duplicate_tenant_mode::ignore); - // Create an unloaded CI request. + // Create an unloaded CI tenant. // // Note: use no delay since we need to (re)create the synthetic conclusion // check run as soon as possible. @@ -550,7 +556,7 @@ namespace brep // 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) + // notifications until (1) we load the tenant, (2) we cancel it, or (3) // it gets archived after some timeout. // auto pr (create (error, @@ -779,7 +785,7 @@ namespace brep move (pr.repository.clone_url), pr.pull_request.number); - // Create an unloaded CI request for the pre-check phase (during which we + // Create an unloaded CI tenant for the pre-check phase (during which we // wait for the PR's merge commit and behindness to become available). // // Create with an empty service id so that the generated tenant id is used @@ -794,9 +800,9 @@ namespace brep // // 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.) + // until we cancel the tenant 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, @@ -842,16 +848,20 @@ namespace brep { NOTIFICATION_DIAG (log_writer); - // Note: PR only (but both local and remote). + // We get here for PRs only (but both local and remote). The overall + // plan is as follows: + // + // 1. Ask for the mergeability/behind status/test merge commit. + // + // 2. If not ready, get called again. + // + // 3. If not mergeable, behind, or different head (head changed while + // waiting for merge commit and thus differs from what's in the + // service_data), cancel the pre-check tenant and do nothing. + // + // 4. Otherwise, create an unloaded CI tenant and cancel ourselves. Note + // that all re-requested cases are handled elsewhere. // - // - Ask for test merge commit. - // - If not ready, get called again. - // - If not mergeable, behind, or different head (head changed while - // waiting for merge commit and thus differs from what's in the - // service_data), cancel itself and ignore. - // - 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 @@ -859,12 +869,12 @@ namespace brep // // Request PR pre-check info (triggering the generation of the test merge - // commit on GitHub's side). + // commit on the GitHub's side). // - optional<gq_pr_pre_check> pc ( // Pre-check information. - gq_pull_request_pre_check_info (error, - sd.installation_access.token, - sd.event_node_id)); + optional<gq_pr_pre_check_info> pc ( + gq_fetch_pull_request_pre_check_info (error, + sd.installation_access.token, + sd.event_node_id)); if (!pc) { @@ -873,7 +883,7 @@ namespace brep return nullptr; } - // Create the CI request if nothing is wrong, otherwise issue diagnostics. + // Create the CI tenant if nothing is wrong, otherwise issue diagnostics. // if (pc->behind) { @@ -892,29 +902,29 @@ namespace brep } else { - // Create the CI request. + // Create the CI tenant. // Update service data's check_sha if this is a remote PR. // if (sd.kind == service_data::remote) sd.check_sha = pc->merge_commit_sha; - // Service id that will uniquely identify the CI request. + // Service id that will uniquely identify the CI tenant. // string sid (sd.repository_node_id + ":" + sd.report_sha); - // Create an unloaded CI request, doing nothing if one already exists + // Create an unloaded CI tenant, doing nothing if one already exists // (which could've been created by a head branch push or another PR // sharing the same head commit). // - // 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 (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 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) + // notifications until (1) we load the tenant, (2) we cancel it, or (3) // it gets archived after some timeout. // if (auto pr = create (error, warn, verb_ ? &trace : nullptr, @@ -932,15 +942,10 @@ namespace brep // is expected, so do nothing. // // If this is a remote PR then it could be anything (branch push, - // local PR, or another remote PR), so we can't recover. This PR - // could go green despite its test merge commit not having been - // CI'd. - // - // @@ TMP Perhaps we should update the conclusion CR to failing - // with a message asking the user to sort the problem out - // manually and then re-request the checks in the UI if they - // want to re-CI the branch push or, if they want to CI one of - // the PRs, they can close and reopen it (action "reopened"). + // local PR, or another remote PR) which in turn means the CI result + // may end up being for head, not merge commit. There is nothing we + // can do about it on our side (the user can enable the head-behing- + // base protection on their side). // if (sd.kind == service_data::remote) { @@ -955,14 +960,11 @@ namespace brep << ": unable to create unloaded CI request " << "with tenant_service id " << sid; - // Retry by bailing out before cancelling in order to get called - // again. - // - return nullptr; + // Fall throught to cancel. } } - // Cancel this, the pre-check request. + // Cancel the pre-check tenant. // if (!cancel (error, warn, verb_ ? &trace : nullptr, *build_db_, ts.type, ts.id)) @@ -984,8 +986,15 @@ namespace brep { NOTIFICATION_DIAG (log_writer); - // @@@ TODO: load the tenant: should be the same for both branch push and - // PR. + // Load the tenant, which is essentially the same for both branch push and + // PR. The overall plan is as follows: + // + // - Create synthetic conclusion check run with the building state. If + // unable to, get called again to re-try. + // + // - Load the tenant. If unable to, fail the conclusion check run. + // + // - Update service data. // // Get a new installation access token if the current one has expired. @@ -1139,6 +1148,9 @@ namespace brep else merge_node_id = *sd.merge_node_id; + // @@@ TMP: move/remove. + // +#if 0 // Start/check PR mergeability. // optional<gq_pr_pre_check> mc ( // Merge commit. @@ -1230,6 +1242,7 @@ namespace brep return sd.json (); }; } +#endif // If we are here, then it means we have a merge commit that we can load. // |