From de8de9508bf9751dec924e087d0e1cc22128b709 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Fri, 31 May 2024 11:58:13 +0200 Subject: Review --- mod/mod-ci-github.cxx | 99 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 73 insertions(+), 26 deletions(-) diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index 0bcaa3d..b7c271c 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -359,13 +359,17 @@ namespace brep if (pr.action == "opened" || pr.action == "synchronize") { - // opened: A pull request was opened. + // opened + // A pull request was opened. // - // synchronize: 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.) + // synchronize + // 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.) + // + // Note that both cases are handled the same: we start a new CI + // request which will be reported on the new commit id. // return handle_pull_request (move (pr), warning_success); } @@ -669,10 +673,46 @@ namespace brep if (iat == nullptr) return nullptr; // Try again on the next call. - // Create an in-progress check run. Return the check run on success or - // nullopt on failure. + auto make_iat_updater = [] () + { + function (const tenant_service&)> r; + + if (new_iat) + { + r = [&error, + iat = move (new_iat)] (const tenant_service& ts) + -> optional + { + // NOTE: this lambda may be called repeatedly (e.g., due to + // transaction being aborted) and so should not move out of its + // captures. + + service_data sd; + try + { + sd = service_data (*ts.data); + } + catch (const invalid_argument& e) + { + error << "failed to parse service data: " << e; + return nullopt; + } + + sd.installation_access = *iat; + + return sd.json (); + }; + } + + return r; + }; + + // Create a synthetic check run with an in-progress state. Return the + // check run on success or nullopt on failure. // - auto create_cr = [iat, &sd, &error] (string name) -> optional + auto create_synthetic_cr = [iat, + &sd, + &error] (string name) -> optional { check_run cr; cr.name = move (name); @@ -682,7 +722,7 @@ namespace brep iat->token, sd.repository_node_id, sd.report_sha, - nullopt, // details_url + nullopt /* details_url */, build_state::building)) { return cr; @@ -691,14 +731,15 @@ namespace brep return nullopt; }; - // Update a check run with success or failure. Return the check run on - // success or nullopt on failure. + // Update a synthetic check run with success or failure. Return the check + // run on success or nullopt on failure. // - auto update_cr = [iat, &sd, &error] (const string& node_id, + auto update_synthetic_cr = [iat, + &sd, + &error] (const string& node_id, const string& name, result_status rs, - string summary) - -> optional + string summary) -> optional { assert (!node_id.empty ()); @@ -715,7 +756,7 @@ namespace brep iat->token, sd.repository_node_id, node_id, - nullopt, // details_url + nullopt /* details_url */, build_state::built, move (br))) { @@ -753,11 +794,8 @@ namespace brep merge_node_id = move (*cr->node_id); } - // @@ TMP If new_iat.has_value() then we won't store the new IAT. Same - // applies to some of the other cases below. - // else - return nullptr; // Try again on the next call. + return make_iat_updater (); // Try again on the next call. } else merge_node_id = *sd.merge_node_id; @@ -775,7 +813,7 @@ namespace brep // then there is nothing to do. // if (!first) - return nullptr; + return make_iat_updater (); // Fall through to update service data. } @@ -790,7 +828,7 @@ namespace brep if (auto cr = update_cr (merge_node_id, merge_check_run_name, result_status::error, - "Merge would create conflicts")) + "GitHub is unable to create test merge commit")) { l3 ([&]{trace << "updated check_run { " << *cr << " }";}); @@ -813,7 +851,7 @@ namespace brep // so that we can try again on the next call. if (!first) - return nullptr; + return make_iat_updater (); // Fall through to update service data. } @@ -888,8 +926,10 @@ namespace brep { // Update merge check run to successful. // - if (auto cr = update_cr (merge_node_id, merge_check_run_name, - result_status::success, "Merge would succeed")) + if (auto cr = update_cr (merge_node_id, + merge_check_run_name, + result_status::success, + "GitHub created test merge commit")) { l3 ([&]{trace << "updated check_run { " << *cr << " }";}); @@ -900,7 +940,7 @@ namespace brep // #pull/28/merge@1b6c9a361086ed93e6f1e67189e82d52de91c49b // repository_location rl (*sd.repository_clone_url + "#pull/" + - to_string (*sd.pr_number) + "/merge@" + *mc, + to_string (*sd.pr_number) + "/merge@" + *mc, repository_type::git); optional r ( @@ -921,6 +961,13 @@ namespace brep { l3 ([&]{trace << "updated check_run { " << *cr << " }";}); } + else + { + // Nothing really we can do in this case since we will not receive + // any further notifications. + } + + return nullptr; // No need to update service data in this case. } } else -- cgit v1.1