diff options
| -rw-r--r-- | INSTALL-GITHUB-DEV | 13 | ||||
| -rw-r--r-- | mod/mod-ci-github-gh.cxx | 11 | ||||
| -rw-r--r-- | mod/mod-ci-github-gh.hxx | 19 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 193 | ||||
| -rw-r--r-- | mod/mod-ci-github.hxx | 14 |
5 files changed, 183 insertions, 67 deletions
diff --git a/INSTALL-GITHUB-DEV b/INSTALL-GITHUB-DEV index 716ed28..dfcbfe7 100644 --- a/INSTALL-GITHUB-DEV +++ b/INSTALL-GITHUB-DEV @@ -128,10 +128,15 @@ aspects but can't redeliver webhooks. - Branch push (BP). - - Success (also observe check run state transitions). - - Failure (also observe check run state transitions). + - Success (observe check runs state transitions). Test with 2 build configs. + - Failure (observe check runs state transitions). + - Push new commit to branch. - Re-requested check suite. - - Re-requested check run. + - Re-requested check run (observe check run state transitions). + - Re-requested check run but tenant archived. + - Cancel previous check suite on forced push. + - Head commit shared with another BP. + - Cancel previous check suite on forced push with shared previous commit. - Pull request (PR). @@ -139,6 +144,7 @@ aspects but can't redeliver webhooks. - Success. - Failure. + - Push new commit to head. - Re-requested check suite. - Re-requested check run. - Head shared with BP. @@ -148,6 +154,7 @@ aspects but can't redeliver webhooks. - Success. - Failure. + - Push new commit to head. - Re-requested check suite. - Re-requested check run. - Head shared with another remote PR. diff --git a/mod/mod-ci-github-gh.cxx b/mod/mod-ci-github-gh.cxx index 70155ad..2e886ac 100644 --- a/mod/mod-ci-github-gh.cxx +++ b/mod/mod-ci-github-gh.cxx @@ -663,7 +663,8 @@ namespace brep { p.next_expect (event::begin_object); - bool rf (false), bf (false), af (false), fd (false), rp (false); + bool rf (false), bf (false), af (false), fd (false), dl (false), + rp (false), in (false); // Skip unknown/uninteresting members. // @@ -678,7 +679,9 @@ namespace brep else if (c (bf, "before")) before = p.next_expect_string (); else if (c (af, "after")) after = p.next_expect_string (); else if (c (fd, "forced")) forced = p.next_expect_boolean<bool> (); + else if (c (dl, "deleted")) deleted = p.next_expect_boolean<bool> (); else if (c (rp, "repository")) repository = gh_repository (p); + else if (c (in, "installation")) installation = gh_installation (p); else p.next_expect_value_skip (); } @@ -686,7 +689,9 @@ namespace brep if (!bf) missing_member (p, "gh_push_event", "before"); if (!af) missing_member (p, "gh_push_event", "after"); if (!fd) missing_member (p, "gh_push_event", "forced"); + if (!dl) missing_member (p, "gh_push_event", "deleted"); if (!rp) missing_member (p, "gh_push_event", "repository"); + if (!in) missing_member (p, "gh_push_event", "installation"); } ostream& @@ -696,7 +701,9 @@ namespace brep << ", before: " << p.before << ", after: " << p.after << ", forced: " << p.forced - << ", repository { " << p.repository << " }"; + << ", deleted: " << p.deleted + << ", repository { " << p.repository << " }" + << ", installation { " << p.installation << " }"; return os; } diff --git a/mod/mod-ci-github-gh.hxx b/mod/mod-ci-github-gh.hxx index 24e9cda..91f5bfe 100644 --- a/mod/mod-ci-github-gh.hxx +++ b/mod/mod-ci-github-gh.hxx @@ -226,6 +226,10 @@ namespace brep // The SHA of the most recent commit on ref before the push. // + // The GitHub API reference says this member is always present and + // non-null. Testing shows that an absent before commit is represented by + // a value of "0000000000000000000000000000000000000000". + // string before; // The SHA of the most recent commit on ref after the push. @@ -237,7 +241,22 @@ namespace brep // bool forced; + // True if this was a branch deletion. + // + bool deleted; + gh_repository repository; + gh_installation installation; + + // Note: not received from GitHub but set from the app-id webhook query + // parameter instead. + // + // For some reason, unlike the check_suite and check_run webhooks, the + // push webhook does not contain the app id. For the sake of simplicity we + // emulate check_suite and check_run by storing the app-id webhook query + // parameter here. + // + string app_id; explicit gh_push_event (json::parser&); diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index 0f9a926..8f796af 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -277,7 +277,7 @@ namespace brep // Note: "GitHub continues to add new event types and new actions to // existing event types." As a result we ignore known actions that we are // not interested in and log and ignore unknown actions. The thinking here - // is that we want be "notified" of new actions at which point we can + // is that we want to be "notified" of new actions at which point we can // decide whether to ignore them or to handle. // if (event == "check_suite") @@ -307,14 +307,17 @@ namespace brep if (cs.action == "requested") { - return handle_check_suite_request (move (cs), warning_success); + // Branch pushes are handled in handle_push_request() so ignore this + // event. + // + return true; } else if (cs.action == "rerequested") { // Someone manually requested to re-run all the check runs in this // check suite. Treat as a new request. // - return handle_check_suite_request (move (cs), warning_success); + return handle_check_suite_rerequest (move (cs), warning_success); } else if (cs.action == "completed") { @@ -404,7 +407,7 @@ namespace brep throw invalid_request (400, move (m)); } - // Store the app-id webhook query parameter in the gh_pull_request + // Store the app-id webhook query parameter in the gh_pull_request_event // object (see gh_pull_request for an explanation). // // When we receive the other webhooks we do check that the app ids in @@ -496,6 +499,9 @@ namespace brep } else if (event == "push") { + // Push events are triggered by branch pushes, branch creation, and + // branch deletion. + // gh_push_event ps; try { @@ -513,7 +519,17 @@ namespace brep throw invalid_request (400, move (m)); } - return handle_push_request (move (ps)); + // Store the app-id webhook query parameter in the gh_push_event + // object (see gh_push_event for an explanation). + // + // When we receive the other webhooks we do check that the app ids in + // the payload and query match but here we have to assume it is valid. + // + ps.app_id = app_id; + + // Note that the push request event has no action. + // + return handle_push_request (move (ps), warning_success); } else { @@ -570,12 +586,14 @@ namespace brep } bool ci_github:: - handle_check_suite_request (gh_check_suite_event cs, bool warning_success) + handle_check_suite_rerequest (gh_check_suite_event cs, bool warning_success) { HANDLER_DIAG; l3 ([&]{trace << "check_suite event { " << cs << " }";}); + assert (cs.action == "rerequested"); + // While we don't need the installation access token in this request, // let's obtain it to flush out any permission issues early. Also, it is // valid for an hour so we will most likely make use of it. @@ -593,15 +611,6 @@ namespace brep l3 ([&]{trace << "installation_access_token { " << *iat << " }";}); - // While it would have been nice to cancel CIs of PRs with this branch as - // base not to waste resources, there are complications: Firstly, we can - // only do this for remote PRs (since local PRs will most likely share the - // result with branch push). Secondly, we try to do our best even if the - // branch protection rule for head behind is not enabled. In this case, it - // would be good to complete the CI. So maybe/later. See also the head - // case in handle_pull_request(), where we do cancel remote PRs that are - // not shared. - // Service id that uniquely identifies the CI tenant. // string sid (cs.repository.node_id + ':' + cs.check_suite.head_sha); @@ -625,9 +634,10 @@ namespace brep string check_sha; service_data::kind_type kind; - bool re_requested (cs.action == "rerequested"); - if (re_requested && !cs.check_suite.head_branch) + if (!cs.check_suite.head_branch) { + // Rebuild of remote PR. + // kind = service_data::remote; if (optional<tenant_data> d = find (*build_db_, "ci-github", sid)) @@ -655,7 +665,7 @@ namespace brep } else { - // Branch push or local PR rebuild. + // Rebuild of branch push or local PR. // kind = service_data::local; check_sha = cs.check_suite.head_sha; @@ -668,20 +678,15 @@ namespace brep cs.installation.id, move (cs.repository.node_id), move (cs.repository.clone_url), - kind, false /* pre_check */, re_requested, + kind, false /* pre_check */, true /* re_requested */, move (check_sha), move (cs.check_suite.head_sha) /* report_sha */); - // If this check suite is being re-run, replace the existing CI tenant if - // it exists; otherwise create a new one, doing nothing if a tenant - // already exists (which could've been created by handle_pull_request()). + // Replace the existing CI tenant if it exists. // // Note that GitHub UI does not allow re-running the entire check suite // until all the check runs are completed. // - duplicate_tenant_mode dtm (re_requested - ? duplicate_tenant_mode::replace - : duplicate_tenant_mode::ignore); // Create an unloaded CI tenant. // @@ -702,7 +707,7 @@ namespace brep tenant_service (sid, "ci-github", sd.json ()), chrono::seconds (30) /* interval */, chrono::seconds (0) /* delay */, - dtm)); + duplicate_tenant_mode::replace)); if (!pr) { @@ -710,8 +715,7 @@ namespace brep << ": unable to create unloaded CI tenant"; } - if (dtm == duplicate_tenant_mode::replace && - pr->second == duplicate_tenant_result::created) + if (pr->second == duplicate_tenant_result::created) { error << "check suite " << cs.check_suite.node_id << ": re-requested but tenant_service with id " << sid @@ -1431,7 +1435,7 @@ namespace brep : ""); // Note that PR rebuilds (re-requested) are handled by - // handle_check_suite_request(). + // handle_check_suite_rerequest(). // // Note that, in the case of a remote PR, GitHub will copy the PR head // commit from the head (forked) repository into the base repository. So @@ -1458,7 +1462,7 @@ namespace brep // Create with an empty service id so that the generated tenant id is used // instead during the pre-check phase (so as not to clash with a proper // service id for this head commit, potentially created in - // handle_check_suite() or as another PR). + // handle_branch_push() or as another PR). // tenant_service ts ("", "ci-github", sd.json ()); @@ -1487,45 +1491,119 @@ namespace brep } bool ci_github:: - handle_push_request (gh_push_event ps) + handle_push_request (gh_push_event ps, bool warning_success) { HANDLER_DIAG; l3 ([&]{trace << "push event { " << ps << " }";}); - // Do nothing if this is a fast-forwarding push. + // Cancel the CI tenant associated with the overwritten/deleted previous + // head commit if this is a forced push or a branch deletion. // - if (!ps.forced) + if (ps.forced || ps.deleted) { - l3 ([&]{trace << "ignoring fast-forward push " - << ps.after << " to " << ps.ref;}); - return true; + // The common log entry subject. @@ Move inline. + // + string sub (ps.forced ? "forced push " + ps.after + " to " + ps.ref + : "deletion of " + ps.ref); + + // Service id that will uniquely identify the CI tenant. + // + string sid (ps.repository.node_id + ':' + ps.before); + + // Note that it's possible this commit still exists in another branch so + // we do refcount-aware cancel. + // + if (optional<tenant_service> ts = cancel (error, warn, + verb_ ? &trace : nullptr, + *build_db_, retry_, + "ci-github", sid, + true /* ref_count */)) + { + l3 ([&]{trace << sub << ": attempted to cancel CI of previous" + << " head commit with tenant_service id " << sid + << " (ref_count: " << ts->ref_count << ')';}); + } + else + { + // It's possible that there was no CI for the previous commit for + // various reasons (e.g., CI was not enabled). + // + l3 ([&]{trace << sub << ": failed to cancel CI of previous head commit" + << " with tenant_service id " << sid;}); + } } - // Cancel the CI tenant associated with the overwritten previous head - // commit. + if (ps.deleted) + return true; // Do nothing further if this was a branch deletion. - // Service id that will uniquely identify the CI tenant. + // While we don't need the installation access token in this request, + // let's obtain it to flush out any permission issues early. Also, it is + // valid for an hour so we will most likely make use of it. // - string sid (ps.repository.node_id + ':' + ps.before); + optional<string> jwt (generate_jwt (ps.app_id, trace, error)); + if (!jwt) + throw server_error (); - if (optional<tenant_service> ts = cancel (error, warn, - verb_ ? &trace : nullptr, - *build_db_, retry_, - "ci-github", sid)) - { - l3 ([&]{trace << "forced push to " << ps.ref - << ": canceled CI of previous head commit" - << " with tenant_service id " << sid;}); - } - else + optional<gh_installation_access_token> iat ( + obtain_installation_access_token (ps.installation.id, + move (*jwt), + error)); + if (!iat) + throw server_error (); + + l3 ([&]{trace << "installation_access_token { " << *iat << " }";}); + + // While it would have been nice to cancel CIs of PRs with this branch as + // base not to waste resources, there are complications: Firstly, we can + // only do this for remote PRs (since local PRs will most likely share the + // result with branch push). Secondly, we try to do our best even if the + // branch protection rule for head behind is not enabled. In this case, it + // would be good to complete the CI. So maybe/later. See also the head + // case in handle_pull_request(), where we do cancel remote PRs that are + // not shared. + + // Service id that uniquely identifies the CI tenant. + // + string sid (ps.repository.node_id + ':' + ps.after); + + service_data sd (warning_success, + iat->token, + iat->expires_at, + ps.app_id, + ps.installation.id, + move (ps.repository.node_id), + move (ps.repository.clone_url), + service_data::local, + false /* pre_check */, + false /* re_requested */, + ps.after /* check_sha */, + ps.after /* report_sha */); + + // Create an unloaded CI tenant, doing nothing if one already exists + // (which could've been created by handle_pull_request() or by us as as + // result of a push to another branch). Note that the tenant's reference + // count is incremented in all cases. + // + // Note: use no delay since we need to (re)create the synthetic conclusion + // check run as soon as possible. + // + // Note that we use the create() API instead of start() since duplicate + // management is not available in start(). + // + // After this call we will start getting the build_unloaded() + // notifications until (1) we load the tenant, (2) we cancel it, or (3) + // it gets archived after some timeout. + // + if (!create (error, warn, verb_ ? &trace : nullptr, + *build_db_, retry_, + tenant_service (sid, "ci-github", sd.json ()), + chrono::seconds (30) /* interval */, + chrono::seconds (0) /* delay */, + duplicate_tenant_mode::ignore)) { - // It's possible that there was no CI for the previous commit for - // various reasons (e.g., CI was not enabled). - // - l3 ([&]{trace << "forced push to " << ps.ref - << ": failed to cancel CI of previous head commit" - << " with tenant_service id " << sid;}); + fail << "push " + ps.after + " to " + ps.ref + << ": unable to create unloaded CI tenant"; } return true; @@ -1644,7 +1722,8 @@ namespace brep // Create an unloaded CI tenant, doing nothing if one already exists // (which could've been created by a head branch push or another PR - // sharing the same head commit). + // sharing the same head commit). Note that the tenant's reference count + // is incremented in all cases. // // Note: use no delay since we need to (re)create the synthetic // conclusion check run as soon as possible. diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx index 8f7c10b..4f69f5c 100644 --- a/mod/mod-ci-github.hxx +++ b/mod/mod-ci-github.hxx @@ -82,13 +82,15 @@ namespace brep virtual void init (cli::scanner&); - // Handle the check_suite event `requested` and `rerequested` actions. + // @@ TODO Reorder handlers: push, pull_request, then check_suite. + + // Handle the check_suite event `rerequested` action. // // If warning_success is true, then map result_status::warning to SUCCESS // and to FAILURE otherwise. // bool - handle_check_suite_request (gh_check_suite_event, bool warning_success); + handle_check_suite_rerequest (gh_check_suite_event, bool warning_success); // Handle the check_suite event `completed` action. // @@ -114,11 +116,13 @@ namespace brep bool handle_pull_request (gh_pull_request_event, bool warning_success); - // Handle forced push events by canceling the overwritten previous head - // commit's CI request. + // Handle push events (branch push). + // + // If warning_success is true, then map result_status::warning to SUCCESS + // and to FAILURE otherwise. // bool - handle_push_request (gh_push_event); + handle_branch_push (gh_push_event, bool warning_success); // Build a check run details_url for a build. // |