aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorBoris Kolpackov <boris@codesynthesis.com>2024-06-06 12:51:52 +0200
committerFrancois Kritzinger <francois@codesynthesis.com>2024-10-15 09:05:28 +0200
commite0d074e946c482a3cf497b89a883e955928519ca (patch)
tree27cb438175449835eaa925fdebc4b1d1b835550e
parent1011d1bd0acadd7e8267137ceb4e3b9af866f157 (diff)
Review
-rw-r--r--mod/mod-ci-github-gq.cxx27
-rw-r--r--mod/mod-ci-github.cxx7
-rw-r--r--mod/mod-ci-github.hxx8
3 files changed, 32 insertions, 10 deletions
diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx
index 26b4c1f..df81854 100644
--- a/mod/mod-ci-github-gq.cxx
+++ b/mod/mod-ci-github-gq.cxx
@@ -628,9 +628,13 @@ namespace brep
{
if (ma == "CONFLICTING")
value = "";
- else if (ma == "UNKNOWN")
+ if (ma == "UNKNOWN")
+ ; // Still being generated; leave value absent.
+ else
{
- // Still being generated; leave value absent.
+ error << "unexpected mergeable value '" << ma << "'";
+
+ // Carry on as if it were UNKNOWN.
}
// Skip the merge commit ID (which should be null).
@@ -697,7 +701,18 @@ namespace brep
// page) open pull requests with the specified base branch from the
// repository with the specified node ID.
//
- // @@ TMP Should we support more than 100?
+ // @@ TMP Should we support more/less than 100?
+ //
+ // Doing more (or even 100) could waste a lot of CI resources on
+ // re-testing stale PRs. Maybe we should create a failed synthetic
+ // conclusion check run asking the user to re-run the CI manually if/when
+ // needed.
+ //
+ // Note that we cannot request more than 100 at a time (will need to
+ // do multiple requests with paging, etc).
+ //
+ // Also, maybe we should limit the result to "fresh" PRs, e.g., those
+ // that have been "touched" in the last week.
//
// Example query:
//
@@ -789,7 +804,7 @@ namespace brep
{
using event = json::event;
- gq_parse_response (p, [this] (json::parser& p)
+ auto parse_data = [this] (json::parser& p)
{
p.next_expect (event::begin_object);
@@ -825,7 +840,9 @@ namespace brep
}
p.next_expect (event::end_object);
- });
+ };
+
+ gq_parse_response (p, move (parse_data));
}
resp () = default;
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 8ea730f..70c1c7d 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -495,7 +495,8 @@ namespace brep
// The merge commits of any open pull requests with this branch as base
// branch will now be out of date, and thus so will be their CI builds and
- // associated check runs.
+ // associated check runs (and, no, GitHub does not invalidate those CI
+ // results automatically; see below).
//
// Unfortunately GitHub does not provide a webhook for PR base branch
// updates (as it does for PR head branch updates) so we have to handle it
@@ -512,7 +513,9 @@ namespace brep
// user would be able to merge a broken PR.
//
// Regardless of the nature of the error, we have to let the check suite
- // handling code proceed so we only issue diagnostics.
+ // handling code proceed so we only issue diagnostics. Note also that we
+ // want to run this code as early as possible to minimize the window of
+ // the user seeing misleading CI results.
//
{
// Fetch open pull requests with the check suite's head branch as base
diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx
index 07295f2..489aac7 100644
--- a/mod/mod-ci-github.hxx
+++ b/mod/mod-ci-github.hxx
@@ -17,6 +17,8 @@
namespace brep
{
+ struct service_data;
+
class ci_github: public database_module,
private ci_start,
public tenant_service_build_unloaded,
@@ -83,8 +85,8 @@ namespace brep
// true, cancel its existing CI request first.
//
// Return true if an unloaded CI request was created. Ignore failure to
- // cancel because the CI request may already have been cancelled for a
- // legitimate reason.
+ // cancel because the CI request may already have been cancelled for other
+ // reasons.
//
// After this call we will start getting the build_unloaded()
// notifications until (1) we load the request, (2) we cancel it, or (3)
@@ -94,7 +96,7 @@ namespace brep
create_pull_request_ci (const basic_mark& error,
const basic_mark& warn,
const basic_mark& trace,
- string service_data,
+ const service_data&,
const string& pull_request_node_id,
bool cancel_first) const;