diff options
| -rw-r--r-- | mod/mod-ci-github-gq.cxx | 16 | ||||
| -rw-r--r-- | mod/mod-ci-github-gq.hxx | 4 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.hxx | 2 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 25 | ||||
| -rw-r--r-- | mod/module.cli | 2 |
5 files changed, 19 insertions, 30 deletions
diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx index c30ab7a..bea9a7f 100644 --- a/mod/mod-ci-github-gq.cxx +++ b/mod/mod-ci-github-gq.cxx @@ -66,8 +66,6 @@ namespace brep // 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. // - // @@ TODO: This function is only called in one place. - // static void gq_parse_response (json::parser& p, function<void (json::parser&)> parse_data) @@ -84,6 +82,8 @@ namespace brep // 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)) @@ -94,13 +94,16 @@ namespace brep // 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. + // json::stream_serializer s (data, 0 /* indentation */); try { for (event e: p) { - if (!s.next (e, p.data (), false /* check */)) + if (!s.next (e, p.data ())) break; // Stop if data object is complete. } } @@ -136,6 +139,9 @@ namespace brep 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 ()); @@ -177,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) @@ -531,7 +537,7 @@ namespace brep // assert (!du.empty ()); - // Set `started at` to current time if updating to building. + // Set `startedAt` to current time if updating to building. // optional<timestamp> sa; diff --git a/mod/mod-ci-github-gq.hxx b/mod/mod-ci-github-gq.hxx index 5ad77e1..8f7a9ca 100644 --- a/mod/mod-ci-github-gq.hxx +++ b/mod/mod-ci-github-gq.hxx @@ -26,10 +26,6 @@ namespace brep // Note: no details_url yet since there will be no entry in the build result // search page until the task starts building. // - // @@ TMP We only create multiple check runs in build_queued() so - // build_state is redundant. Maybe we should rename this - // gq_queue_check_runs()? - // bool gq_create_check_runs (const basic_mark& error, vector<check_run>& check_runs, diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx index 8d0b634..ff1c1a3 100644 --- a/mod/mod-ci-github-service-data.hxx +++ b/mod/mod-ci-github-service-data.hxx @@ -56,7 +56,7 @@ namespace brep 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. diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index e433d44..5aa4e6d 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -39,13 +39,7 @@ // - Check that delivery UUID has not been received before (replay attack). // -// @@ TODO -// -// 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 -// - -// @@ TODO Best practices +// Resources: // // Webhooks: // https://docs.github.com/en/webhooks/using-webhooks/best-practices-for-using-webhooks @@ -53,13 +47,10 @@ // // 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. using namespace std; using namespace butl; @@ -118,7 +109,7 @@ namespace brep // Process headers. // - // @@ TMP Shouldn't we also error<< in some of these header problem cases? + // @@@ TMP Shouldn't we also error<< in some of these header problem cases? // string event; // Webhook event. string hmac; // Received HMAC. @@ -191,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; { @@ -388,7 +379,7 @@ namespace brep .json ()); // @@ What happens if we call this functions with an already existing - // node_id (e.g., replay attack). + // node_id (e.g., replay attack). See the UUID header above. // optional<start_result> r ( start (error, @@ -987,10 +978,6 @@ namespace brep sm = os.str (); } - // @@ Maybe we should map status here according to warning_success - // instead of passing it to gq_*() functions? Let's see how we handle - // the report. - // gq_built_result br (gh_to_conclusion (*b.status, sd.warning_success), circle (*b.status) + ' ' + ucase (to_string (*b.status)), diff --git a/mod/module.cli b/mod/module.cli index 05e9188..a23b62a 100644 --- a/mod/module.cli +++ b/mod/module.cli @@ -815,7 +815,7 @@ 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: repository_url, |