diff options
| -rw-r--r-- | INSTALL | 111 | ||||
| -rw-r--r-- | etc/brep-module.conf | 8 | ||||
| -rw-r--r-- | etc/private/install/brep-module.conf | 8 | ||||
| -rw-r--r-- | libbrep/utility.hxx | 5 | ||||
| -rw-r--r-- | manifest | 6 | ||||
| -rw-r--r-- | mod/hmac.cxx | 13 | ||||
| -rw-r--r-- | mod/mod-build-configs.cxx | 6 | ||||
| -rw-r--r-- | mod/mod-build-result.cxx | 30 | ||||
| -rw-r--r-- | mod/mod-build-task.cxx | 27 | ||||
| -rw-r--r-- | mod/mod-builds.cxx | 6 | ||||
| -rw-r--r-- | mod/mod-ci-github-gq.cxx | 94 | ||||
| -rw-r--r-- | mod/mod-ci-github-gq.hxx | 36 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.cxx | 10 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.hxx | 13 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 460 | ||||
| -rw-r--r-- | mod/mod-ci-github.hxx | 15 | ||||
| -rw-r--r-- | mod/mod-ci.cxx | 55 | ||||
| -rw-r--r-- | mod/mod-ci.hxx | 4 | ||||
| -rw-r--r-- | mod/mod-submit.cxx | 10 | ||||
| -rw-r--r-- | mod/module.cli | 10 | ||||
| -rw-r--r-- | mod/tenant-service.cxx | 18 | ||||
| -rw-r--r-- | mod/tenant-service.hxx | 12 |
22 files changed, 699 insertions, 258 deletions
@@ -356,7 +356,116 @@ For sample CI request handler implementations see brep/handler/ci/. 6.4 Enabling GitHub CI integration -@@ TODO +6.4.1 Background + +The GitHub CI integration has one user-configurable setting: +warning=<success|failure> (whether or not to fail on warnings). + +In order not to have to support repository configuration files, a deployment +will consist of two registered GitHub Apps with the same webhook URL (i.e., +the same brep instance) but different query parameters: one with +warning=success and the other with warning=failure. The App id is passed (as a +query parameter) so that we know which private key to use (the key cannot be +shared between Apps). + +We will call the warning=success App the "Default App" and the warning=failure +App the "Werror App". + +6.4.2 Create the GitHub Apps + +To create a GitHub App under the <org> organization, visit +https://github.com/organizations/<org>/settings/apps (Settings -> Developer +settings -> GitHub Apps). Then click on New GitHub App. + +App names (note: 34 character limit): + + Default App: "<org> CI" + Werror App: "<org> CI - warnings as errors" + +App description: + + Default App: "Trigger <org> CI on branch push and pull request." + Werror App: "Trigger <org> CI on branch push and pull request. Warnings are + treated as errors". + +App homepage: + + https://ci.<org>.org/ + +Skip the "Identifying and authorizing users" and "Post installation" sections. + +Leave webhooks active. + +Webhook URL: + + Default App: https://ci.<org>.org/?ci-github&app-id=XXX&warning=success + Werror App: https://ci.<org>.org/?ci-github&app-id=XXX&warning=failure + +Note that the App id only becomes available once the App has been registered +so we update it later in both URLs. + +Webhook secret: Use the same random 64-character string for both Apps. + + echo `tr -dc -- A-Za-z0-9 </dev/urandom | head -c 64` + +Note that GitHub says only that the secret should be "a random string with +high entropy." However lots of sources say 32 bytes should be secure enough +for HMAC-SHA256, while other sources recommend 64 bytes for maximal security +at an insignificant performance cost. (Keys longer than 64 bytes are hashed to +match the internal block size and are therefore not recommended.) + +Repository permissions: + - Checks: RW + - Contents: RO (for Push events) + - Metadata (mandatory): RO + - Pull requests: RO + +Subscribed events: + - Check suite + - Pull request + - Push + +Note that GitHub Apps with write access to the "Checks" permission are +automatically subscribed to check_suite(requested|rerequested) and check_run +events so no need to subscribe explicitly. However in order to receive +check_suite(completed) events, which we need, one does have to subscribe to +Check suite. + +Select "Any account" under "Where can this GitHub App be installed?". + +Click "Create GitHub App". + +When the page reloads (should be the General tab), note the App id and replace +the XXX in the webhook URL with it. + +Still in the General tab, scroll to Private keys and generate a private key. +The file will be downloaded by the browser. + +@@ TODO Logo +@@ TODO Create Marketplace listing + +6.4.3 Configure brep + +Assume the following configuration values: + +- Webhook secret: abcdefg +- Default App id: 12345 +- Werror App id: 67890 + +In brep-module.conf: + +Set the webhook secret from the GitHub App settings: + + ci-github-app-webhook-secret abcdefg + +Associate each GitHub App id with the App's private key: + + ci-github-app-id-private-key 12345=path/to/default-app-private-key.pem + ci-github-app-id-private-key 67890=path/to/werror-app-private-key.pem + +Now brep should be ready to handle the webhook event requests triggered by +branch pushes and pull requests in repositories into which one of these Apps +has been installed. 7. Optimize CSS diff --git a/etc/brep-module.conf b/etc/brep-module.conf index fd6ba67..cdf028a 100644 --- a/etc/brep-module.conf +++ b/etc/brep-module.conf @@ -454,13 +454,15 @@ menu About=?about # The GitHub App's configured webhook secret. If not set, then the GitHub CI -# service is disabled. Note: make sure to choose a strong (random) secret. +# service is disabled. Note that the path must be absolute. Note: make sure to +# choose a strong (random) secret. # -# ci-github-app-webhook-secret +# ci-github-app-webhook-secret <path> # The private key used during GitHub API authentication for the specified -# GitHub App ID. Both vales are found in the GitHub App's settings. +# GitHub App ID. Both vales are found in the GitHub App's settings. Note that +# the paths must be absolute. # # ci-github-app-id-private-key <id>=<path> diff --git a/etc/private/install/brep-module.conf b/etc/private/install/brep-module.conf index 07db881..2545a87 100644 --- a/etc/private/install/brep-module.conf +++ b/etc/private/install/brep-module.conf @@ -462,13 +462,15 @@ submit-handler-timeout 120 # The GitHub App's configured webhook secret. If not set, then the GitHub CI -# service is disabled. Note: make sure to choose a strong (random) secret. +# service is disabled. Note that the path must be absolute. Note: make sure to +# choose a strong (random) secret. # -# ci-github-app-webhook-secret +# ci-github-app-webhook-secret <path> # The private key used during GitHub API authentication for the specified -# GitHub App ID. Both vales are found in the GitHub App's settings. +# GitHub App ID. Both vales are found in the GitHub App's settings. Note that +# the paths must be absolute. # # ci-github-app-id-private-key <id>=<path> diff --git a/libbrep/utility.hxx b/libbrep/utility.hxx index fce8fb5..1925d01 100644 --- a/libbrep/utility.hxx +++ b/libbrep/utility.hxx @@ -12,7 +12,7 @@ #include <algorithm> // * #include <libbutl/utility.hxx> // icasecmp(), reverse_iterate(), - // operator<<(ostream, exception) + // operator<<(ostream, exception), etc namespace brep { @@ -28,6 +28,9 @@ namespace brep // <libbutl/utility.hxx> // using butl::utf8; + using butl::trim; + using butl::trim_left; + using butl::trim_right; using butl::icasecmp; using butl::reverse_iterate; } @@ -24,9 +24,9 @@ depends: libapr1 depends: libapreq2 depends: libcmark-gfm == 0.29.0-a.4 depends: libcmark-gfm-extensions == 0.29.0-a.4 -depends: libstudxml ^1.1.0-b.10 -depends: libodb ^2.5.0-b.27 -depends: libodb-pgsql ^2.5.0-b.27 +depends: libstudxml ^1.1.0 +depends: libodb ^2.5.0 +depends: libodb-pgsql ^2.5.0 depends: libbutl [0.18.0-a.0.1 0.18.0-a.1) depends: libbpkg [0.18.0-a.0.1 0.18.0-a.1) depends: libbbot [0.18.0-a.0.1 0.18.0-a.1) diff --git a/mod/hmac.cxx b/mod/hmac.cxx index 1a78b4c..cfb0e23 100644 --- a/mod/hmac.cxx +++ b/mod/hmac.cxx @@ -16,6 +16,12 @@ compute_hmac (const options::openssl_options& o, // To compute an HMAC over stdin with the key <secret>: // + // openssl dgst -sha256 -hmac <secret> + // + // Note that since openssl 3.0 the `mac` command is the preferred method + // for generating HMACs. For future reference, the equivalent command + // would be: + // // openssl mac -digest SHA256 -macopt "key:<secret>" HMAC // // Note that here we assume both output and diagnostics will fit into pipe @@ -25,10 +31,9 @@ compute_hmac (const options::openssl_options& o, path ("-"), // Write output to openssl::in. process::pipe (errp.in.get (), move (errp.out)), process_env (o.openssl (), o.openssl_envvar ()), - "mac", o.openssl_option (), - "-digest", "SHA256", - "-macopt", string ("key:") + k, - "HMAC"); + "dgst", o.openssl_option (), + "-sha256", + "-hmac", k); ifdstream err (move (errp.in)); diff --git a/mod/mod-build-configs.cxx b/mod/mod-build-configs.cxx index ce79edb..2754f95 100644 --- a/mod/mod-build-configs.cxx +++ b/mod/mod-build-configs.cxx @@ -34,10 +34,12 @@ init (scanner& s) s, unknown_mode::fail, unknown_mode::fail); if (options_->build_config_specified ()) + { build_config_module::init (*options_); - if (options_->root ().empty ()) - options_->root (dir_path ("/")); + if (options_->root ().empty ()) + options_->root (dir_path ("/")); + } } bool brep::build_configs:: diff --git a/mod/mod-build-result.cxx b/mod/mod-build-result.cxx index cc058b5..b303386 100644 --- a/mod/mod-build-result.cxx +++ b/mod/mod-build-result.cxx @@ -565,7 +565,7 @@ handle (request& rq, response&) { assert (tss); // Wouldn't be here otherwise. - const tenant_service& ss (tss->first); + tenant_service& ss (tss->first); const build& b (*tss->second); // Release the database connection since build_built() notification can @@ -576,7 +576,33 @@ handle (request& rq, response&) if (auto f = tsb->build_built (b.tenant, ss, b, log_writer_)) { conn = build_db_->connection (); - update_tenant_service_state (conn, ss.type, ss.id, f); + + bool build_completed (false); + + if (optional<string> data = + update_tenant_service_state ( + conn, ss.type, ss.id, + [&f, &build_completed] (const string& tid, + const tenant_service& ts) + { + auto r (f (tid, ts)); + build_completed = r.second; + return move (r.first); + })) + { + ss.data = move (data); + } + + if (build_completed) + { + // Release the database connection since the build_completed() + // notification can potentially be time-consuming (e.g., it may + // perform an HTTP request). + // + conn.reset (); + + tsb->build_completed (b.tenant, ss, log_writer_); + } } } diff --git a/mod/mod-build-task.cxx b/mod/mod-build-task.cxx index c8b1bb2..88e5618 100644 --- a/mod/mod-build-task.cxx +++ b/mod/mod-build-task.cxx @@ -2407,7 +2407,7 @@ handle (request& rq, response& rs) } // If a third-party service needs to be notified about the package - // build, then call the tenant_service_build_built::build_building() + // build, then call the tenant_service_build_building::build_building() // callback function and, if requested, update the tenant-associated // service state. // @@ -2556,9 +2556,32 @@ handle (request& rq, response& rs) { conn = build_db_->connection (); + bool build_completed (false); + if (optional<string> data = - update_tenant_service_state (conn, ss.type, ss.id, f)) + update_tenant_service_state ( + conn, ss.type, ss.id, + [&f, &build_completed] (const string& tid, + const tenant_service& ts) + { + auto r (f (tid, ts)); + build_completed = r.second; + return move (r.first); + })) + { ss.data = move (data); + } + + if (build_completed) + { + // Release the database connection since the build_completed() + // notification can potentially be time-consuming (e.g., it may + // perform an HTTP request). + // + conn.reset (); + + tsb->build_completed (b.tenant, ss, log_writer_); + } } } } diff --git a/mod/mod-builds.cxx b/mod/mod-builds.cxx index 0155c2e..b11b3d7 100644 --- a/mod/mod-builds.cxx +++ b/mod/mod-builds.cxx @@ -58,10 +58,10 @@ init (scanner& s) { database_module::init (*options_, options_->build_db_retry ()); build_config_module::init (*options_); - } - if (options_->root ().empty ()) - options_->root (dir_path ("/")); + if (options_->root ().empty ()) + options_->root (dir_path ("/")); + } } template <typename T, typename C> diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx index db69f0c..37d944c 100644 --- a/mod/mod-ci-github-gq.cxx +++ b/mod/mod-ci-github-gq.cxx @@ -380,9 +380,12 @@ namespace brep assert (cr.state != build_state::built); // Not supported. - // Ensure details URL is non-empty if present. + // Ensure details URL and output are non-empty if present. // assert (!cr.details_url || !cr.details_url->empty ()); + assert (!cr.description || + (!cr.description->title.empty () && + !cr.description->summary.empty ())); string al ("cr" + to_string (i)); // Field alias. @@ -396,6 +399,13 @@ namespace brep os << '\n'; os << " detailsUrl: " << gq_str (*cr.details_url); } + if (cr.description) + { + os << " output: {" << '\n' + << " title: " << gq_str (cr.description->title) << '\n' + << " summary: " << gq_str (cr.description->summary) << '\n' + << " }"; + } os << "})" << '\n' // Specify the selection set (fields to be returned). Note that we // rename `id` to `node_id` (using a field alias) for consistency with @@ -417,9 +427,9 @@ namespace brep // Serialize a `createCheckRun` mutation for a build to GraphQL. // - // The build result argument (`br`) is required if the build_state is built - // because GitHub does not allow a check run status of completed without a - // conclusion. + // The conclusion argument (`co`) is required if the check run status is + // completed because GitHub does not allow a check run status of completed + // without a conclusion. // // The details URL argument (`du`) can be empty for queued but not for the // other states. @@ -433,12 +443,18 @@ namespace brep const optional<string>& du, // Details URL. const check_run& cr, const string& st, // Check run status. - optional<gq_built_result> br = nullopt) + const string& ti, // Output title. + const string& su, // Output summary. + optional<string> co = nullopt) // Conclusion. { // Ensure details URL is non-empty if present. // assert (!du || !du->empty ()); + // Ensure we have conclusion if the status is completed. + // + assert (st != "COMPLETED" || co); + ostringstream os; os << "mutation {" << '\n'; @@ -455,15 +471,13 @@ namespace brep os << '\n'; os << " detailsUrl: " << gq_str (*du); } - if (br) - { - os << '\n'; - os << " conclusion: " << gq_enum (br->conclusion) << '\n' - << " output: {" << '\n' - << " title: " << gq_str (br->title) << '\n' - << " summary: " << gq_str (br->summary) << '\n' - << " }"; - } + os << '\n'; + if (co) + os << " conclusion: " << gq_enum (*co) << '\n'; + os << " output: {" << '\n' + << " title: " << gq_str (ti) << '\n' + << " summary: " << gq_str (su) << '\n' + << " }"; os << "})" << '\n' // Specify the selection set (fields to be returned). Note that we // rename `id` to `node_id` (using a field alias) for consistency with @@ -485,7 +499,7 @@ namespace brep // Serialize an `updateCheckRun` mutation for one build to GraphQL. // - // The `co` (conclusion) argument is required if the build_state is built + // The `br` argument is required if the check run status is completed // because GitHub does not allow updating a check run to completed without a // conclusion. // @@ -495,14 +509,11 @@ namespace brep static string gq_mutation_update_check_run (const string& ri, // Repository ID. const string& ni, // Node ID. - const optional<string>& du, // Details URL. const string& st, // Check run status. optional<timestamp> sa, // Started at. optional<gq_built_result> br) { - // Ensure details URL is non-empty if present. - // - assert (!du || !du->empty ()); + assert (st != "COMPLETED" || br); ostringstream os; @@ -527,11 +538,6 @@ namespace brep ": " + e.what ()); } } - if (du) - { - os << '\n'; - os << " detailsUrl: " << gq_str (*du); - } if (br) { os << '\n'; @@ -586,11 +592,11 @@ namespace brep const string& hs, const optional<string>& du, build_state st, - optional<gq_built_result> br) + string ti, string su) { - // Must have a result if state is built. + // State cannot be built without a conclusion. // - assert (st != build_state::built || br); + assert (st != build_state::built && !ti.empty () && !su.empty ()); string rq ( gq_serialize_request ( @@ -599,7 +605,8 @@ namespace brep du, cr, gh_to_status (st), - move (br)))); + move (ti), move (su), + nullopt /* conclusion */))); vector<check_run> crs {move (cr)}; crs[0].state = st; @@ -612,12 +619,40 @@ namespace brep } bool + gq_create_check_run (const basic_mark& error, + check_run& cr, + const string& iat, + const string& rid, + const string& hs, + const optional<string>& du, + gq_built_result br) + { + string rq ( + gq_serialize_request ( + gq_mutation_create_check_run (rid, + hs, + du, + cr, + gh_to_status (build_state::built), + move (br.title), move (br.summary), + move (br.conclusion)))); + + vector<check_run> crs {move (cr)}; + crs[0].state = build_state::built; + + bool r (gq_mutate_check_runs (error, crs, iat, move (rq))); + + cr = move (crs[0]); + + return r; + } + + bool gq_update_check_run (const basic_mark& error, check_run& cr, const string& iat, const string& rid, const string& nid, - const optional<string>& du, build_state st, optional<gq_built_result> br) { @@ -636,7 +671,6 @@ namespace brep gq_serialize_request ( gq_mutation_update_check_run (rid, nid, - du, gh_to_status (st), sa, move (br)))); diff --git a/mod/mod-ci-github-gq.hxx b/mod/mod-ci-github-gq.hxx index 50950d4..0fc3817 100644 --- a/mod/mod-ci-github-gq.hxx +++ b/mod/mod-ci-github-gq.hxx @@ -20,7 +20,7 @@ namespace brep // // Create a new check run on GitHub for each build with the build state, - // name, and details_url taken from each check_run object. Update + // name, details_url, and output taken from each check_run object. Update // `check_runs` with the new data (node id and state_synced). Return false // and issue diagnostics if the request failed. // @@ -39,18 +39,32 @@ namespace brep const string& repository_id, const string& head_sha); - // Create a new check run on GitHub for a build. Update `cr` with the new - // data (node id, state, and state_synced). Return false and issue - // diagnostics if the request failed. + // Create a new check run on GitHub for a build in the queued or building + // state. Note that the state cannot be built because in that case a + // conclusion is required. + // + // Update `cr` with the new data (node id, state, and state_synced). Return + // false and issue diagnostics if the request failed. // // Throw invalid_argument if the passed data is invalid, missing, or // inconsistent. // - // If the details_url is absent GitHub will use the app's homepage. + // If the details_url is absent GitHub will use the app's homepage. Title + // and summary are required and cannot be empty. // - // The gq_built_result is required if the build_state is built because - // GitHub does not allow a check run status of `completed` without at least - // a conclusion. + bool + gq_create_check_run (const basic_mark& error, + check_run& cr, + const string& installation_access_token, + const string& repository_id, + const string& head_sha, + const optional<string>& details_url, + build_state, + string title, + string summary); + + // As above but create a check run in the built state (which requires a + // conclusion). // struct gq_built_result { @@ -66,8 +80,7 @@ namespace brep const string& repository_id, const string& head_sha, const optional<string>& details_url, - build_state, - optional<gq_built_result> = nullopt); + gq_built_result); // Update a check run on GitHub. Update `cr` with the new data (state and // state_synced). Return false and issue diagnostics if the request failed. @@ -79,8 +92,6 @@ namespace brep // built to built is allowed). The latter case is signalled by setting the // check_run state_synced member to false and the state member to built. // - // If the details_url is absent GitHub will use the app's homepage. - // // The gq_built_result is required if the build_state is built because // GitHub does not allow a check run status of `completed` without at least // a conclusion. @@ -91,7 +102,6 @@ namespace brep const string& installation_access_token, const string& repository_id, const string& node_id, - const optional<string>& details_url, build_state, optional<gq_built_result> = nullopt); diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx index 9f66a6c..c51f791 100644 --- a/mod/mod-ci-github-service-data.cxx +++ b/mod/mod-ci-github-service-data.cxx @@ -113,7 +113,15 @@ namespace brep } } - check_runs.emplace_back (move (bid), move (nm), move (nid), s, ss, rs); + check_runs.push_back ( + check_run {move (bid), + move (nm), + move (nid), + s, + ss, + rs, + nullopt, /* details_url */ + nullopt /* description */}); p.next_expect (event::end_object); } diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx index 50bb49d..5d36696 100644 --- a/mod/mod-ci-github-service-data.hxx +++ b/mod/mod-ci-github-service-data.hxx @@ -33,10 +33,17 @@ namespace brep optional<result_status> status; // Only if state is built & synced. - // Note: never serialized (only used to pass information to the GraphQL - // functions). + // Note: these are never serialized (only used to pass information to the + // GraphQL functions). // - optional<string> details_url; + struct description_type + { + string title; + string summary; + }; + + optional<string> details_url; + optional<description_type> description; string state_string () const diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index 25c238e..b995256 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -77,12 +77,45 @@ namespace brep // Prepare for the CI requests handling, if configured. // - if (options_->build_config_specified () && - options_->ci_github_app_webhook_secret_specified ()) + if (options_->ci_github_app_webhook_secret_specified ()) { + if (!options_->build_config_specified ()) + fail << "package building functionality must be enabled"; + if (!options_->ci_github_app_id_private_key_specified ()) fail << "no app id/private key mappings configured"; + for (const auto& pr: options_->ci_github_app_id_private_key ()) + { + if (pr.second.relative ()) + fail << "ci-github-app-id-private-key path must be absolute"; + } + + // Read the webhook secret from the configured path. + // + { + const path& p (options_->ci_github_app_webhook_secret ()); + + if (p.relative ()) + fail << "ci-github-app-webhook-secret path must be absolute"; + + try + { + ifdstream is (p); + getline (is, webhook_secret_, '\0'); + + // Trim leading/trailing whitespaces (presumably GitHub does the + // same in its web UI). + // + if (trim (webhook_secret_).empty ()) + fail << "empty webhook secret in " << p; + } + catch (const io_error& e) + { + fail << "unable to read webhook secret from " << p << ": " << e; + } + } + ci_start::init (make_shared<options::ci_start> (*options_)); database_module::init (*options_, options_->build_db_retry ()); @@ -207,10 +240,10 @@ namespace brep // try { - string h ( - compute_hmac (*options_, - body.data (), body.size (), - options_->ci_github_app_webhook_secret ().c_str ())); + string h (compute_hmac (*options_, + body.data (), + body.size (), + webhook_secret_.c_str ())); if (!icasecmp (h, hmac)) { @@ -546,6 +579,10 @@ namespace brep // static string conclusion_check_run_name ("CONCLUSION"); + static check_run::description_type conclusion_check_run_building_description { + "\U000026AA IN PROGRESS", // "Medium white" circle. + "Waiting for all builds to complete"}; + bool ci_github:: handle_branch_push (gh_push_event ps, bool warning_success) { @@ -1124,8 +1161,14 @@ namespace brep static gq_built_result make_built_result (result_status rs, bool warning_success, string message) { + string title (circle (rs == result_status::warning && !warning_success + ? result_status::error + : rs)); + title += ' '; + title += ucase (to_string (rs)); + return {gh_to_conclusion (rs, warning_success), - circle (rs) + ' ' + ucase (to_string (rs)), + move (title), move (message)}; } @@ -1286,7 +1329,6 @@ namespace brep if (gq_update_check_run (error, bcr, iat->token, repo_node_id, cr.check_run.node_id, - nullopt /* details_url */, build_state::built, br)) { l3 ([&]{trace << "updated check_run { " << bcr << " }";}); @@ -1300,7 +1342,6 @@ namespace brep if (gq_update_check_run (error, ccr, iat->token, repo_node_id, *sd.conclusion_node_id, - nullopt /* details_url */, build_state::built, move (br))) { l3 ([&]{trace << "updated conclusion check_run { " << ccr << " }";}); @@ -1349,7 +1390,6 @@ namespace brep // if (gq_update_check_run (error, ccr, iat->token, repo_node_id, *sd.conclusion_node_id, - nullopt /* details_url */, build_state::built, move (br))) { l3 ([&]{trace << "updated conclusion check_run { " << ccr << " }";}); @@ -1430,6 +1470,8 @@ namespace brep ccr.state = build_state::building; ccr.state_synced = false; + ccr.details_url = details_url (tenant_id); + ccr.description = conclusion_check_run_building_description; if (gq_create_check_runs (error, check_runs, iat->token, repo_node_id, head_sha)) @@ -1580,7 +1622,6 @@ namespace brep // if (gq_update_check_run (error, bcr, iat->token, repo_node_id, *bcr.node_id, - nullopt /* details_url */, build_state::built, br)) { l3 ([&]{trace << "updated check_run { " << bcr << " }";}); @@ -1597,7 +1638,6 @@ namespace brep // if (gq_update_check_run (error, ccr, iat->token, repo_node_id, *ccr.node_id, - nullopt /* details_url */, build_state::built, move (br))) { l3 ([&]{trace << "updated conclusion check_run { " << ccr << " }";}); @@ -1896,9 +1936,13 @@ namespace brep // 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, + auto create_synthetic_cr = [&tenant_id, + iat, &sd, - &error] (string name) -> optional<check_run> + &error, + this] (string name, + const check_run::description_type& output) + -> optional<check_run> { check_run cr; cr.name = move (name); @@ -1910,8 +1954,9 @@ namespace brep iat->token, sd.repository_node_id, sd.report_sha, - nullopt /* details_url */, - build_state::building)) + details_url (tenant_id), + build_state::building, + output.title, output.summary)) { return cr; } @@ -1946,7 +1991,6 @@ namespace brep iat->token, sd.repository_node_id, node_id, - nullopt /* details_url */, build_state::built, move (br))) { @@ -1969,7 +2013,9 @@ namespace brep if (!sd.conclusion_node_id) { - if (auto cr = create_synthetic_cr (conclusion_check_run_name)) + if (auto cr = + create_synthetic_cr (conclusion_check_run_name, + conclusion_check_run_building_description)) { l3 ([&]{trace << "created check_run { " << *cr << " }";}); @@ -2263,11 +2309,15 @@ 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 */); + crs.push_back ( + check_run {move (bid), + gh_check_run_name (b, &hs), + nullopt, /* node_id */ + build_state::queued, + false /* state_synced */, + nullopt /* status */, + details_url (b), + nullopt /* description */}); } } @@ -2471,7 +2521,6 @@ namespace brep iat->token, sd.repository_node_id, *cr->node_id, - details_url (b), build_state::building)) { // Do nothing further if the state was already built on GitHub (note @@ -2547,7 +2596,8 @@ namespace brep return nullptr; } - function<optional<string> (const string&, const tenant_service&)> ci_github:: + function<pair<optional<string>, bool> (const string&, + const tenant_service&)> ci_github:: build_built (const string& tenant_id, const tenant_service& ts, const build& b, @@ -2578,44 +2628,17 @@ namespace brep if (sd.completed) return nullptr; - // Here we need to update the state of this check run and, if there are no - // more unbuilt ones, update the synthetic conclusion check run and mark - // the check suite as completed. - // - // Absent means we still have unbuilt check runs. - // - optional<result_status> conclusion (*b.status); + // Here we only update the state of this check run. If there are no more + // unbuilt ones, then the synthetic conclusion check run will be updated + // in build_completed(). Note that determining whether we have no more + // unbuilt would be racy here so instead we do it in the service data + // update function that we return. check_run cr; // Updated check run. { string bid (gh_check_run_name (b)); // Full build id. - optional<check_run> scr; - for (check_run& cr: sd.check_runs) - { - if (cr.build_id == bid) - { - assert (!scr); - scr = move (cr); - } - else - { - if (cr.state == build_state::built) - { - assert (cr.status); - - if (conclusion) - *conclusion |= *cr.status; - } - else - conclusion = nullopt; - } - - if (scr && !conclusion.has_value ()) - break; - } - - if (scr) + if (check_run* scr = sd.find_check_run (bid)) { if (scr->state != build_state::building) { @@ -2664,8 +2687,6 @@ namespace brep else iat = &sd.installation_access; - bool completed (false); - // Note: we treat the failure to obtain the installation access token the // same as the failure to notify GitHub (state is updated but not marked // synced). @@ -2787,7 +2808,6 @@ namespace brep iat->token, sd.repository_node_id, *cr.node_id, - details_url (b), build_state::built, move (br))) { @@ -2811,7 +2831,6 @@ namespace brep sd.repository_node_id, sd.report_sha, details_url (b), - build_state::built, move (br))) { assert (cr.state == build_state::built); @@ -2821,71 +2840,27 @@ namespace brep if (cr.state_synced) { - // Check run was created/updated successfully to built (with - // status we specified). + // Check run was created/updated successfully to built (with status we + // specified). // cr.status = b.status; - - // Update the conclusion check run if all check runs are now built. - // - if (conclusion) - { - assert (sd.conclusion_node_id); - - result_status rs (*conclusion); - - gq_built_result br ( - make_built_result (rs, sd.warning_success, - "All configurations are built")); - - check_run cr; - - // Set some fields for display purposes. - // - cr.node_id = *sd.conclusion_node_id; - cr.name = conclusion_check_run_name; - - // Let unlikely invalid_argument propagate. - // - 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 conclusion check_run { " << cr << " }";}); - } - else - { - // Nothing we can do here except log the error. - // - error << "tenant_service id " << ts.id - << ": unable to update conclusion check run " - << *sd.conclusion_node_id; - } - - completed = true; - } } } return [tenant_id, iat = move (new_iat), cr = move (cr), - completed = completed, error = move (error), warn = move (warn)] (const string& ti, - const tenant_service& ts) -> optional<string> + const tenant_service& ts) { // NOTE: this lambda may be called repeatedly (e.g., due to transaction // being aborted) and so should not move out of its captures. + // Do nothing if the tenant has been replaced. + // if (tenant_id != ti) - return nullopt; // Do nothing if the tenant has been replaced. + return make_pair (optional<string> (), false); service_data sd; try @@ -2895,7 +2870,19 @@ namespace brep catch (const invalid_argument& e) { error << "failed to parse service data: " << e; - return nullopt; + return make_pair (optional<string> (), false); + } + + // Feel like this could potentially happen in case of an out of order + // notification (see above). + // + if (sd.completed) + { + // @@ Perhaps this should be a warning but let's try error for now (we + // essentially missed a build, which could have failed). + // + error << "built notification for completed check suite"; + return make_pair (optional<string> (), false); } if (iat) @@ -2925,46 +2912,204 @@ namespace brep else sd.check_runs.push_back (cr); - if (bool c = completed) + // Determine of this check suite is completed. + // + sd.completed = find_if (sd.check_runs.begin (), sd.check_runs.end (), + [] (const check_run& scr) + { + return scr.state != build_state::built; + }) == sd.check_runs.end (); + } + + return make_pair (optional<string> (sd.json ()), sd.completed); + }; + } + catch (const std::exception& e) + { + NOTIFICATION_DIAG (log_writer); + + string bid (gh_check_run_name (b)); // Full build id. + + error << "check run " << bid << ": unhandled exception: " << e.what (); + + return nullptr; + } + + void ci_github:: + build_completed (const string& /* tenant_id */, + const tenant_service& ts, + const diag_epilogue& log_writer) const noexcept + try + { + // NOTE: this function is noexcept and should not throw. + + 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; + } + + // This could have been reset by handle_check_run_rerequest(). + // + if (!sd.completed) + return; + + assert (!sd.check_runs.empty ()); + + // Here we need to update the state of the synthetic conclusion check run. + // + result_status result (result_status::success); + + // Conclusion check run summary. Will include the success/warning/failure + // count breakdown. + // + string summary; + { + // The success/warning/failure counts. + // + // Note that the warning count will be included in the success or + // failure count (depending on the value of sd.warning_success). + // + size_t succ_count (0), warn_count (0), fail_count (0); + + // Count a result_status under the appropriate category. + // + auto count = [&succ_count, + &warn_count, + &fail_count, + ws = sd.warning_success] (result_status rs) + { + switch (rs) { - // Note that this can be racy: while we calculated the completed - // value based on the snapshot of the service data, it could have - // been changed (e.g., by handle_check_run_rerequest()). So we - // re-calculate it based on the check run states and only update if - // it matches. Otherwise, we log an error. - // - for (const check_run& scr: sd.check_runs) + case result_status::success: ++succ_count; break; + + case result_status::error: + case result_status::abort: + case result_status::abnormal: ++fail_count; break; + + case result_status::warning: { - if (scr.state != build_state::built) - { - string sid (sd.repository_node_id + ':' + sd.report_sha); + ++warn_count; - error << "tenant_service id " << sid - << ": out of order built notification service data update; " - << "check suite is no longer complete"; + if (ws) + ++succ_count; + else + ++fail_count; - c = false; - break; - } + break; } - if (c) - sd.completed = true; + case result_status::skip: + case result_status::interrupt: + { + assert (false); + } } + }; + + for (const check_run& cr: sd.check_runs) + { + assert (cr.state == build_state::built && cr.status); + + result |= *cr.status; + count (*cr.status); } - return sd.json (); - }; + // Construct the conclusion check run summary. + // + ostringstream os; + + // Note: the warning count has already been included in the success or + // failure count. + // + os << fail_count << " failed"; + if (!sd.warning_success && warn_count != 0) + os << " (" << warn_count << " due to warnings)"; + + os << ", " << succ_count << " succeeded"; + if (sd.warning_success && warn_count != 0) + os << " (" << warn_count << " with warnings)"; + + os << ", " << (succ_count + fail_count) << " total"; + + summary = os.str (); + } + + // Get a new installation access token if the current one has expired + // (unlikely since we just returned from build_built()). Note also that we + // are not saving the new token in the service data. + // + 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 (sd.app_id, 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; + + // Note: we treat the failure to obtain the installation access token the + // same as the failure to notify GitHub. + // + if (iat != nullptr) + { + // Update the conclusion check run if all check runs are now built. + // + assert (sd.conclusion_node_id); + + gq_built_result br ( + make_built_result (result, sd.warning_success, move (summary))); + + check_run cr; + + // Set some fields for display purposes. + // + cr.node_id = *sd.conclusion_node_id; + cr.name = conclusion_check_run_name; + + // Let unlikely invalid_argument propagate. + // + if (gq_update_check_run (error, + cr, + iat->token, + sd.repository_node_id, + *sd.conclusion_node_id, + build_state::built, + move (br))) + { + assert (cr.state == build_state::built); + l3 ([&]{trace << "updated conclusion check_run { " << cr << " }";}); + } + else + { + // Nothing we can do here except log the error. + // + error << "tenant_service id " << ts.id + << ": unable to update conclusion check run " + << *sd.conclusion_node_id; + } + } } catch (const std::exception& e) { NOTIFICATION_DIAG (log_writer); - string bid (gh_check_run_name (b)); // Full build id. - - error << "check run " << bid << ": unhandled exception: " << e.what(); - - return nullptr; + error << "unhandled exception: " << e.what (); } string ci_github:: @@ -2972,10 +3117,11 @@ namespace brep { // This code is based on build_force_url() in mod/build.cxx. // - return options_->host () + - "/@" + b.tenant + + return + options_->host () + + tenant_dir (options_->root (), b.tenant).string () + "?builds=" + mime_url_encode (b.package_name.string ()) + - "&pv=" + b.package_version.string () + + "&pv=" + mime_url_encode (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) + @@ -2983,6 +3129,15 @@ namespace brep b.toolchain_version.string (); } + string ci_github:: + details_url (const string& t) const + { + return + options_->host () + + tenant_dir (options_->root (), t).string () + + "?builds"; + } + static optional<build_id> parse_details_url (const string& details_url) try @@ -2995,12 +3150,21 @@ namespace brep // Extract the tenant from the URL path. // - // Example path: @d2586f57-21dc-40b7-beb2-6517ad7917dd + // Example paths: + // + // @d2586f57-21dc-40b7-beb2-6517ad7917dd (37 characters) + // <brep-root>/@d2586f57-21dc-40b7-beb2-6517ad7917dd // - if (!u.path || u.path->size () != 37 || (*u.path)[0] != '@') + if (!u.path) return nullopt; - r.package.tenant = u.path->substr (1); + { + size_t p (u.path->find ('@')); + if (p == string::npos || u.path->size () - p != 37) + return nullopt; // Tenant not found or too short. + + r.package.tenant = u.path->substr (p + 1); + } // Extract the rest of the build_id members from the URL query. // @@ -3043,7 +3207,7 @@ namespace brep }; if (c (pn, "builds")) r.package.name = package_name (decval ()); - else if (c (pv, "pv")) r.package.version = make_version (rawval ()); + else if (c (pv, "pv")) r.package.version = make_version (decval ()); else if (c (tg, "tg")) r.target = target_triplet (decval ()); else if (c (tc, "tc")) r.target_config_name = decval (); else if (c (pc, "pc")) r.package_config_name = decval (); @@ -3055,8 +3219,8 @@ namespace brep // Note: parsing code based on mod/mod-builds.cxx. // - size_t p (v.find_first_of ('-')); - if (p >= v.size () - 1) + size_t p (v.find ('-')); + if (p == string::npos || p >= v.size () - 1) return nullopt; // Invalid format. r.toolchain_name = v.substr (0, p); diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx index 4fcfa7e..4cedc94 100644 --- a/mod/mod-ci-github.hxx +++ b/mod/mod-ci-github.hxx @@ -72,12 +72,18 @@ namespace brep const build&, const diag_epilogue& log_writer) const noexcept override; - virtual function<optional<string> (const string&, const tenant_service&)> + virtual function<pair<optional<string>, bool> (const string&, + const tenant_service&)> build_built (const string& tenant_id, const tenant_service&, const build&, const diag_epilogue& log_writer) const noexcept override; + virtual void + build_completed (const string& tenant_id, + const tenant_service& ts, + const diag_epilogue& log_writer) const noexcept override; + private: virtual void init (cli::scanner&); @@ -127,6 +133,11 @@ namespace brep string details_url (const build&) const; + // Build a check run details_url for a tenant. + // + string + details_url (const string& tenant) const; + optional<string> generate_jwt (const string& app_id, const basic_mark& trace, @@ -145,6 +156,8 @@ namespace brep shared_ptr<options::ci_github> options_; tenant_service_map& tenant_service_map_; + + string webhook_secret_; }; } diff --git a/mod/mod-ci.cxx b/mod/mod-ci.cxx index 46fbf6a..85c00c6 100644 --- a/mod/mod-ci.cxx +++ b/mod/mod-ci.cxx @@ -105,17 +105,17 @@ init (scanner& s) fail << "unable to read ci-form file '" << ci_form << "': " << e; } } - } #ifdef BREP_CI_TENANT_SERVICE_UNLOADED - if (!options_->build_config_specified ()) - fail << "package building functionality must be enabled"; + if (!options_->build_config_specified ()) + fail << "package building functionality must be enabled"; - database_module::init (*options_, options_->build_db_retry ()); + database_module::init (*options_, options_->build_db_retry ()); #endif - if (options_->root ().empty ()) - options_->root (dir_path ("/")); + if (options_->root ().empty ()) + options_->root (dir_path ("/")); + } } bool brep::ci:: @@ -131,8 +131,6 @@ handle (request& rq, response& rs) HANDLER_DIAG; - const dir_path& root (options_->root ()); - // We will respond with the manifest to the CI request submission protocol // violations and with a plain text message on the internal errors. In the // latter case we will always respond with the same neutral message for @@ -180,6 +178,8 @@ handle (request& rq, response& rs) if (!options_->ci_data_specified ()) return respond_manifest (404, "CI request submission disabled"); + const dir_path& root (options_->root ()); + // Parse the request form data and verify the submission size limit. // // Note that the submission may include the overrides upload that we don't @@ -387,18 +387,19 @@ handle (request& rq, response& rs) optional<start_result> r; - if (optional<string> ref = create (error, - warn, - verb_ ? &trace : nullptr, - *build_db_, - tenant_service ("", "ci", rl.string ()), - chrono::seconds (40), - chrono::seconds (10))) + if (optional<pair<string, duplicate_tenant_result>> ref = + create (error, + warn, + verb_ ? &trace : nullptr, + *build_db_, retry_, + tenant_service ("", "ci", rl.string ()), + chrono::seconds (40), + chrono::seconds (10))) { string msg ("unloaded CI request is created: " + - options_->host () + tenant_dir (root, *ref).string ()); + options_->host () + tenant_dir (root, ref->first).string ()); - r = start_result {200, move (msg), move (*ref), {}}; + r = start_result {200, move (msg), move (ref->first), {}}; } #endif @@ -495,8 +496,8 @@ build_building (const string& /*tenant_id*/, }; } -function<optional<string> (const string& tenant_id, - const brep::tenant_service&)> brep::ci:: +function<pair<optional<string>, bool> (const string& tenant_id, + const brep::tenant_service&)> brep::ci:: build_built (const string& /*tenant_id*/, const tenant_service&, const build& b, @@ -514,13 +515,16 @@ build_built (const string& /*tenant_id*/, b.toolchain_name + '/' + b.toolchain_version.string ()); - return ts.data ? *ts.data + ", " + s : s; + return make_pair ( + optional<string> (ts.data ? *ts.data + ", " + s : s), false); }; } #ifdef BREP_CI_TENANT_SERVICE_UNLOADED -function<optional<string> (const brep::tenant_service&)> brep::ci:: -build_unloaded (tenant_service&& ts, +function<optional<string> (const string& tenant_id, + const brep::tenant_service&)> brep::ci:: +build_unloaded (const string& /* tenant_id */, + tenant_service&& ts, const diag_epilogue& log_writer) const noexcept { NOTIFICATION_DIAG (log_writer); @@ -532,7 +536,7 @@ build_unloaded (tenant_service&& ts, repository_location rl (*ts.data); if (!load (error, warn, verb_ ? &trace : nullptr, - *build_db_, + *build_db_, retry_, move (ts), rl)) return nullptr; // The diagnostics is already issued. @@ -545,7 +549,10 @@ build_unloaded (tenant_service&& ts, return nullptr; } - return [] (const tenant_service& ts) {return "loaded " + *ts.data;}; + return [] (const string& tenant_id, const tenant_service& ts) + { + return "loaded " + tenant_id + ' ' + *ts.data; + }; } #endif #endif diff --git a/mod/mod-ci.hxx b/mod/mod-ci.hxx index 132b5b0..54532e6 100644 --- a/mod/mod-ci.hxx +++ b/mod/mod-ci.hxx @@ -87,8 +87,8 @@ namespace brep const build&, const diag_epilogue& log_writer) const noexcept override; - virtual function<optional<string> (const string& tenant_id, - const tenant_service&)> + virtual function<pair<optional<string>, bool> (const string& tenant_id, + const tenant_service&)> build_built (const string& tenant_id, const tenant_service&, const build&, diff --git a/mod/mod-submit.cxx b/mod/mod-submit.cxx index 5ee358a..6c767cb 100644 --- a/mod/mod-submit.cxx +++ b/mod/mod-submit.cxx @@ -93,10 +93,10 @@ init (scanner& s) if (options_->submit_handler_specified () && options_->submit_handler ().relative ()) fail << "submit-handler path must be absolute"; - } - if (options_->root ().empty ()) - options_->root (dir_path ("/")); + if (options_->root ().empty ()) + options_->root (dir_path ("/")); + } } bool brep::submit:: @@ -109,8 +109,6 @@ handle (request& rq, response& rs) HANDLER_DIAG; - const dir_path& root (options_->root ()); - // We will respond with the manifest to the submission protocol violations // and with a plain text message on the internal errors. In the latter case // we will always respond with the same neutral message for security reason, @@ -163,6 +161,8 @@ handle (request& rq, response& rs) if (!options_->submit_data_specified ()) return respond_manifest (404, "submission disabled"); + const dir_path& root (options_->root ()); + // Parse the request form data and verify the submission size limit. // // Note that if it is exceeded, then there are parameters and this is the diff --git a/mod/module.cli b/mod/module.cli index 1273bf4..ba2b986 100644 --- a/mod/module.cli +++ b/mod/module.cli @@ -850,12 +850,12 @@ namespace brep // GitHub CI-specific options. // - string ci-github-app-webhook-secret + path ci-github-app-webhook-secret { - "<secret>", + "<path>", "The GitHub App's configured webhook secret. If not set, then the - GitHub CI service is disabled. Note: make sure to choose a strong - (random) secret." + GitHub CI service is disabled. Note that the path must be absolute. + Note: make sure to choose a strong (random) secret." } std::map<string, dir_path> ci-github-app-id-private-key @@ -863,7 +863,7 @@ namespace brep "<id>=<path>", "The private key used during GitHub API authentication for the specified GitHub App ID. Both vales are found in the GitHub App's - settings." + settings. Note that the paths must be absolute." } uint16_t ci-github-jwt-validity-period = 600 diff --git a/mod/tenant-service.cxx b/mod/tenant-service.cxx new file mode 100644 index 0000000..2c1f3bc --- /dev/null +++ b/mod/tenant-service.cxx @@ -0,0 +1,18 @@ +// file : mod/tenant-service.cxx -*- C++ -*- +// license : MIT; see accompanying LICENSE file + +#include <mod/tenant-service.hxx> + +namespace brep +{ + void tenant_service_build_built:: + build_completed (const string& /* tenant_id */, + const tenant_service&, + const diag_epilogue& /* log_writer */) const noexcept + { + // If this notification is requested, then this function needs to be + // overridden by the tenant service implementation. + // + assert (false); + } +} diff --git a/mod/tenant-service.hxx b/mod/tenant-service.hxx index 5564a56..d909eaa 100644 --- a/mod/tenant-service.hxx +++ b/mod/tenant-service.hxx @@ -127,12 +127,20 @@ namespace brep class tenant_service_build_built: public virtual tenant_service_base { public: - virtual function<optional<string> (const string& tenant_id, - const tenant_service&)> + // The second half of the pair signals whether to call the + // build_completed() notification. + // + virtual function<pair<optional<string>, bool> (const string& tenant_id, + const tenant_service&)> build_built (const string& tenant_id, const tenant_service&, const build&, const diag_epilogue& log_writer) const noexcept = 0; + + virtual void + build_completed (const string& tenant_id, + const tenant_service&, + const diag_epilogue& log_writer) const noexcept; }; // This notification is only made on unloaded CI requests created with the |