diff options
| -rw-r--r-- | mod/mod-ci-github-service-data.cxx | 13 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.hxx | 3 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 230 |
3 files changed, 70 insertions, 176 deletions
diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx index 0f6e593..c5bf6a4 100644 --- a/mod/mod-ci-github-service-data.cxx +++ b/mod/mod-ci-github-service-data.cxx @@ -94,14 +94,18 @@ namespace brep p.next_expect (event::end_object); } + // check_suite constructor. + // service_data:: service_data (bool ws, string iat_tok, timestamp iat_ea, uint64_t iid, string rid, - string rs) - : warning_success (ws), + string rs, + 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)), @@ -109,6 +113,8 @@ namespace brep { } + // pull_request constructor. + // service_data:: service_data (bool ws, string iat_tok, @@ -118,7 +124,8 @@ namespace brep string rs, string rcu, uint32_t prn) - : warning_success (ws), + : kind (local), pre_check (true), re_request (false), + warning_success (ws), installation_access (move (iat_tok), iat_ea), installation_id (iid), repository_node_id (move (rid)), diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx index 89fcaab..462e7f7 100644 --- a/mod/mod-ci-github-service-data.hxx +++ b/mod/mod-ci-github-service-data.hxx @@ -134,7 +134,8 @@ namespace brep timestamp iat_expires_at, uint64_t installation_id, string repository_node_id, - string report_sha); + string report_sha, + bool re_request); // The pull_request constructor. // diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index ce191ca..20c9dc3 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -441,6 +441,10 @@ namespace brep l3 ([&]{trace << "check_suite event { " << cs << " }";}); + // While we don't need the installation access token in this request, + // let's obtain it to flush out any permission issues early. Also, it is + // valid for an hour so we will most likely make use of it. + // optional<string> jwt (generate_jwt (trace, error)); if (!jwt) throw server_error (); @@ -449,188 +453,70 @@ namespace brep obtain_installation_access_token (cs.installation.id, move (*jwt), error)); - if (!iat) throw server_error (); l3 ([&]{trace << "installation_access_token { " << *iat << " }";}); + // @@ What happens if we call this functions with an already existing + // node_id (e.g., replay attack). See the UUID header above. + // + + // While it would have been nice to cancel CIs of PRs with this branch as + // base not to waste resources, there are complications: Firsty, we can + // only do this for remote PRs (since local PRs may share the result with + // branch push). Secondly, we try to do our best even if the branch + // 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. + // + string sid (cs.repository.node_id + ":" + cs.check_suite.head_sha); + service_data sd (warning_success, iat->token, iat->expires_at, cs.installation.id, move (cs.repository.node_id), - move (cs.check_suite.head_sha)); - - // Create the conclusion check run. - // - { - check_run cr; - cr.name = conclusion_check_run_name; - - if (gq_create_check_run (error, - cr, - iat->token, - sd.repository_node_id, - sd.report_sha, - nullopt /* details_url */, - build_state::building)) - { - l3 ([&]{trace << "created check_run { " << cr << " }";}); - - sd.conclusion_node_id = move (cr.node_id); - } - else - { - // We could try to carry on in this case by either updating or - // creating this conclusion check run later. But let's not complicate - // things for now. - // - fail << "check suite " << cs.check_suite.node_id - << ": unable to create conclusion check run"; - } - } + move (cs.check_suite.head_sha), + cs.action == "rerequested" /* re_request */); - // @@ Not anymore (and may not need separate create_pull_request_ci()). + // 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 + // already exists (which could've been created by handle_pull_request()). // - // 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 - // results automatically; see below). + // Note that GitHub UI does not allow re-running the entire check suite + // until all the check runs are completed. // - // Unfortunately GitHub does not provide a webhook for PR base branch - // updates (as it does for PR head branch updates) so we have to handle it - // here. We do so by fetching the open pull requests with this branch as - // base branch and then recreating the CI requests (cancel existing, - // create new) for each pull request. - // - // If we fail to recreate any of the PR CI requests, they and their check - // runs will be left reflecting outdated merge commits. If the new merge - // commit failed to be generated (merge conflicts) the PR will not be - // mergeable which is not entirely catastrophic. But on the other hand, if - // all of the existing CI request's check runs have already succeeded and - // the new merge commit succeeds (no conflicts) with logic errors then a - // user would be able to merge a broken PR. - // - // Regardless of the nature of the error, we have to let the check suite - // handling code proceed so we only issue diagnostics. Note also that we - // want to run this code as early as possible to minimize the window of - // the user seeing misleading CI results. - // - if (cs.action == "requested") - { - // Fetch open pull requests with the check suite's head branch as base - // branch. - // - optional<vector<gh_pull_request>> prs ( - gq_fetch_open_pull_requests (error, - iat->token, - sd.repository_node_id, - cs.check_suite.head_branch)); + duplicate_tenant_mode dtm (sd.re_request ? duplicate_tenant_mode::replace + : duplicate_tenant_mode::ignore); - if (prs) - { - // Recreate each PR's CI request. - // - for (const gh_pull_request& pr: *prs) - { - service_data prsd (sd.warning_success, - sd.installation_access.token, - sd.installation_access.expires_at, - sd.installation_id, - sd.repository_node_id, - pr.head_sha, - cs.repository.clone_url, - pr.number); - - // Cancel the existing CI request and create a new unloaded CI - // request. 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. - // - if (!create_pull_request_ci (error, warn, trace, - prsd, pr.node_id, - true /* cancel_first */)) - { - error << "pull request " << pr.node_id - << ": unable to create unloaded CI request"; - } - } - } - else - { - error << "unable to fetch open pull requests with base branch " - << cs.check_suite.head_branch; - } - } - // Cancel existing CI request if this check suite is being re-run. + // Create an unloaded CI request. + // + // Note that we use the create() API instead of start() since duplicate + // management is no available in start(). // - else if (cs.action == "rerequested") + auto pr (create (error, + warn, + verb_ ? &trace : nullptr, + *build_db_, + tenant_service (sid, "ci-github", sd.json ()), + chrono::seconds (30) /* interval */, + chrono::seconds (0) /* delay */, + dtm)); + + if (!pr.first) { - const string& nid (cs.check_suite.node_id); - - if (!cancel (error, warn, &trace, *build_db_, "ci-github", nid)) - error << "check suite " << nid << " (re-requested): unable to cancel"; + fail << "check suite " << cs.check_suite.node_id + << ": unable to create unloaded CI request"; } - // @@@ Use repo+head ad service id. - - // Start CI for the check suite. - // - repository_location rl (cs.repository.clone_url + '#' + - cs.check_suite.head_branch, - repository_type::git); - - // @@ What happens if we call this functions with an already existing - // node_id (e.g., replay attack). See the UUID header above. - // - optional<start_result> r ( - start (error, - warn, - verb_ ? &trace : nullptr, - tenant_service (cs.check_suite.node_id, "ci-github", sd.json ()), - move (rl), - vector<package> {}, - nullopt, /* client_ip */ - nullopt /* user_agent */)); - - if (!r || r->status != 200) + if (dtm == duplicate_tenant_mode::replace && + pr.second == duplicate_tenant_result::created) { - // Update the conclusion check run with failure. - // - result_status rs (result_status::error); - - optional<gq_built_result> br ( - gq_built_result (gh_to_conclusion (rs, sd.warning_success), - circle (rs) + ' ' + ucase (to_string (rs)), - to_check_run_summary (r))); - - check_run cr; - - // Set some fields for display purposes. - // - cr.node_id = *sd.conclusion_node_id; - cr.name = conclusion_check_run_name; - - if (gq_update_check_run (error, - cr, - iat->token, - sd.repository_node_id, - *sd.conclusion_node_id, - nullopt /* details_url */, - build_state::built, - move (br))) - { - assert (cr.state == build_state::built); - - l3 ([&]{trace << "updated check_run { " << cr << " }";}); - } - else - { - fail << "check suite " << cs.check_suite.node_id - << ": unable to update conclusion check_run " - << *sd.conclusion_node_id; - } + error << "check suite " << cs.check_suite.node_id + << ": re-requested but tenant_service with id " << sid + << " did not exist"; } return true; @@ -847,10 +733,9 @@ namespace brep } function<optional<string> (const tenant_service&)> ci_github:: - build_unloaded_pre_check ( - tenant_service&&, - service_data&&, - const diag_epilogue& log_writer) const noexcept + build_unloaded_pre_check (tenant_service&&, + service_data&&, + const diag_epilogue& log_writer) const noexcept { NOTIFICATION_DIAG (log_writer); @@ -858,7 +743,9 @@ namespace brep // // - Ask for test merge commit. // - If not ready, get called again. - // - If not mergeable, behind, of different head, cancel itself and ignore. + // - 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. @@ -866,10 +753,9 @@ namespace brep } function<optional<string> (const tenant_service&)> ci_github:: - build_unloaded_load ( - tenant_service&& ts, - service_data&& sd, - const diag_epilogue& log_writer) const noexcept + build_unloaded_load (tenant_service&& ts, + service_data&& sd, + const diag_epilogue& log_writer) const noexcept { NOTIFICATION_DIAG (log_writer); |