diff options
| -rwxr-xr-x | libbrep/odb.sh | 7 | ||||
| -rw-r--r-- | libbrep/version.hxx.in | 4 | ||||
| -rw-r--r-- | mod/mod-ci-github-gh.cxx | 31 | ||||
| -rw-r--r-- | mod/mod-ci-github-gh.hxx | 9 | ||||
| -rw-r--r-- | mod/mod-ci-github-gq.cxx | 272 | ||||
| -rw-r--r-- | mod/mod-ci-github-gq.hxx | 48 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.cxx | 15 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.hxx | 10 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 412 | ||||
| -rw-r--r-- | mod/mod-ci-github.hxx | 10 | ||||
| -rw-r--r-- | mod/module.cli | 8 | ||||
| -rw-r--r-- | repositories.manifest | 6 |
12 files changed, 561 insertions, 271 deletions
diff --git a/libbrep/odb.sh b/libbrep/odb.sh index 608ca41..89dc5be 100755 --- a/libbrep/odb.sh +++ b/libbrep/odb.sh @@ -16,6 +16,8 @@ if test -d ../.bdep; then sed -r -ne 's#^(@[^ ]+ )?([^ ]+)/ .*default.*$#\2#p')" fi + # Note: here we use libodb*, not libbutl-odb. + # inc+=("-I$(echo "$cfg"/libodb-[1-9]*/)") inc+=("-I$(echo "$cfg"/libodb-pgsql-[1-9]*/)") @@ -33,6 +35,11 @@ sed -r -ne 's#^(@[^ ]+ )?([^ ]+)/ .*default.*$#\2#p')" else + # Feels like this case should not be necessary (unlike in bpkg/bdep). + # + echo "not bdep-initialized" 1>&2 + exit 1 + inc+=("-I$HOME/work/odb/builds/default/libodb-pgsql-default") inc+=("-I$HOME/work/odb/libodb-pgsql") diff --git a/libbrep/version.hxx.in b/libbrep/version.hxx.in index 3ac3752..9adb5ab 100644 --- a/libbrep/version.hxx.in +++ b/libbrep/version.hxx.in @@ -49,11 +49,11 @@ $libbbot.check(LIBBBOT_VERSION, LIBBBOT_SNAPSHOT)$ #include <odb/version.hxx> -$libodb.check(LIBODB_VERSION, LIBODB_SNAPSHOT)$ +$libodb.check(LIBODB_VERSION_FULL, LIBODB_SNAPSHOT)$ #include <odb/pgsql/version.hxx> -$libodb_pgsql.check(LIBODB_PGSQL_VERSION, LIBODB_PGSQL_SNAPSHOT)$ +$libodb_pgsql.check(LIBODB_PGSQL_VERSION_FULL, LIBODB_PGSQL_SNAPSHOT)$ // For now these are the same. // diff --git a/mod/mod-ci-github-gh.cxx b/mod/mod-ci-github-gh.cxx index 5c4809c..7007db8 100644 --- a/mod/mod-ci-github-gh.cxx +++ b/mod/mod-ci-github-gh.cxx @@ -20,10 +20,9 @@ namespace brep case build_state::queued: return "QUEUED"; case build_state::building: return "IN_PROGRESS"; case build_state::built: return "COMPLETED"; - default: - throw invalid_argument ("invalid build_state value: " + - to_string (static_cast<int> (st))); } + + return ""; // Should never reach. } // Return the build_state corresponding to a GitHub check run status @@ -36,19 +35,21 @@ namespace brep else if (s == "IN_PROGRESS") return build_state::building; else if (s == "COMPLETED") return build_state::built; else - throw invalid_argument ("invalid GitHub check run status: '" + s + + throw invalid_argument ("unexpected GitHub check run status: '" + s + '\''); } string - gh_to_conclusion (result_status rs) + gh_to_conclusion (result_status rs, bool warning_success) { switch (rs) { case result_status::success: - case result_status::warning: return "SUCCESS"; + case result_status::warning: + return warning_success ? "SUCCESS" : "FAILURE"; + case result_status::error: case result_status::abort: case result_status::abnormal: @@ -60,13 +61,9 @@ namespace brep case result_status::interrupt: throw invalid_argument ("unexpected result_status value: " + to_string (rs)); - - // Invalid value. - // - default: - throw invalid_argument ("invalid result_status value: " + - to_string (static_cast<int> (rs))); } + + return ""; // Should never reach. } string @@ -121,7 +118,7 @@ namespace brep { p.next_expect (event::begin_object); - bool ni (false), hb (false), hs (false), bf (false), at (false); + bool ni (false), hb (false), hs (false); // Skip unknown/uninteresting members. // @@ -135,16 +132,12 @@ namespace brep if (c (ni, "node_id")) node_id = p.next_expect_string (); else if (c (hb, "head_branch")) head_branch = p.next_expect_string (); else if (c (hs, "head_sha")) head_sha = p.next_expect_string (); - else if (c (bf, "before")) before = p.next_expect_string (); - else if (c (at, "after")) after = p.next_expect_string (); else p.next_expect_value_skip (); } if (!ni) missing_member (p, "gh_check_suite", "node_id"); if (!hb) missing_member (p, "gh_check_suite", "head_branch"); if (!hs) missing_member (p, "gh_check_suite", "head_sha"); - if (!bf) missing_member (p, "gh_check_suite", "before"); - if (!at) missing_member (p, "gh_check_suite", "after"); } ostream& @@ -152,9 +145,7 @@ namespace brep { os << "node_id: " << cs.node_id << ", head_branch: " << cs.head_branch - << ", head_sha: " << cs.head_sha - << ", before: " << cs.before - << ", after: " << cs.after; + << ", head_sha: " << cs.head_sha; return os; } diff --git a/mod/mod-ci-github-gh.hxx b/mod/mod-ci-github-gh.hxx index 45b5359..b3da197 100644 --- a/mod/mod-ci-github-gh.hxx +++ b/mod/mod-ci-github-gh.hxx @@ -42,15 +42,11 @@ namespace brep // The "check_suite" object within a check_suite webhook event request. // - // @@ TODO Remove unused fields. - // struct gh_check_suite { string node_id; string head_branch; string head_sha; - string before; - string after; explicit gh_check_suite (json::parser&); @@ -81,8 +77,11 @@ namespace brep build_state gh_from_status (const string&); + // If warning_success is true, then map result_status::warning to SUCCESS + // and to FAILURE otherwise. + // string - gh_to_conclusion (result_status); + gh_to_conclusion (result_status, bool warning_success); // Create a check_run name from a build. If the second argument is not // NULL, return an abbreviated id if possible. diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx index 4e13199..bea9a7f 100644 --- a/mod/mod-ci-github-gq.cxx +++ b/mod/mod-ci-github-gq.cxx @@ -35,6 +35,8 @@ namespace brep // Throw runtime_error if the response indicated errors and // invalid_json_input if the GitHub response contained invalid JSON. // + // The parse_data function should not throw anything but invalid_json_input. + // // The response format is defined in the GraphQL spec: // https://spec.graphql.org/October2021/#sec-Response. // @@ -48,10 +50,21 @@ namespace brep // The contents of `data`, including its opening and closing braces, are // parsed by the `parse_data` function. // - // @@ TODO: specify what parse_data may throw (probably only - // invalid_json_input). + // If the `errors` field is present in the response, error(s) occurred + // before or during execution of the operation. + // + // If the `data` field is not present the errors are request errors which + // occur before execution and are typically the client's fault. + // + // If the `data` field is also present in the response the errors are field + // errors which occur during execution and are typically the GraphQL + // endpoint's fault, and some fields in `data` that should not be are likely + // to be null. // - // @@ TODO data comes before errors in GitHub's responses. + // Although the spec recommends that the errors field (if present) should + // come before the data field, GitHub places data before errors. Therefore + // we need to check that the errors field is not present before parsing the + // data field as it might contain nulls if errors is present. // static void gq_parse_response (json::parser& p, @@ -61,13 +74,16 @@ namespace brep // True if the data/errors fields are present. // - // Although the spec merely recommends that the `errors` field, if - // present, comes before the `data` field, assume it always does because - // letting the client parse data in the presence of field errors - // (unexpected nulls) would not make sense. - // bool dat (false), err (false); + // Because the errors field is likely to come before the data field, + // serialize data to a stringstream and only parse it later once we're + // sure there are no errors. + // + ostringstream data; // The value of the data field. + + // @@@ Use iostringstream for both output and input. + p.next_expect (event::begin_object); while (p.next_expect (event::name, event::end_object)) @@ -76,13 +92,27 @@ namespace brep { dat = true; - // Currently we're not handling fields that are null due to field - // errors (see below for details) so don't parse any further. + // Serialize the data field to a string. + // + // Note that the JSON payload sent by GitHub is not pretty-printed so + // there is no need to worry about that. // - if (err) - break; + json::stream_serializer s (data, 0 /* indentation */); - parse_data (p); + try + { + for (event e: p) + { + if (!s.next (e, p.data ())) + break; // Stop if data object is complete. + } + } + catch (const json::invalid_json_output& e) + { + throw_json (p, + string ("serializer rejected response 'data' field: ") + + e.what ()); + } } else if (p.name () == "errors") { @@ -107,23 +137,22 @@ namespace brep } } - // If the `errors` field was present in the response, error(s) occurred - // before or during execution of the operation. - // - // If the `data` field was not present the errors are request errors which - // occur before execution and are typically the client's fault. - // - // If the `data` field was also present in the response the errors are - // field errors which occur during execution and are typically the GraphQL - // endpoint's fault, and some fields in `data` that should not be are - // likely to be null. - // - if (err) + if (!err) + { + if (!dat) + throw runtime_error ("no data received from GraphQL endpoint"); + + // Parse the data field now that we know there are no errors. + // + string d (data.str ()); + json::parser dp (d, p.input_name); + + parse_data (dp); + } + else { if (dat) { - // @@ TODO: Consider parsing partial data? - // throw runtime_error ("field error(s) received from GraphQL endpoint; " "incomplete data received"); } @@ -154,7 +183,7 @@ namespace brep // } // } // - // @@ TODO Handle response errors properly. + // @@@ TODO Handle response errors properly. // static vector<gh_check_run> gq_parse_response_check_runs (json::parser& p) @@ -198,11 +227,11 @@ namespace brep // if unset. Return false and issue diagnostics if the request failed. // static bool - gq_mutate_check_runs (vector<check_run>& crs, + gq_mutate_check_runs (const basic_mark& error, + vector<check_run>& crs, const string& iat, string rq, - build_state st, - const basic_mark& error) noexcept + build_state st) noexcept { vector<gh_check_run> rcrs; @@ -317,16 +346,20 @@ namespace brep // Serialize `createCheckRun` mutations for one or more builds to GraphQL. // - // The result_status 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 build_state is built + // 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. // static string - gq_mutation_create_check_runs ( - const string& ri, // Repository ID - const string& hs, // Head SHA - const vector<reference_wrapper<const build>>& bs, - build_state st, optional<result_status> rs, - const build_queued_hints* bh) + gq_mutation_create_check_runs (const string& ri, // Repository ID + const string& hs, // Head SHA + const string& du, // Details URL. + const vector<check_run>& crs, + const string& st, // Check run status. + optional<gq_built_result> br = nullopt) { ostringstream os; @@ -334,33 +367,36 @@ namespace brep // Serialize a `createCheckRun` for each build. // - for (size_t i (0); i != bs.size (); ++i) + for (size_t i (0); i != crs.size (); ++i) { - const build& b (bs[i]); - string al ("cr" + to_string (i)); // Field alias. - // Check run name. - // - string nm (gh_check_run_name (b, bh)); - os << gq_name (al) << ":createCheckRun(input: {" << '\n' - << " name: " << gq_str (nm) << ',' << '\n' - << " repositoryId: " << gq_str (ri) << ',' << '\n' - << " headSha: " << gq_str (hs) << ',' << '\n' - << " status: " << gq_enum (gh_to_status (st)); - if (rs) + << " name: " << gq_str (crs[i].name) << '\n' + << " repositoryId: " << gq_str (ri) << '\n' + << " headSha: " << gq_str (hs) << '\n' + << " status: " << gq_enum (st); + if (!du.empty ()) + { + os << '\n'; + os << " detailsUrl: " << gq_str (du); + } + if (br) { - os << ',' << '\n' - << " conclusion: " << gq_enum (gh_to_conclusion (*rs)); + 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' // Specify the selection set (fields to be returned). // << "{" << '\n' << " checkRun {" << '\n' - << " id," << '\n' - << " name," << '\n' + << " id" << '\n' + << " name" << '\n' << " status" << '\n' << " }" << '\n' << "}" << '\n'; @@ -373,34 +409,51 @@ namespace brep // Serialize an `updateCheckRun` mutation for one build to GraphQL. // - // The result_status is required if the build_state is built because GitHub - // does not allow updating a check run to completed without a conclusion. + // The `co` (conclusion) argument is required if the build_state is built + // because GitHub does not allow updating a check run to completed without a + // conclusion. // static string - gq_mutation_update_check_run (const string& ri, // Repository ID. - const string& ni, // Node ID. - build_state st, - optional<result_status> rs) + gq_mutation_update_check_run (const string& ri, // Repository ID. + const string& ni, // Node ID. + const string& du, // Details URL. + const string& st, // Check run status. + optional<timestamp> sa, // Started at. + optional<gq_built_result> br) { ostringstream os; os << "mutation {" << '\n' << "cr0:updateCheckRun(input: {" << '\n' - << " checkRunId: " << gq_str (ni) << ',' << '\n' - << " repositoryId: " << gq_str (ri) << ',' << '\n' - << " status: " << gq_enum (gh_to_status (st)); - if (rs) + << " checkRunId: " << gq_str (ni) << '\n' + << " repositoryId: " << gq_str (ri) << '\n' + << " status: " << gq_enum (st); + if (sa) + { + os << '\n'; + os << " startedAt: " << gq_str (gh_to_iso8601 (*sa)); + } + if (!du.empty ()) { - os << ',' << '\n' - << " conclusion: " << gq_enum (gh_to_conclusion (*rs)); + 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' // Specify the selection set (fields to be returned). // << "{" << '\n' << " checkRun {" << '\n' - << " id," << '\n' - << " name," << '\n' + << " id" << '\n' + << " name" << '\n' << " status" << '\n' << " }" << '\n' << "}" << '\n' @@ -410,51 +463,56 @@ namespace brep } bool - gq_create_check_runs (vector<check_run>& crs, + gq_create_check_runs (const basic_mark& error, + vector<check_run>& crs, const string& iat, const string& rid, const string& hs, - const vector<reference_wrapper<const build>>& bs, - build_state st, - const build_queued_hints& bh, - const basic_mark& error) + build_state st) { // No support for result_status so state cannot be built. // assert (st != build_state::built); - string rq (gq_serialize_request ( - gq_mutation_create_check_runs (rid, - hs, - bs, - st, - nullopt /* result_status */, - &bh))); + // Empty details URL because it's not available until building. + // + string rq ( + gq_serialize_request ( + gq_mutation_create_check_runs (rid, hs, "", crs, gh_to_status (st)))); - return gq_mutate_check_runs (crs, iat, move (rq), st, error); + return gq_mutate_check_runs (error, crs, iat, move (rq), st); } bool - gq_create_check_run (check_run& cr, + gq_create_check_run (const basic_mark& error, + check_run& cr, const string& iat, const string& rid, const string& hs, - const build& b, + const string& du, build_state st, - optional<result_status> rs, - const build_queued_hints& bh, - const basic_mark& error) + optional<gq_built_result> br) { // Must have a result if state is built. // - assert (st != build_state::built || rs); + assert (st != build_state::built || br); - string rq (gq_serialize_request ( - gq_mutation_create_check_runs (rid, hs, {b}, st, rs, &bh))); + // Must have a details URL because `st` should never be queued. + // + assert (!du.empty ()); vector<check_run> crs {move (cr)}; - bool r (gq_mutate_check_runs (crs, iat, move (rq), st, error)); + string rq ( + gq_serialize_request ( + gq_mutation_create_check_runs (rid, + hs, + du, + crs, + gh_to_status (st), + move (br)))); + + bool r (gq_mutate_check_runs (error, crs, iat, move (rq), st)); cr = move (crs[0]); @@ -462,24 +520,42 @@ namespace brep } bool - gq_update_check_run (check_run& cr, + gq_update_check_run (const basic_mark& error, + check_run& cr, const string& iat, const string& rid, const string& nid, + const string& du, build_state st, - optional<result_status> rs, - const basic_mark& error) + optional<gq_built_result> br) { // Must have a result if state is built. // - assert (st != build_state::built || rs); + assert (st != build_state::built || br); + + // Must have a details URL for building and built. + // + assert (!du.empty ()); + + // Set `startedAt` to current time if updating to building. + // + optional<timestamp> sa; + + if (st == build_state::building) + sa = system_clock::now (); - string rq (gq_serialize_request ( - gq_mutation_update_check_run (rid, nid, st, rs))); + string rq ( + gq_serialize_request ( + gq_mutation_update_check_run (rid, + nid, + du, + gh_to_status (st), + sa, + move (br)))); vector<check_run> crs {move (cr)}; - bool r (gq_mutate_check_runs (crs, iat, move (rq), st, error)); + bool r (gq_mutate_check_runs (error, crs, iat, move (rq), st)); cr = move (crs[0]); diff --git a/mod/mod-ci-github-gq.hxx b/mod/mod-ci-github-gq.hxx index 9331b2b..8f7a9ca 100644 --- a/mod/mod-ci-github-gq.hxx +++ b/mod/mod-ci-github-gq.hxx @@ -23,35 +23,41 @@ namespace brep // the new states and node IDs. Return false and issue diagnostics if the // request failed. // + // Note: no details_url yet since there will be no entry in the build result + // search page until the task starts building. + // bool - gq_create_check_runs (vector<check_run>& check_runs, + gq_create_check_runs (const basic_mark& error, + vector<check_run>& check_runs, const string& installation_access_token, const string& repository_id, const string& head_sha, - const vector<reference_wrapper<const build>>&, - build_state, - const build_queued_hints&, - const basic_mark& error); + build_state); // Create a new check run on GitHub for a build. Update `cr` with the new // state and the node ID. Return false and issue diagnostics if the request // failed. // - // The result_status is required if the build_state is built because GitHub - // does not allow a check run status of `completed` without a conclusion. - // - // @@ TODO Support output (title, summary, text). + // 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. // + struct gq_built_result + { + string conclusion; + string title; + string summary; + }; + bool - gq_create_check_run (check_run& cr, + 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 build&, + const string& details_url, build_state, - optional<result_status>, - const build_queued_hints&, - const basic_mark& error); + optional<gq_built_result> = nullopt); // Update a check run on GitHub. // @@ -59,19 +65,19 @@ namespace brep // with the new state. Return false and issue diagnostics if the request // failed. // - // The result_status is required if the build_state is built because GitHub - // does not allow updating a check run to `completed` without a conclusion. - // - // @@ TODO Support output (title, summary, text). + // 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_update_check_run (check_run& cr, + gq_update_check_run (const basic_mark& error, + check_run& cr, const string& installation_access_token, const string& repository_id, const string& node_id, + const string& details_url, build_state, - optional<result_status>, - const basic_mark& error); + optional<gq_built_result> = nullopt); } #endif // MOD_MOD_CI_GITHUB_GQ_HXX diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx index f79550c..83e952b 100644 --- a/mod/mod-ci-github-service-data.cxx +++ b/mod/mod-ci-github-service-data.cxx @@ -26,6 +26,8 @@ namespace brep to_string (version)); } + warning_success = p.next_expect_member_boolean<bool> ("warning_success"); + // Installation access token. // p.next_expect_member_object ("installation_access"); @@ -43,6 +45,7 @@ namespace brep while (p.next_expect (event::begin_object, event::end_array)) { string bid (p.next_expect_member_string ("build_id")); + string nm (p.next_expect_member_string ("name")); optional<string> nid; { @@ -54,7 +57,7 @@ namespace brep build_state s (to_build_state (p.next_expect_member_string ("state"))); bool ss (p.next_expect_member_boolean<bool> ("state_synced")); - check_runs.emplace_back (move (bid), move (nid), s, ss); + check_runs.emplace_back (move (bid), move (nm), move (nid), s, ss); p.next_expect (event::end_object); } @@ -63,12 +66,14 @@ namespace brep } service_data:: - service_data (string iat_tok, + service_data (bool ws, + string iat_tok, timestamp iat_ea, uint64_t iid, string rid, string hs) - : installation_access (move (iat_tok), iat_ea), + : warning_success (ws), + installation_access (move (iat_tok), iat_ea), installation_id (iid), repository_id (move (rid)), head_sha (move (hs)) @@ -85,6 +90,8 @@ namespace brep s.member ("version", 1); + s.member ("warning_success", warning_success); + // Installation access token. // s.member_begin_object ("installation_access"); @@ -101,6 +108,7 @@ namespace brep { s.begin_object (); s.member ("build_id", cr.build_id); + s.member ("name", cr.name); s.member_name ("node_id"); if (cr.node_id) @@ -136,6 +144,7 @@ namespace brep { os << "node_id: " << cr.node_id.value_or ("null") << ", build_id: " << cr.build_id + << ", name: " << cr.name << ", state: " << cr.state_string (); return os; diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx index d4a47d5..ff1c1a3 100644 --- a/mod/mod-ci-github-service-data.hxx +++ b/mod/mod-ci-github-service-data.hxx @@ -25,6 +25,7 @@ namespace brep struct check_run { string build_id; // Full build id. + string name; // Potentially shortened build id. optional<string> node_id; // GitHub id. build_state state; @@ -46,12 +47,16 @@ namespace brep // uint64_t version = 1; + // Check suite settings. + // + bool warning_success; // See gh_to_conclusion(). + // Check suite-global data. // gh_installation_access_token installation_access; uint64_t installation_id; - // @@ TODO Rename to repository_node_id. + // @@@ TODO Rename to repository_node_id. // string repository_id; // GitHub-internal opaque repository id. @@ -72,7 +77,8 @@ namespace brep explicit service_data (const string& json); - service_data (string iat_token, + service_data (bool warning_success, + string iat_token, timestamp iat_expires_at, uint64_t installation_id, string repository_id, diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index 9449f97..5aa4e6d 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,13 +19,27 @@ #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. // -// 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 +// 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 +// +// - Check that delivery UUID has not been received before (replay attack). // -// @@ TODO Best practices +// Resources: // // Webhooks: // https://docs.github.com/en/webhooks/using-webhooks/best-practices-for-using-webhooks @@ -29,18 +47,11 @@ // // REST API: // https://docs.github.com/en/rest/using-the-rest-api/best-practices-for-using-the-rest-api?apiVersion=2022-11-28 +// @@@ Add link to GraphQL? // // Creating an App: // https://docs.github.com/en/apps/creating-github-apps/about-creating-github-apps/best-practices-for-creating-a-github-app -// -// 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; @@ -98,14 +109,7 @@ namespace brep // 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? + // @@@ TMP Shouldn't we also error<< in some of these header problem cases? // string event; // Webhook event. string hmac; // Received HMAC. @@ -178,8 +182,8 @@ namespace brep // 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 +230,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 +269,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 +295,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 +312,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; } @@ -305,7 +344,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 +370,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"; @@ -411,8 +453,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 @@ -501,6 +543,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 +580,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)) + build_state::queued)) { for (const check_run& cr: crs) { @@ -603,7 +644,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); @@ -674,13 +716,13 @@ namespace brep // if (iat != nullptr) { - if (gq_update_check_run (*cr, + if (gq_update_check_run (error, + *cr, iat->token, sd.repository_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 +785,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 +803,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 +858,143 @@ namespace brep // if (iat != nullptr) { + // Return the colored circle corresponding to a result_status. + // + auto circle = [] (result_status rs) -> string + { + 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. + }; + + // 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, circle, &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, *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 +1011,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)) + details_url (b), + build_state::built, + move (br))) { assert (cr.state == build_state::built); @@ -861,40 +1031,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:: diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx index 07feca8..6bb01e3 100644 --- a/mod/mod-ci-github.hxx +++ b/mod/mod-ci-github.hxx @@ -60,8 +60,16 @@ namespace brep // Handle the check_suite event `requested` and `rerequested` actions. // + // If warning_success is true, then map result_status::warning to SUCCESS + // and to FAILURE otherwise. + // bool - handle_check_suite_request (gh_check_suite_event); + handle_check_suite_request (gh_check_suite_event, bool warning_success); + + // Build a check run details_url for a build. + // + string + details_url (const build&) const; optional<string> generate_jwt (const basic_mark& trace, const basic_mark& error) const; diff --git a/mod/module.cli b/mod/module.cli index 166adbd..a23b62a 100644 --- a/mod/module.cli +++ b/mod/module.cli @@ -815,10 +815,14 @@ namespace brep } }; - // @@ TODO Is etc/brep-module.conf updated manually? Yes, will need to + // @@@ TODO Is etc/brep-module.conf updated manually? Yes, will need to // replicate there eventually. // - class ci_github: ci_start, ci_cancel, build_db, handler, openssl_options + class ci_github: repository_url, + ci_start, ci_cancel, + build_db, + handler, + openssl_options { // GitHub CI-specific options. // diff --git a/repositories.manifest b/repositories.manifest index da9ee2b..faed09f 100644 --- a/repositories.manifest +++ b/repositories.manifest @@ -35,11 +35,7 @@ location: https://git.build2.org/packaging/cmark-gfm/cmark-gfm.git##HEAD : role: prerequisite -location: https://git.codesynthesis.com/odb/libodb.git##HEAD - -: -role: prerequisite -location: https://git.codesynthesis.com/odb/libodb-pgsql.git##HEAD +location: https://git.codesynthesis.com/odb/odb.git##HEAD : role: prerequisite |