diff options
Diffstat (limited to 'mod/mod-ci-github.cxx')
| -rw-r--r-- | mod/mod-ci-github.cxx | 1042 |
1 files changed, 913 insertions, 129 deletions
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index 9449f97..19798ad 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -5,8 +5,12 @@ #include <libbutl/json/parser.hxx> +#include <web/xhtml/serialization.hxx> +#include <web/server/mime-url-encoding.hxx> // mime_url_encode() + #include <mod/jwt.hxx> #include <mod/hmac.hxx> +#include <mod/build.hxx> // build_log_url() #include <mod/module-options.hxx> #include <mod/mod-ci-github-gq.hxx> @@ -15,32 +19,43 @@ #include <stdexcept> -// @@ TODO +// @@ Remaining TODOs +// +// - Rerequested checks +// +// - check_suite (action: rerequested): received when user re-runs all +// checks. +// +// - check_run (action: rerequested): received when user re-runs a +// specific check or all failed checks. +// +// Will need to extract a few more fields from check_runs, but the layout +// is very similar to that of check_suite. +// +// - Pull requests. Handle +// +// - Choose strong webhook secret (when deploying). // -// Building CI checks with a GitHub App -// https://docs.github.com/en/apps/creating-github-apps/writing-code-for-a-github-app/building-ci-checks-with-a-github-app +// - Check that delivery UUID has not been received before (replay attack). // -// @@ TODO Best practices +// Resources: +// +// Creating an App: +// https://docs.github.com/en/apps/creating-github-apps/about-creating-github-apps/best-practices-for-creating-a-github-app // // Webhooks: // https://docs.github.com/en/webhooks/using-webhooks/best-practices-for-using-webhooks // https://docs.github.com/en/webhooks/using-webhooks/validating-webhook-deliveries // // REST API: -// https://docs.github.com/en/rest/using-the-rest-api/best-practices-for-using-the-rest-api?apiVersion=2022-11-28 +// All docs: https://docs.github.com/en/rest#all-docs +// Best practices: https://docs.github.com/en/rest/using-the-rest-api/best-practices-for-using-the-rest-api // -// Creating an App: -// https://docs.github.com/en/apps/creating-github-apps/about-creating-github-apps/best-practices-for-creating-a-github-app +// GraphQL API: +// Reference: https://docs.github.com/en/graphql/reference // -// Use a webhook secret to ensure request is coming from Github. HMAC: -// https://en.wikipedia.org/wiki/HMAC#Definition. A suitable implementation -// is provided by OpenSSL. -// @@ TODO Centralize exception/error handling around calls to -// github_post(). Currently it's mostly duplicated and there is quite -// a lot of it. -// using namespace std; using namespace butl; using namespace web; @@ -56,7 +71,7 @@ namespace brep ci_github:: ci_github (const ci_github& r, tenant_service_map& tsm) - : handler (r), + : database_module (r), ci_start (r), options_ (r.initialized_ ? r.options_ : nullptr), tenant_service_map_ (tsm) @@ -80,9 +95,12 @@ namespace brep // Prepare for the CI requests handling, if configured. // - if (options_->ci_github_app_webhook_secret_specified ()) + if (options_->build_config_specified () && + options_->ci_github_app_webhook_secret_specified ()) { ci_start::init (make_shared<options::ci_start> (*options_)); + + database_module::init (*options_, options_->build_db_retry ()); } } @@ -93,22 +111,14 @@ namespace brep HANDLER_DIAG; - if (!options_->ci_github_app_webhook_secret_specified ()) - throw invalid_request (404, "GitHub CI request submission disabled"); + if (build_db_ == nullptr) + throw invalid_request (501, "GitHub CI submission not implemented"); // Process headers. // - // @@ TMP Shouldn't we also error<< in some of these header problem cases? - // - // @@ TMP From GitHub docs: "You can create webhooks that subscribe to the - // events listed on this page." - // - // So it seems appropriate to generally use the term "event" (which - // we already do for the most part), and "webhook event" only when - // more context would be useful? - // string event; // Webhook event. string hmac; // Received HMAC. + try { bool content_type (false); @@ -175,11 +185,16 @@ namespace brep if (hmac.empty ()) throw invalid_request (400, "missing x-hub-signature-256 header"); } + catch (const invalid_request& e) + { + error << "request header error: " << e.content; + throw; + } // Read the entire request body into a buffer because we need to compute // an HMAC over it and then parse it as JSON. The alternative of reading - // from the stream twice works out to be more complicated (see also @@ - // TODO item in web/server/module.hxx). + // from the stream twice works out to be more complicated (see also a TODO + // item in web/server/module.hxx). // string body; { @@ -226,6 +241,35 @@ namespace brep fail << "unable to compute request HMAC: " << e; } + // Process the `warning` webhook request query parameter. + // + bool warning_success; + { + const name_values& rps (rq.parameters (1024, true /* url_only */)); + + auto i (find_if (rps.begin (), rps.end (), + [] (auto&& rp) {return rp.name == "warning";})); + + if (i == rps.end ()) + throw invalid_request (400, + "missing 'warning' webhook query parameter"); + + if (!i->value) + throw invalid_request ( + 400, "missing 'warning' webhook query parameter value"); + + const string& v (*i->value); + + if (v == "success") warning_success = true; + else if (v == "failure") warning_success = false; + else + { + throw invalid_request ( + 400, + "invalid 'warning' webhook query parameter value: '" + v + '\''); + } + } + // There is a webhook event (specified in the x-github-event header) and // each event contains a bunch of actions (specified in the JSON request // body). @@ -236,6 +280,11 @@ namespace brep // is that we want be "notified" of new actions at which point we can decide // whether to ignore them or to handle. // + // @@ There is also check_run even (re-requested by user, either + // individual check run or all the failed check runs). + // + // @@ There is also the pull_request event. Probably need to handle. + // if (event == "check_suite") { gh_check_suite_event cs; @@ -257,14 +306,16 @@ namespace brep if (cs.action == "requested") { - return handle_check_suite_request (move (cs)); + return handle_check_suite_request (move (cs), warning_success); } else if (cs.action == "rerequested") { // Someone manually requested to re-run the check runs in this check // suite. Treat as a new request. // - return handle_check_suite_request (move (cs)); + // @@ This is probably broken. + // + return handle_check_suite_request (move (cs), warning_success); } else if (cs.action == "completed") { @@ -272,9 +323,8 @@ namespace brep // completed and a conclusion is available". Looks like this one we // ignore? // - // @@ TODO What if our bookkeeping says otherwise? See conclusion - // field which includes timedout. Need to come back to this once - // have the "happy path" implemented. + // What if our bookkeeping says otherwise? But then we can't even + // access the service data easily here. @@ TODO: maybe/later. // return true; } @@ -290,9 +340,46 @@ namespace brep } else if (event == "pull_request") { - // @@ TODO + gh_pull_request_event pr; + try + { + json::parser p (body.data (), body.size (), "pull_request event"); + + pr = gh_pull_request_event (p); + } + catch (const json::invalid_json_input& e) + { + string m ("malformed JSON in " + e.name + " request body"); - throw invalid_request (501, "pull request events not implemented yet"); + error << m << ", line: " << e.line << ", column: " << e.column + << ", byte offset: " << e.position << ", error: " << e; + + throw invalid_request (400, move (m)); + } + + if (pr.action == "opened" || pr.action == "synchronize") + { + // 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.) + // + // 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); + } + else + { + // Ignore the remaining actions by sending a 200 response with empty + // body. + // + return true; + } } else { @@ -305,7 +392,7 @@ namespace brep } bool ci_github:: - handle_check_suite_request (gh_check_suite_event cs) + handle_check_suite_request (gh_check_suite_event cs, bool warning_success) { HANDLER_DIAG; @@ -331,25 +418,28 @@ namespace brep cs.check_suite.head_branch, repository_type::git); - string sd (service_data (move (iat->token), + string sd (service_data (warning_success, + move (iat->token), iat->expires_at, cs.installation.id, move (cs.repository.node_id), move (cs.check_suite.head_sha)) .json ()); + // @@ 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 (move (cs.check_suite.node_id), - "ci-github", - move (sd)), - move (rl), - vector<package> {}, - nullopt, // client_ip - nullopt // user_agent - )); + start (error, + warn, + verb_ ? &trace : nullptr, + tenant_service (move (cs.check_suite.node_id), + "ci-github", + move (sd)), + move (rl), + vector<package> {}, + nullopt, /* client_ip */ + nullopt /* user_agent */)); if (!r) fail << "unable to submit CI request"; @@ -357,6 +447,573 @@ namespace brep return true; } + // High-level description of PR handling: + // + // - CI the merge commit but add checks to PR head ref + // + // Recommended by the following blog posts by a GitHub employee who is one + // of the best sources on these topics: + // https://www.kenmuse.com/blog/shared-commits-and-github-checks/ and + // https://www.kenmuse.com/blog/creating-github-checks/. + // + // DO NOT ADD CHECKS TO THE MERGE COMMIT because: + // + // - The PR head ref is the only commit that can be relied upon to exist + // throughout the PR's lifetime. The merge commit, on the other hand, + // can get replaced during the PR process. When that happens the PR will + // look for checks on the new merge commit, effectively discarding the + // ones we had before. + // + // - Although some of the GitHub documentation makes it sound like they + // expect checks to be added to both the PR head and the merge commit, + // the PR UI does not react to the merge commit's checks + // consistently. It actually seems to be quite broken. + // + // The only thing it seems to do reliably is blocking the PR merge if + // the merge commit is not successful (overriding the PR head ref's + // checks). But the UI looks quite messed up generally in this state. + // + // - PR Creation + // + // False green window problem + // - User creates new commit on feature branch + // - We receive check_suite; check runs start, run, succeed + // - User creates PR from feature branch to master + // - Because feature branch head has successful CRs, PR is green to merge + // - But merge commit has not been CI'd yet + // - False green until first merge commit CR is queued + // + // check_suite(requested, PR_head): [shared repo model] + // + // - Starts CI with check runs on what will become the PR head ref + // + // pull_request(opened) + // + // - Fetch pull request to trigger start of merge commit job; ignore + // response + // + // - To close false green window + // - if pull_request.head.repo.node_id == pull_request.base.repo.node_id + // - Replace all (or just one?) of the CRs on the PR head ref with new, + // queued ones + // - Fetch all check runs on PR head SHA + // - Find CRs with this PR in pull_requests[] + // - Create new CRs with same names as existing, thus effectively + // destroying the old ones + // + // - Create unloaded tenant + // + // - On callback + // Fetch pull request; if mergeable, start CI proper + // + // - New commits are added to PR head branch + // + // False green window + // + // PR already exists so merge will be red initially after CS is + // received, but those CRs could theoretically all succeed before our CI + // of the merge commit starts. Either way the CS CRs are an inaccurate + // representation of what's being CI'd so need to be avoided if + // possible. + // + // check_suite(requested, PR_head) [shared repo model] + // + // Ignore if pull_requests[] is non-empty (which should be the case if + // the head branch is in the same repository). + // + // pull_request(synchronize) + // + // - Fetch pull request to trigger start of merge commit job; ignore + // response + // + // - Create unloaded tenant + // + // - On callback + // Fetch pull request; if mergeable, start CI proper + // + // - New commits are added to PR base branch + // + // check_suite(requested, PR_base) + // + // Fetch all open PRs for base branch (triggering the merge commit jobs) + // + // Replace CRs on PR head refs to close false green window. + // + // Submit new unloaded tenants for all of the PRs. + // + // NOTE: pull_request.base.sha does not change. But merge commit (once + // updated) does try to merge PR head to new tip of PR base. + // + // - PR closed/merged/etc. @@ TODO + // + // pull_request(closed/merged/...) + // + // Note: when a PR is merged we will get a pull_request(closed) and then a + // check_suite for the base branch. + // + bool ci_github:: + handle_pull_request (gh_pull_request_event pr, bool warning_success) + { + HANDLER_DIAG; + + l3 ([&]{trace << "pull_request event { " << pr << " }";}); + + // 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 (); + + optional<gh_installation_access_token> iat ( + obtain_installation_access_token (pr.installation.id, + move (*jwt), + error)); + if (!iat) + throw server_error (); + + l3 ([&]{trace << "installation_access_token { " << *iat << " }";}); + + string sd (service_data (warning_success, + move (iat->token), + iat->expires_at, + pr.installation.id, + move (pr.repository.node_id), + pr.pull_request.head_sha, + pr.repository.clone_url, + pr.pull_request.number) + .json ()); + + // Create 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. + // + // Note: use no delay since we need to (re)create the synthetic merge + // check run as soon as possible. + // + optional<string> tid ( + create (error, warn, &trace, + *build_db_, + tenant_service (move (pr.pull_request.node_id), + "ci-github", + move (sd)), + chrono::seconds (30) /* interval */, + chrono::seconds (0) /* delay */)); + + if (!tid) + fail << "unable to create unloaded CI request"; + + return true; + } + + // Return the colored circle corresponding to a result_status. + // + static string + circle (result_status rs) + { + switch (rs) + { + case result_status::success: return "\U0001F7E2"; // Green circle. + case result_status::warning: return "\U0001F7E0"; // Orange circle. + case result_status::error: + case result_status::abort: + case result_status::abnormal: return "\U0001F534"; // Red circle. + + // Valid values we should never encounter. + // + case result_status::skip: + case result_status::interrupt: + throw invalid_argument ("unexpected result_status value: " + + to_string (rs)); + } + + return ""; // Should never reach. + } + + // Let's capitalize the synthetic check run names to make them easier to + // distinguish from the regular ones. + // + static string merge_check_run_name ("MERGE-COMMIT"); + static string conclusion_check_run_name ("CONCLUSION"); + + function<optional<string> (const tenant_service&)> ci_github:: + build_unloaded (tenant_service&& ts, + const diag_epilogue& log_writer) const noexcept + { + NOTIFICATION_DIAG (log_writer); + + service_data sd; + try + { + sd = service_data (*ts.data); + } + catch (const invalid_argument& e) + { + error << "failed to parse service data: " << e; + return nullptr; + } + + // Get a new installation access token if the current one has expired. + // + const gh_installation_access_token* iat (nullptr); + optional<gh_installation_access_token> new_iat; + + if (system_clock::now () > sd.installation_access.expires_at) + { + if (optional<string> jwt = generate_jwt (trace, error)) + { + new_iat = obtain_installation_access_token (sd.installation_id, + move (*jwt), + error); + if (new_iat) + iat = &*new_iat; + } + } + else + iat = &sd.installation_access; + + if (iat == nullptr) + return nullptr; // Try again on the next call. + + auto make_iat_updater = [] () + { + function<optional<string> (const tenant_service&)> r; + + if (new_iat) + { + r = [&error, + iat = move (new_iat)] (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 + // 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_synthetic_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.report_sha, + nullopt /* details_url */, + build_state::building)) + { + return cr; + } + else + return nullopt; + }; + + // Update a synthetic check run with success or failure. Return the check + // run on success or nullopt on failure. + // + auto update_synthetic_cr = [iat, + &sd, + &error] (const string& node_id, + const string& name, + result_status rs, + string summary) -> optional<check_run> + { + assert (!node_id.empty ()); + + 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 = name; // For display purposes only. + + if (gq_update_check_run (error, + cr, + iat->token, + sd.repository_node_id, + node_id, + nullopt /* details_url */, + build_state::built, + move (br))) + { + return cr; + } + else + return nullopt; + }; + + // Synthetic merge check run node ID. Empty until created on the first + // call or retrieved from service data on subsequent calls. + // + string merge_node_id; + + // True if this is the first call (or the merge commit couldn't be created + // on the first call, in which case we just re-try by treating it as a + // 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. + // + // Note that there is still a window between receipt of the pull_request + // event and the first bot/worker asking for a task, which could be + // substantial. We could probably (also) try to (re)create the merge + // checkrun in the webhook. @@ Maybe/later. + // + if (first) + { + if (auto cr = create_cr (merge_check_run_name)) + { + l3 ([&]{trace << "created check_run { " << *cr << " }";}); + + merge_node_id = move (*cr->node_id); + } + else + return make_iat_updater (); // Try again on the next call. + } + else + merge_node_id = *sd.merge_node_id; + + // Start/check PR mergeability. + // + optional<string> mc ( + gq_pull_request_mergeable (error, iat->token, ts.id)); // Merge commit. + + if (!mc || mc->empty ()) + { + if (!mc) // No merge commit yet. + { + // If this is a subsequent notification and there is no merge commit, + // then there is nothing to do. + // + if (!first) + return make_iat_updater (); + + // Fall through to update service data. + } + else // Not mergeable. + { + // 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. + + if (auto cr = update_cr (merge_node_id, merge_check_run_name, + result_status::error, + "GitHub is unable to create test merge commit")) + { + l3 ([&]{trace << "updated check_run { " << *cr << " }";}); + + // Cancel the CI request. + // + if (!cancel (error, warn, &trace, *build_db_, ts.type, ts.id)) + { + // Nothing we can do but also highly unlikely. + // + error << "unable to cancel CI request: " + << "no tenant for service type/ID " + << ts.type << '/' << ts.id; + } + + return nullptr; // No need to update service data in this case. + } + else + { + // Don't cancel the CI request if the merge check run update failed + // so that we can try again on the next call. + + if (!first) + return make_iat_updater (); + + // Fall through to update service data. + } + } + + // This is a first notification, so record the merge check run in the + // service data. + // + return [&error, + iat = move (new_iat), + mni = move (merge_node_id)] (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 + // 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; + + sd.merge_node_id = mni; + + return sd.json (); + }; + } + + // If we are here, then it means we have a merge commit that we can load. + // + // Note that this can still be the first call (first=true). + // + + // 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. + + // Synthetic conclusion check run node ID. Empty until created on the + // "second" call or retrieved from service data on subsequent calls. + // + string conclusion_node_id; + + // True if this is the first call after the merge commit became available, + // which we will call the "second" call (or we couldn't create the + // conclusion check run on the first such call, in which case we just + // re-try by treating it as a "second" call). + // + bool second (!sd.conclusion_node_id); + + if (second) + { + if (auto cr = create_cr (conclusion_check_run_name)) + { + l3 ([&]{trace << "created check_run { " << *cr << " }";}); + + conclusion_node_id = move (*cr->node_id); + } + } + else + conclusion_node_id = *sd.conclusion_node_id; + + if (!conclusion_node_id.empty ()) // Conclusion check run was created. + { + // Update merge check run to successful. + // + 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 << " }";}); + + // Load the CI request. + // + // Example repository URL fragment: + // + // #pull/28/merge@1b6c9a361086ed93e6f1e67189e82d52de91c49b + // + repository_location rl (*sd.repository_clone_url + "#pull/" + + to_string (*sd.pr_number) + "/merge@" + *mc, + repository_type::git); + + optional<start_result> r ( + load (error, warn, &trace, *build_db_, move (ts), rl)); + + if (!r || r->status != 200) + { + string msg; + msg = "```\n"; + if (r) msg += r->message; + else msg += "Internal service error"; + msg += "\n```"; + + if (auto cr = update_cr (conclusion_node_id, + conclusion_check_run_name, + result_status::error, + move (msg))) + { + 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 + { + // 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), + mni = (first ? move (merge_node_id) : string ()), + cni = (second ? move (conclusion_node_id) : string ())] + (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 + // 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.empty ()) + sd.merge_node_id = mni; + + if (!cni.empty ()) + sd.conclusion_node_id = cni; + + return sd.json (); + }; + } + // Build state change notifications (see tenant-services.hxx for // background). Mapping our state transitions to GitHub pose multiple // problems: @@ -411,8 +1068,8 @@ namespace brep // building Skip if there is no check run in service data or it's // not in the queued state, otherwise update. // - // built Update if there is check run in service data and its - // state is not built, otherwise create new. + // built Update if there is check run in service data unless its + // state is built, otherwise create new. // // The rationale for this semantics is as follows: the building // notification is a "nice to have" and can be skipped if things are not @@ -487,12 +1144,14 @@ namespace brep } else if (*istate == build_state::built) { - // Unexpectd built->queued transition (rebuild). + // Unexpected built->queued transition (rebuild). // warn << "check run " << bid << ": unexpected rebuild"; } else - ; // Ignore interrupted. + { + // Ignore interrupted. + } } else { @@ -501,6 +1160,7 @@ namespace brep bs.push_back (b); crs.emplace_back (move (bid), + gh_check_run_name (b, &hs), nullopt, /* node_id */ build_state::queued, false /* state_synced */); @@ -537,13 +1197,11 @@ namespace brep { // Create a check_run for each build. // - if (gq_create_check_runs (crs, + if (gq_create_check_runs (error, + crs, iat->token, - sd.repository_id, sd.head_sha, - bs, - build_state::queued, - hs, - error)) + sd.repository_node_id, sd.report_sha, + build_state::queued)) { for (const check_run& cr: crs) { @@ -603,7 +1261,8 @@ namespace brep } function<optional<string> (const tenant_service&)> ci_github:: - build_building (const tenant_service& ts, const build& b, + build_building (const tenant_service& ts, + const build& b, const diag_epilogue& log_writer) const noexcept { NOTIFICATION_DIAG (log_writer); @@ -636,7 +1295,9 @@ namespace brep cr->state_synced = false; } else - ; // Network error during queued notification, ignore. + { + // Network error during queued notification, ignore. + } } else warn << "check run " << bid << ": out of order building " @@ -674,13 +1335,13 @@ namespace brep // if (iat != nullptr) { - if (gq_update_check_run (*cr, + if (gq_update_check_run (error, + *cr, iat->token, - sd.repository_id, + sd.repository_node_id, *cr->node_id, - build_state::building, - nullopt, /* result_status */ - error)) + details_url (b), + build_state::building)) { // Do nothing further if the state was already built on GitHub (note // that this is based on the above-mentioned special GitHub semantics @@ -743,7 +1404,8 @@ namespace brep } function<optional<string> (const tenant_service&)> ci_github:: - build_built (const tenant_service& ts, const build& b, + build_built (const tenant_service& ts, + const build& b, const diag_epilogue& log_writer) const noexcept { NOTIFICATION_DIAG (log_writer); @@ -760,32 +1422,35 @@ namespace brep } check_run cr; // Updated check run. - string bid (gh_check_run_name (b)); // Full Build ID. - - if (check_run* scr = sd.find_check_run (bid)) { - if (scr->state != build_state::building) + string bid (gh_check_run_name (b)); // Full Build ID. + + if (check_run* scr = sd.find_check_run (bid)) { - warn << "check run " << bid << ": out of order built " - << "notification; existing state: " << scr->state_string (); - } + if (scr->state != build_state::building) + { + warn << "check run " << bid << ": out of order built notification; " + << "existing state: " << scr->state_string (); + } - // Do nothing if already built. - // - if (scr->state == build_state::built) - return nullptr; + // Do nothing if already built (e.g., rebuild). + // + if (scr->state == build_state::built) + return nullptr; - cr = move (*scr); - } - else - { - warn << "check run " << bid << ": out of order built " - << "notification; no check run state in service data"; + cr = move (*scr); + } + else + { + warn << "check run " << bid << ": out of order built notification; " + << "no check run state in service data"; - cr.build_id = move (bid); - } + cr.build_id = move (bid); + cr.name = cr.build_id; + } - cr.state_synced = false; + cr.state_synced = false; + } // Get a new installation access token if the current one has expired. // @@ -812,16 +1477,120 @@ namespace brep // if (iat != nullptr) { + // Prepare the check run's summary field (the build information in an + // XHTML table). + // + string sm; // Summary. + { + using namespace web::xhtml; + + ostringstream os; + xml::serializer s (os, "check_run_summary"); + + // This hack is required to disable XML element name prefixes (which + // GitHub does not like). Note that this adds an xmlns declaration for + // the XHTML namespace which for now GitHub appears to ignore. If that + // ever becomes a problem, then we should redo this with raw XML + // serializer calls. + // + struct table: element + { + table (): element ("table") {} + + void + start (xml::serializer& s) const override + { + s.start_element (xmlns, name); + s.namespace_decl (xmlns, ""); + } + } TABLE; + + // Serialize a result row (colored circle, result text, log URL) for + // an operation and result_status. + // + auto tr_result = [this, &b] (xml::serializer& s, + const string& op, + result_status rs) + { + // The log URL. + // + string lu (build_log_url (options_->host (), + options_->root (), + b, + op != "result" ? &op : nullptr)); + + s << TR + << TD << EM << op << ~EM << ~TD + << TD + << circle (rs) << ' ' + << CODE << to_string (rs) << ~CODE + << " (" << A << HREF << lu << ~HREF << "log" << ~A << ')' + << ~TD + << ~TR; + }; + + // Serialize the summary to an XHTML table. + // + s << TABLE + << TBODY; + + tr_result (s, "result", *b.status); + + s << TR + << TD << EM << "package" << ~EM << ~TD + << TD << CODE << b.package_name << ~CODE << ~TD + << ~TR + << TR + << TD << EM << "version" << ~EM << ~TD + << TD << CODE << b.package_version << ~CODE << ~TD + << ~TR + << TR + << TD << EM << "toolchain" << ~EM << ~TD + << TD + << CODE + << b.toolchain_name << '-' << b.toolchain_version.string () + << ~CODE + << ~TD + << ~TR + << TR + << TD << EM << "target" << ~EM << ~TD + << TD << CODE << b.target.string () << ~CODE << ~TD + << ~TR + << TR + << TD << EM << "target config" << ~EM << ~TD + << TD << CODE << b.target_config_name << ~CODE << ~TD + << ~TR + << TR + << TD << EM << "package config" << ~EM << ~TD + << TD << CODE << b.package_config_name << ~CODE << ~TD + << ~TR; + + for (const operation_result& r: b.results) + tr_result (s, r.operation, r.status); + + s << ~TBODY + << ~TABLE; + + sm = os.str (); + } + + gq_built_result br (gh_to_conclusion (*b.status, sd.warning_success), + circle (*b.status) + ' ' + + ucase (to_string (*b.status)), + move (sm)); + if (cr.node_id) { // Update existing check run to built. // - if (gq_update_check_run (cr, + if (gq_update_check_run (error, + cr, iat->token, - sd.repository_id, + sd.repository_node_id, *cr.node_id, - build_state::built, b.status, - error)) + details_url (b), + build_state::built, + move (br))) { assert (cr.state == build_state::built); @@ -838,17 +1607,14 @@ namespace brep // check run to the service data it will create another check run with // the shortened name which will never get to the built state. // - // @@ TMP If build_queued() runs but fails to create we could store - // the build hints and use them now. - // - if (gq_create_check_run (cr, + if (gq_create_check_run (error, + cr, iat->token, - sd.repository_id, - sd.head_sha, - b, - build_state::built, b.status, - build_queued_hints (false, false), - error)) + sd.repository_node_id, + sd.report_sha, + details_url (b), + build_state::built, + move (br))) { assert (cr.state == build_state::built); @@ -861,40 +1627,58 @@ namespace brep cr = move (cr), error = move (error), warn = move (warn)] (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 captures. + { + // 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; - } + 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 (iat) + sd.installation_access = *iat; - if (check_run* scr = sd.find_check_run (cr.build_id)) + if (check_run* scr = sd.find_check_run (cr.build_id)) + { + // This will most commonly generate a duplicate warning (see above). + // We could save the old state and only warn if it differs but let's + // not complicate things for now. + // +#if 0 + if (scr->state != build_state::building) { - if (scr->state != build_state::building) - { - warn << "check run " << cr.build_id << ": out of order built " - << "notification service data update; existing state: " - << scr->state_string (); - } - - *scr = cr; + warn << "check run " << cr.build_id << ": out of order built " + << "notification service data update; existing state: " + << scr->state_string (); } - else - sd.check_runs.push_back (cr); +#endif + *scr = cr; + } + else + sd.check_runs.push_back (cr); - return sd.json (); - }; + return sd.json (); + }; + } + + string ci_github:: + details_url (const build& b) const + { + return options_->host () + + "/@" + b.tenant + + "?builds=" + mime_url_encode (b.package_name.string ()) + + "&pv=" + b.package_version.string () + + "&tg=" + mime_url_encode (b.target.string ()) + + "&tc=" + mime_url_encode (b.target_config_name) + + "&pc=" + mime_url_encode (b.package_config_name) + + "&th=" + mime_url_encode (b.toolchain_version.string ()); } optional<string> ci_github:: |