Review

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 4bb4e2f..f4231fc 100644
--- a/mod/module.cli
+++ b/mod/module.cli
@@ -845,7 +845,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,