diff options
Diffstat (limited to 'mod')
| -rw-r--r-- | mod/mod-ci-github-gq.cxx | 12 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.cxx | 45 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 352 |
3 files changed, 218 insertions, 191 deletions
diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx index 54822ae..d8a1a0e 100644 --- a/mod/mod-ci-github-gq.cxx +++ b/mod/mod-ci-github-gq.cxx @@ -575,7 +575,7 @@ namespace brep return os.str (); } - pair<optional<gq_pr_mergeability>, bool> + optional<string> gq_pull_request_mergeable (const basic_mark& error, const string& iat, const string& nid) @@ -595,7 +595,7 @@ namespace brep // The response value. Absent if the merge commit is still being // generated. // - optional<gq_pr_mergeability> value; + optional<string> value; resp (json::parser& p) { @@ -617,11 +617,11 @@ namespace brep string oid (p.next_expect_member_string ("oid")); p.next_expect (event::end_object); - value = {true, move (oid)}; + value = move (oid); } else if (ma == "CONFLICTING") { - value = {false, ""}; + value = ""; } else if (ma == "UNKNOWN") { @@ -648,7 +648,7 @@ namespace brep if (!rs.found) error << "pull request '" << nid << "' not found"; - return {move (rs.value), rs.found}; + return move (rs.value); } else error << "failed to fetch pull request: error HTTP response status " @@ -679,7 +679,7 @@ namespace brep error << "unable to fetch pull request: " << e; } - return {nullopt, false}; + return nullopt; } // bool diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx index 8c6970c..7fd554d 100644 --- a/mod/mod-ci-github-service-data.cxx +++ b/mod/mod-ci-github-service-data.cxx @@ -43,12 +43,18 @@ namespace brep { string* s (p.next_expect_member_string_null ("repository_clone_url")); - if (s) + if (s != nullptr) repository_clone_url = *s; } pr_number = p.next_expect_member_number_null<uint32_t> ("pr_number"); + { + string* s (p.next_expect_member_string_null ("merge_node_id")); + if (s != nullptr) + merge_node_id = *s; + } + head_sha = p.next_expect_member_string ("head_sha"); p.next_expect_member_array ("check_runs"); @@ -72,6 +78,12 @@ namespace brep p.next_expect (event::end_object); } + { + string* s (p.next_expect_member_string_null ("conclusion_node_id")); + if (s != nullptr) + conclusion_node_id = *s; + } + p.next_expect (event::end_object); } @@ -81,9 +93,24 @@ namespace brep timestamp iat_ea, uint64_t iid, string rid, + string hs) + : warning_success (ws), + installation_access (move (iat_tok), iat_ea), + installation_id (iid), + repository_node_id (move (rid)), + head_sha (move (hs)) + { + } + + service_data:: + service_data (bool ws, + string iat_tok, + timestamp iat_ea, + uint64_t iid, + string rid, string hs, - optional<string> rcu, - optional<uint32_t> prn) + string rcu, + uint32_t prn) : warning_success (ws), installation_access (move (iat_tok), iat_ea), installation_id (iid), @@ -128,6 +155,12 @@ namespace brep else s.value (nullptr); + s.member_name ("merge_node_id"); + if (merge_node_id) + s.value (*merge_node_id); + else + s.value (nullptr); + s.member ("head_sha", head_sha); s.member_begin_array ("check_runs"); @@ -150,6 +183,12 @@ namespace brep } s.end_array (); + s.member_name ("conclusion_node_id"); + if (conclusion_node_id) + s.value (*conclusion_node_id); + else + s.value (nullptr); + s.end_object (); return b; diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index f9fd453..be9daa9 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -650,15 +650,89 @@ namespace brep if (iat == nullptr) return nullptr; - bool first (...); // @@ TODO + // Synthetic merge check run node ID. Absent if the check run was created + // in a previous call (and has already been stored in the service data). + // + optional<string> mni; + + // Create an in-progress check run. Return the check run on success or + // nullopt on failure. + // + auto create_cr = [iat, &sd, &error] (string name) -> optional<check_run> + { + check_run cr; + cr.name = move (name); + + if (gq_create_check_run (error, + cr, + iat->token, + sd.repository_node_id, + sd.head_sha, + // @@ TODO What details URL to use? + "https://build2.org", // details URL. + build_state::building)) + { + return cr; + } + else + return nullopt; + }; + + // Update the merge check run with success or failure. Return the check + // run on success or nullopt on failure. + // + auto update_merge_cr = [iat, &sd, &mni, &error] (result_status rs, + const string& summary) + -> optional<check_run> + { + optional<gq_built_result> br ( + gq_built_result (gh_to_conclusion (rs, sd.warning_success), + circle (rs) + ' ' + ucase (to_string (rs)), + move (summary))); + + check_run cr; + cr.name = "merge-commit"; // For display purposes only. + + const string& ni (mni ? *mni : *sd.merge_node_id); // Node ID. + + if (gq_update_check_run (error, + cr, + iat->token, + sd.repository_node_id, + ni, + "https://build2.org", // details URL. + build_state::built, + move (br))) + { + return cr; + } + else + return nullopt; + }; + + // True if this is the first call (or the merge commit couldn't be created + // on the first call). + // + bool first (!sd.merge_node_id); // If this is the first call, (re)create the synthetic merge check run as // soon as possible to make sure the previous check suite, if any, is no // longer completed. // + // @@ TMP There is still a window between receipt of the pull_request + // event and the first bot/worker asking for a task. That could be + // minutes, right? + // if (first) { - // TODO: in-progress + if (auto cr = create_cr ("merge-commit")) + { + l3 ([&]{trace << "created check_run { " << *cr << " }";}); + + mni = move (cr->node_id); + } + else + return nullptr; // Try again on the next call. } // Start/check PR mergeability. @@ -668,7 +742,7 @@ namespace brep if (!mc) // No merge commit yet. { - // If this is a subseqent notification and there is no merge commit, + // If this is a subsequent notification and there is no merge commit, // then there is nothing to do. // if (!first) @@ -679,217 +753,139 @@ namespace brep // return [&error, iat = move (new_iat), - cr = move (cr), - ccr = move (ccr)] (const tenant_service& ts) -> optional<string> + mni = move (mni)] (const tenant_service& ts) -> optional<string> { - // @@ TODO - } + // 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; + } + + if (iat) + sd.installation_access = *iat; + + if (mni) + sd.merge_node_id = *mni; + + return sd.json (); + }; } - else if (mc->empty ()) // Not meargable. + else if (mc->empty ()) // Not mergeable. { - // If the commit is not mergable, cancel the CI request and fail the + // If the commit is not mergeable, cancel the CI request and fail the // merge check run. // // Note that it feels like in this case we don't really need to create a // failed synthetic conclusion check run since the PR cannot be merged // anyway. - // @@ TODO: cancel CI request - // @@ TODO: fail merge check run and update in service data. + if (auto cr = update_merge_cr (result_status::error, + "merge would create conflicts")) + { + l3 ([&]{trace << "updated check_run { " << *cr << " }";}); + + // @@ TODO: cancel CI request + } + // Don't cancel the CI request if the merge check run update failed so + // that we can try again on the next call. return [&error, iat = move (new_iat), - cr = move (cr), - ccr = move (ccr)] (const tenant_service& ts) -> optional<string> + mni = move (mni)] (const tenant_service& ts) -> optional<string> { - // @@ TODO - } - } - - // If we are here, then it means we have a merge commit that we can load. - // - - // As a first step, (re)create the synthetic conclusion check run and then - // change the merge check run state to success. Do it in this order so - // that the check suite does not become completed. - - // @@ TODO: create synthetic conclusion check run (in progress) - // @@ TODO: update synthetic merge checj run (success) - // @@ TODO: will need to update conclusion check run node_id in service data + // NOTE: this lambda may be called repeatedly (e.g., due to + // transaction being aborted) and so should not move out of its + // captures. - // @@ TODO: load CI request. + service_data sd; + try + { + sd = service_data (*ts.data); + } + catch (const invalid_argument& e) + { + error << "failed to parse service data: " << e; + return nullopt; + } - { - pair<optional<gq_pr_mergeability>, bool> pr ( - gq_pull_request_mergeable (error, iat->token, ts.id)); + if (iat) + sd.installation_access = *iat; - if (pr.second) // No errors. - { - if (pr.first) // Merge commit has been generated. - ma = move (pr.first); - } - else - { - error << "failed to get pull request " << ts.id << " mergeability"; + if (mni) + sd.merge_node_id = mni; - return nullptr; - } + return sd.json (); + }; } - // The merge commit's build state. Note that the PR fetch above initiated - // its generation on GitHub. - // - build_state bs; - - // The check run result to use if the merge commit has been calculated, or - // absent if it has not. + // If we are here, then it means we have a merge commit that we can load. // - optional<gq_built_result> br; - - if (ma) - { - bs = build_state::built; - - result_status rs; - string sm; // Summary. - - if (ma->mergeable) - { - rs = result_status::success; - sm = "merge would succeed"; - } - else - { - rs = result_status::error; - sm = "merge would create conflicts"; - } - - br = gq_built_result (gh_to_conclusion (rs, sd.warning_success), - circle (rs) + ' ' + ucase (to_string (rs)), - move (sm)); - } - else - bs = build_state::building; - // The stored merge commit check run. - // - check_run* scr (sd.find_check_run ("merge-commit")); + // As a first step, (re)create the synthetic conclusion check run and then + // change the merge check run state to success. Do it in this order so + // that the check suite does not become completed. - // The updated merge commit check run. + // Conclusion check run node ID. Absent if the check run was created in a + // previous call (and has already been stored in the service data). // - check_run cr; + optional<string> cni; - if (scr == nullptr || !scr->node_id) + if (!sd.conclusion_node_id) { - // Create the check run because the merge commit check run has not - // previously been stored, or we failed to create it last time we - // tried. - // - // @@ TMP There is still a window between receipt of the pull_request - // event and the first bot/worker asking for a task. Shouldn't we - // rather create the merge CR when we create the unloaded CI - // build (in the pull_request handler)? - // - if (scr != nullptr) - cr = move (*scr); - else + if (auto cr = create_cr ("conclusion")) { - cr.build_id = "merge-commit"; - cr.name = cr.build_id; - } - cr.state_synced = false; + l3 ([&]{trace << "created check_run { " << *cr << " }";}); - if (gq_create_check_run (error, - cr, - iat->token, - sd.repository_node_id, - sd.head_sha, - // @@ TODO What details URL to use? - "https://build2.org", // details URL. - bs, - move (br))) - { - l3 ([&]{trace << "created check_run { " << cr << " }";}); + cni = move (cr->node_id); } } - else if (ma) - { - // Update because the merge commit check run was previously stored and - // the merge commit result has become available. - // - cr = move (*scr); - cr.state_synced = false; - if (gq_update_check_run (error, - cr, - iat->token, - sd.repository_node_id, - *cr.node_id, - "https://build2.org", // details URL. - bs, - move (br))) - { - l3 ([&]{trace << "updated check_run { " << cr << " }";}); - } - } - else + if (cni || sd.conclusion_node_id) { - // Do nothing because nothing has changed. + // Update merge check run to successful. // - return nullptr; - } - - // Create the conclusion check run and load the CI request. - // - optional<check_run> ccr; // Conclusion check run. - - if (ma && ma->mergeable) - { - // Create the conclusion check run if the merge commit shows the PR is - // mergeable. + // @@ TMP If the CI load below failed on the previous call we probably + // don't want to update the CR again but we can't tell the difference + // (no state for the merge CR). However it should be a no-op due to + // the GitHub API's semantics so it's probably OK? // - // @@ TMP We could do this later if we prefer: if the branch protection - // rule requires the conclusion check run then the PR will not go - // green until the conclusion check run is successful. - // - ccr = check_run (); - ccr->build_id = "conclusion"; - ccr->name = ccr->build_id; - ccr->state_synced = false; - - if (gq_create_check_run (error, - *ccr, - iat->token, - sd.repository_node_id, - sd.head_sha, - // @@ TODO What details URL to use? - "https://build2.org", // details URL. - build_state::queued)) + if (auto cr = update_merge_cr (result_status::success, + "merge would succeed")) { - l3 ([&]{trace << "created check_run { " << *ccr << " }";}); - } + l3 ([&]{trace << "updated check_run { " << *cr << " }";}); - // Load the CI request. - // - repository_location rl (*sd.repository_clone_url + "#refs/pull/" + + // Load the CI request. + // + repository_location rl (*sd.repository_clone_url + "#refs/pull/" + to_string (*sd.pr_number) + "/merge", - repository_type::git); + repository_type::git); - optional<start_result> r ( - load (error, warn, &trace, *build_db_, move (ts), rl)); + optional<start_result> r ( + load (error, warn, &trace, *build_db_, move (ts), rl)); - if (!r) - { - error << "unable to load CI request"; + if (!r) + { + error << "unable to load CI request "; - // @@ TODO Handle this. Will get called again and in those cases do - // nothing except load the CI request. + // Try again next call. + } } + // Don't load the CI request if the merge check run update failed so + // that we can try again on the next call. } - return - [&error, iat = move (new_iat), cr = move (cr), ccr = move (ccr)] ( - const tenant_service& ts) -> optional<string> + return [&error, + iat = move (new_iat), + cni = move (cni)] (const tenant_service& ts) -> optional<string> { // NOTE: this lambda may be called repeatedly (e.g., due to // transaction being aborted) and so should not move out of its @@ -909,16 +905,8 @@ namespace brep if (iat) sd.installation_access = *iat; - if (check_run* scr = sd.find_check_run (cr.build_id)) - *scr = cr; - else - sd.check_runs.push_back (cr); - - if (ccr) - { - assert (sd.find_check_run (ccr->build_id) == nullptr); - sd.check_runs.push_back (*ccr); - } + if (cni) + sd.conclusion_node_id = cni; return sd.json (); }; |