Post-review changes

6 files changed, 45 insertions, 32 deletions

diff --git a/etc/brep-module.conf b/etc/brep-module.conf
index d5a5e78..c1fd26b 100644
--- a/etc/brep-module.conf
+++ b/etc/brep-module.conf
@@ -439,6 +439,28 @@ menu About=?about
 # ci-handler-timeout
 
 
+# The GitHub App ID. Found in the app's settings on GitHub.
+#
+# ci-github-app-id
+
+
+# The GitHub app's configured webhook secret.
+#
+# ci-github-app-webhook-secret
+
+
+# The private key used during GitHub API authentication. Created in the GitHub
+# app's settings.
+#
+# ci-github-app-private-key
+
+
+# The number of seconds a JWT (authentication token) should be valid for. The
+# maximum allowed by GitHub is 10 minutes.
+#
+# ci-github-jwt-validity-period 600
+
+
 # The directory to save upload data to for the specified upload type. If
 # unspecified, the build artifacts upload functionality will be disabled for
 # this type.
diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx
index bea9a7f..954f5a8 100644
--- a/mod/mod-ci-github-gq.cxx
+++ b/mod/mod-ci-github-gq.cxx
@@ -80,9 +80,7 @@ namespace brep
     // 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.
+    stringstream data; // The value of the data field.
 
     p.next_expect (event::begin_object);
 
@@ -144,8 +142,7 @@ namespace brep
 
       // Parse the data field now that we know there are no errors.
       //
-      string d (data.str ());
-      json::parser dp (d, p.input_name);
+      json::parser dp (data, p.input_name);
 
       parse_data (dp);
     }
@@ -183,8 +180,6 @@ namespace brep
   //    }
   //  }
   //
-  // @@@ TODO Handle response errors properly.
-  //
   static vector<gh_check_run>
   gq_parse_response_check_runs (json::parser& p)
   {
diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx
index 83e952b..f1d5fd5 100644
--- a/mod/mod-ci-github-service-data.cxx
+++ b/mod/mod-ci-github-service-data.cxx
@@ -38,7 +38,7 @@ namespace brep
 
     installation_id =
         p.next_expect_member_number<uint64_t> ("installation_id");
-    repository_id = p.next_expect_member_string ("repository_id");
+    repository_node_id = p.next_expect_member_string ("repository_id");
     head_sha = p.next_expect_member_string ("head_sha");
 
     p.next_expect_member_array ("check_runs");
@@ -75,7 +75,7 @@ namespace brep
       : warning_success (ws),
         installation_access (move (iat_tok), iat_ea),
         installation_id (iid),
-        repository_id (move (rid)),
+        repository_node_id (move (rid)),
         head_sha (move (hs))
   {
   }
@@ -100,7 +100,7 @@ namespace brep
     s.end_object ();
 
     s.member ("installation_id", installation_id);
-    s.member ("repository_id", repository_id);
+    s.member ("repository_id", repository_node_id);
     s.member ("head_sha", head_sha);
 
     s.member_begin_array ("check_runs");
diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx
index ff1c1a3..c4e20b3 100644
--- a/mod/mod-ci-github-service-data.hxx
+++ b/mod/mod-ci-github-service-data.hxx
@@ -56,9 +56,8 @@ namespace brep
     gh_installation_access_token installation_access;
 
     uint64_t installation_id;
-    // @@@ TODO Rename to repository_node_id.
-    //
-    string repository_id; // GitHub-internal opaque repository id.
+
+    string repository_node_id; // GitHub-internal opaque repository id.
 
     string head_sha;
 
@@ -81,7 +80,7 @@ namespace brep
                   string iat_token,
                   timestamp iat_expires_at,
                   uint64_t installation_id,
-                  string repository_id,
+                  string repository_node_id,
                   string head_sha);
 
     service_data () = default;
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 5aa4e6d..a99e516 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -41,16 +41,20 @@
 
 // Resources:
 //
+//    Creating an App:
+//    https://docs.github.com/en/apps/creating-github-apps/about-creating-github-apps/best-practices-for-creating-a-github-app
+//
 //    Webhooks:
 //    https://docs.github.com/en/webhooks/using-webhooks/best-practices-for-using-webhooks
 //    https://docs.github.com/en/webhooks/using-webhooks/validating-webhook-deliveries
 //
 //    REST API:
-//    https://docs.github.com/en/rest/using-the-rest-api/best-practices-for-using-the-rest-api?apiVersion=2022-11-28
-//    @@@ Add link to GraphQL?
+//    All docs:       https://docs.github.com/en/rest#all-docs
+//    Best practices: https://docs.github.com/en/rest/using-the-rest-api/best-practices-for-using-the-rest-api
+//
+//    GraphQL API:
+//    Reference: https://docs.github.com/en/graphql/reference
 //
-//    Creating an App:
-//    https://docs.github.com/en/apps/creating-github-apps/about-creating-github-apps/best-practices-for-creating-a-github-app
 
 using namespace std;
 using namespace butl;
@@ -109,8 +113,6 @@ namespace brep
 
     // Process headers.
     //
-    // @@@ TMP Shouldn't we also error<< in some of these header problem cases?
-    //
     string event; // Webhook event.
     string hmac;  // Received HMAC.
     {
@@ -529,12 +531,11 @@ namespace brep
         }
         else if (*istate == build_state::built)
         {
-          // Unexpectd built->queued transition (rebuild).
+          // Unexpected built->queued transition (rebuild).
           //
           warn << "check run " << bid << ": unexpected rebuild";
         }
-        else
-          ; // Ignore interrupted.
+        else {} // Ignore interrupted.
       }
       else
       {
@@ -583,7 +584,7 @@ namespace brep
       if (gq_create_check_runs (error,
                                 crs,
                                 iat->token,
-                                sd.repository_id, sd.head_sha,
+                                sd.repository_node_id, sd.head_sha,
                                 build_state::queued))
       {
         for (const check_run& cr: crs)
@@ -677,8 +678,7 @@ namespace brep
           cr = move (*scr);
           cr->state_synced = false;
         }
-        else
-          ; // Network error during queued notification, ignore.
+        else {} // Network error during queued notification, ignore.
       }
       else
         warn << "check run " << bid << ": out of order building "
@@ -719,7 +719,7 @@ namespace brep
       if (gq_update_check_run (error,
                                *cr,
                                iat->token,
-                               sd.repository_id,
+                               sd.repository_node_id,
                                *cr->node_id,
                                details_url (b),
                                build_state::building))
@@ -990,7 +990,7 @@ namespace brep
         if (gq_update_check_run (error,
                                  cr,
                                  iat->token,
-                                 sd.repository_id,
+                                 sd.repository_node_id,
                                  *cr.node_id,
                                  details_url (b),
                                  build_state::built,
@@ -1014,7 +1014,7 @@ namespace brep
         if (gq_create_check_run (error,
                                  cr,
                                  iat->token,
-                                 sd.repository_id,
+                                 sd.repository_node_id,
                                  sd.head_sha,
                                  details_url (b),
                                  build_state::built,
diff --git a/mod/module.cli b/mod/module.cli
index a23b62a..a809a81 100644
--- a/mod/module.cli
+++ b/mod/module.cli
@@ -815,9 +815,6 @@ namespace brep
       }
     };
 
-    // @@@ TODO Is etc/brep-module.conf updated manually? Yes, will need to
-    //    replicate there eventually.
-    //
     class ci_github: repository_url,
                      ci_start, ci_cancel,
                      build_db,