aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorFrancois Kritzinger <francois@codesynthesis.com>2024-05-28 13:24:30 +0200
committerFrancois Kritzinger <francois@codesynthesis.com>2024-10-15 09:05:28 +0200
commitb73ae12b1e28aff88a4f2a4a1ee466014f8a7cd4 (patch)
treeab0e9f88eb662fe1ad296352313917860262b1e1
parent38331d787cf559a2ce9d6d2b3b2c920c2225c98c (diff)
Post-review changes
-rw-r--r--mod/mod-ci-github-gq.cxx12
-rw-r--r--mod/mod-ci-github-service-data.cxx45
-rw-r--r--mod/mod-ci-github.cxx352
3 files changed, 218 insertions, 191 deletions
diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx
index 54822ae..d8a1a0e 100644
--- a/mod/mod-ci-github-gq.cxx
+++ b/mod/mod-ci-github-gq.cxx
@@ -575,7 +575,7 @@ namespace brep
return os.str ();
}
- pair<optional<gq_pr_mergeability>, bool>
+ optional<string>
gq_pull_request_mergeable (const basic_mark& error,
const string& iat,
const string& nid)
@@ -595,7 +595,7 @@ namespace brep
// The response value. Absent if the merge commit is still being
// generated.
//
- optional<gq_pr_mergeability> value;
+ optional<string> value;
resp (json::parser& p)
{
@@ -617,11 +617,11 @@ namespace brep
string oid (p.next_expect_member_string ("oid"));
p.next_expect (event::end_object);
- value = {true, move (oid)};
+ value = move (oid);
}
else if (ma == "CONFLICTING")
{
- value = {false, ""};
+ value = "";
}
else if (ma == "UNKNOWN")
{
@@ -648,7 +648,7 @@ namespace brep
if (!rs.found)
error << "pull request '" << nid << "' not found";
- return {move (rs.value), rs.found};
+ return move (rs.value);
}
else
error << "failed to fetch pull request: error HTTP response status "
@@ -679,7 +679,7 @@ namespace brep
error << "unable to fetch pull request: " << e;
}
- return {nullopt, false};
+ return nullopt;
}
// bool
diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx
index 8c6970c..7fd554d 100644
--- a/mod/mod-ci-github-service-data.cxx
+++ b/mod/mod-ci-github-service-data.cxx
@@ -43,12 +43,18 @@ namespace brep
{
string* s (p.next_expect_member_string_null ("repository_clone_url"));
- if (s)
+ if (s != nullptr)
repository_clone_url = *s;
}
pr_number = p.next_expect_member_number_null<uint32_t> ("pr_number");
+ {
+ string* s (p.next_expect_member_string_null ("merge_node_id"));
+ if (s != nullptr)
+ merge_node_id = *s;
+ }
+
head_sha = p.next_expect_member_string ("head_sha");
p.next_expect_member_array ("check_runs");
@@ -72,6 +78,12 @@ namespace brep
p.next_expect (event::end_object);
}
+ {
+ string* s (p.next_expect_member_string_null ("conclusion_node_id"));
+ if (s != nullptr)
+ conclusion_node_id = *s;
+ }
+
p.next_expect (event::end_object);
}
@@ -81,9 +93,24 @@ namespace brep
timestamp iat_ea,
uint64_t iid,
string rid,
+ string hs)
+ : warning_success (ws),
+ installation_access (move (iat_tok), iat_ea),
+ installation_id (iid),
+ repository_node_id (move (rid)),
+ head_sha (move (hs))
+ {
+ }
+
+ service_data::
+ service_data (bool ws,
+ string iat_tok,
+ timestamp iat_ea,
+ uint64_t iid,
+ string rid,
string hs,
- optional<string> rcu,
- optional<uint32_t> prn)
+ string rcu,
+ uint32_t prn)
: warning_success (ws),
installation_access (move (iat_tok), iat_ea),
installation_id (iid),
@@ -128,6 +155,12 @@ namespace brep
else
s.value (nullptr);
+ s.member_name ("merge_node_id");
+ if (merge_node_id)
+ s.value (*merge_node_id);
+ else
+ s.value (nullptr);
+
s.member ("head_sha", head_sha);
s.member_begin_array ("check_runs");
@@ -150,6 +183,12 @@ namespace brep
}
s.end_array ();
+ s.member_name ("conclusion_node_id");
+ if (conclusion_node_id)
+ s.value (*conclusion_node_id);
+ else
+ s.value (nullptr);
+
s.end_object ();
return b;
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index f9fd453..be9daa9 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -650,15 +650,89 @@ namespace brep
if (iat == nullptr)
return nullptr;
- bool first (...); // @@ TODO
+ // Synthetic merge check run node ID. Absent if the check run was created
+ // in a previous call (and has already been stored in the service data).
+ //
+ optional<string> mni;
+
+ // Create an in-progress check run. Return the check run on success or
+ // nullopt on failure.
+ //
+ auto create_cr = [iat, &sd, &error] (string name) -> optional<check_run>
+ {
+ check_run cr;
+ cr.name = move (name);
+
+ if (gq_create_check_run (error,
+ cr,
+ iat->token,
+ sd.repository_node_id,
+ sd.head_sha,
+ // @@ TODO What details URL to use?
+ "https://build2.org", // details URL.
+ build_state::building))
+ {
+ return cr;
+ }
+ else
+ return nullopt;
+ };
+
+ // Update the merge check run with success or failure. Return the check
+ // run on success or nullopt on failure.
+ //
+ auto update_merge_cr = [iat, &sd, &mni, &error] (result_status rs,
+ const string& summary)
+ -> optional<check_run>
+ {
+ optional<gq_built_result> br (
+ gq_built_result (gh_to_conclusion (rs, sd.warning_success),
+ circle (rs) + ' ' + ucase (to_string (rs)),
+ move (summary)));
+
+ check_run cr;
+ cr.name = "merge-commit"; // For display purposes only.
+
+ const string& ni (mni ? *mni : *sd.merge_node_id); // Node ID.
+
+ if (gq_update_check_run (error,
+ cr,
+ iat->token,
+ sd.repository_node_id,
+ ni,
+ "https://build2.org", // details URL.
+ build_state::built,
+ move (br)))
+ {
+ return cr;
+ }
+ else
+ return nullopt;
+ };
+
+ // True if this is the first call (or the merge commit couldn't be created
+ // on the first call).
+ //
+ bool first (!sd.merge_node_id);
// If this is the first call, (re)create the synthetic merge check run as
// soon as possible to make sure the previous check suite, if any, is no
// longer completed.
//
+ // @@ TMP There is still a window between receipt of the pull_request
+ // event and the first bot/worker asking for a task. That could be
+ // minutes, right?
+ //
if (first)
{
- // TODO: in-progress
+ if (auto cr = create_cr ("merge-commit"))
+ {
+ l3 ([&]{trace << "created check_run { " << *cr << " }";});
+
+ mni = move (cr->node_id);
+ }
+ else
+ return nullptr; // Try again on the next call.
}
// Start/check PR mergeability.
@@ -668,7 +742,7 @@ namespace brep
if (!mc) // No merge commit yet.
{
- // If this is a subseqent notification and there is no merge commit,
+ // If this is a subsequent notification and there is no merge commit,
// then there is nothing to do.
//
if (!first)
@@ -679,217 +753,139 @@ namespace brep
//
return [&error,
iat = move (new_iat),
- cr = move (cr),
- ccr = move (ccr)] (const tenant_service& ts) -> optional<string>
+ mni = move (mni)] (const tenant_service& ts) -> optional<string>
{
- // @@ TODO
- }
+ // 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;
+ }
+
+ if (iat)
+ sd.installation_access = *iat;
+
+ if (mni)
+ sd.merge_node_id = *mni;
+
+ return sd.json ();
+ };
}
- else if (mc->empty ()) // Not meargable.
+ else if (mc->empty ()) // Not mergeable.
{
- // If the commit is not mergable, cancel the CI request and fail the
+ // If the commit is not mergeable, cancel the CI request and fail the
// merge check run.
//
// Note that it feels like in this case we don't really need to create a
// failed synthetic conclusion check run since the PR cannot be merged
// anyway.
- // @@ TODO: cancel CI request
- // @@ TODO: fail merge check run and update in service data.
+ if (auto cr = update_merge_cr (result_status::error,
+ "merge would create conflicts"))
+ {
+ l3 ([&]{trace << "updated check_run { " << *cr << " }";});
+
+ // @@ TODO: cancel CI request
+ }
+ // Don't cancel the CI request if the merge check run update failed so
+ // that we can try again on the next call.
return [&error,
iat = move (new_iat),
- cr = move (cr),
- ccr = move (ccr)] (const tenant_service& ts) -> optional<string>
+ mni = move (mni)] (const tenant_service& ts) -> optional<string>
{
- // @@ TODO
- }
- }
-
- // If we are here, then it means we have a merge commit that we can load.
- //
-
- // As a first step, (re)create the synthetic conclusion check run and then
- // change the merge check run state to success. Do it in this order so
- // that the check suite does not become completed.
-
- // @@ TODO: create synthetic conclusion check run (in progress)
- // @@ TODO: update synthetic merge checj run (success)
- // @@ TODO: will need to update conclusion check run node_id in service data
+ // NOTE: this lambda may be called repeatedly (e.g., due to
+ // transaction being aborted) and so should not move out of its
+ // captures.
- // @@ TODO: load CI request.
+ service_data sd;
+ try
+ {
+ sd = service_data (*ts.data);
+ }
+ catch (const invalid_argument& e)
+ {
+ error << "failed to parse service data: " << e;
+ return nullopt;
+ }
- {
- pair<optional<gq_pr_mergeability>, bool> pr (
- gq_pull_request_mergeable (error, iat->token, ts.id));
+ if (iat)
+ sd.installation_access = *iat;
- if (pr.second) // No errors.
- {
- if (pr.first) // Merge commit has been generated.
- ma = move (pr.first);
- }
- else
- {
- error << "failed to get pull request " << ts.id << " mergeability";
+ if (mni)
+ sd.merge_node_id = mni;
- return nullptr;
- }
+ return sd.json ();
+ };
}
- // The merge commit's build state. Note that the PR fetch above initiated
- // its generation on GitHub.
- //
- build_state bs;
-
- // The check run result to use if the merge commit has been calculated, or
- // absent if it has not.
+ // If we are here, then it means we have a merge commit that we can load.
//
- optional<gq_built_result> br;
-
- if (ma)
- {
- bs = build_state::built;
-
- result_status rs;
- string sm; // Summary.
-
- if (ma->mergeable)
- {
- rs = result_status::success;
- sm = "merge would succeed";
- }
- else
- {
- rs = result_status::error;
- sm = "merge would create conflicts";
- }
-
- br = gq_built_result (gh_to_conclusion (rs, sd.warning_success),
- circle (rs) + ' ' + ucase (to_string (rs)),
- move (sm));
- }
- else
- bs = build_state::building;
- // The stored merge commit check run.
- //
- check_run* scr (sd.find_check_run ("merge-commit"));
+ // As a first step, (re)create the synthetic conclusion check run and then
+ // change the merge check run state to success. Do it in this order so
+ // that the check suite does not become completed.
- // The updated merge commit check run.
+ // Conclusion check run node ID. Absent if the check run was created in a
+ // previous call (and has already been stored in the service data).
//
- check_run cr;
+ optional<string> cni;
- if (scr == nullptr || !scr->node_id)
+ if (!sd.conclusion_node_id)
{
- // Create the check run because the merge commit check run has not
- // previously been stored, or we failed to create it last time we
- // tried.
- //
- // @@ TMP There is still a window between receipt of the pull_request
- // event and the first bot/worker asking for a task. Shouldn't we
- // rather create the merge CR when we create the unloaded CI
- // build (in the pull_request handler)?
- //
- if (scr != nullptr)
- cr = move (*scr);
- else
+ if (auto cr = create_cr ("conclusion"))
{
- cr.build_id = "merge-commit";
- cr.name = cr.build_id;
- }
- cr.state_synced = false;
+ l3 ([&]{trace << "created check_run { " << *cr << " }";});
- if (gq_create_check_run (error,
- cr,
- iat->token,
- sd.repository_node_id,
- sd.head_sha,
- // @@ TODO What details URL to use?
- "https://build2.org", // details URL.
- bs,
- move (br)))
- {
- l3 ([&]{trace << "created check_run { " << cr << " }";});
+ cni = move (cr->node_id);
}
}
- else if (ma)
- {
- // Update because the merge commit check run was previously stored and
- // the merge commit result has become available.
- //
- cr = move (*scr);
- cr.state_synced = false;
- if (gq_update_check_run (error,
- cr,
- iat->token,
- sd.repository_node_id,
- *cr.node_id,
- "https://build2.org", // details URL.
- bs,
- move (br)))
- {
- l3 ([&]{trace << "updated check_run { " << cr << " }";});
- }
- }
- else
+ if (cni || sd.conclusion_node_id)
{
- // Do nothing because nothing has changed.
+ // Update merge check run to successful.
//
- return nullptr;
- }
-
- // Create the conclusion check run and load the CI request.
- //
- optional<check_run> ccr; // Conclusion check run.
-
- if (ma && ma->mergeable)
- {
- // Create the conclusion check run if the merge commit shows the PR is
- // mergeable.
+ // @@ TMP If the CI load below failed on the previous call we probably
+ // don't want to update the CR again but we can't tell the difference
+ // (no state for the merge CR). However it should be a no-op due to
+ // the GitHub API's semantics so it's probably OK?
//
- // @@ TMP We could do this later if we prefer: if the branch protection
- // rule requires the conclusion check run then the PR will not go
- // green until the conclusion check run is successful.
- //
- ccr = check_run ();
- ccr->build_id = "conclusion";
- ccr->name = ccr->build_id;
- ccr->state_synced = false;
-
- if (gq_create_check_run (error,
- *ccr,
- iat->token,
- sd.repository_node_id,
- sd.head_sha,
- // @@ TODO What details URL to use?
- "https://build2.org", // details URL.
- build_state::queued))
+ if (auto cr = update_merge_cr (result_status::success,
+ "merge would succeed"))
{
- l3 ([&]{trace << "created check_run { " << *ccr << " }";});
- }
+ l3 ([&]{trace << "updated check_run { " << *cr << " }";});
- // Load the CI request.
- //
- repository_location rl (*sd.repository_clone_url + "#refs/pull/" +
+ // Load the CI request.
+ //
+ repository_location rl (*sd.repository_clone_url + "#refs/pull/" +
to_string (*sd.pr_number) + "/merge",
- repository_type::git);
+ repository_type::git);
- optional<start_result> r (
- load (error, warn, &trace, *build_db_, move (ts), rl));
+ optional<start_result> r (
+ load (error, warn, &trace, *build_db_, move (ts), rl));
- if (!r)
- {
- error << "unable to load CI request";
+ if (!r)
+ {
+ error << "unable to load CI request ";
- // @@ TODO Handle this. Will get called again and in those cases do
- // nothing except load the CI request.
+ // Try again next call.
+ }
}
+ // Don't load the CI request if the merge check run update failed so
+ // that we can try again on the next call.
}
- return
- [&error, iat = move (new_iat), cr = move (cr), ccr = move (ccr)] (
- const tenant_service& ts) -> optional<string>
+ return [&error,
+ iat = move (new_iat),
+ cni = move (cni)] (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
@@ -909,16 +905,8 @@ namespace brep
if (iat)
sd.installation_access = *iat;
- if (check_run* scr = sd.find_check_run (cr.build_id))
- *scr = cr;
- else
- sd.check_runs.push_back (cr);
-
- if (ccr)
- {
- assert (sd.find_check_run (ccr->build_id) == nullptr);
- sd.check_runs.push_back (*ccr);
- }
+ if (cni)
+ sd.conclusion_node_id = cni;
return sd.json ();
};