diff options
| -rw-r--r-- | mod/mod-ci-github-gh.cxx | 4 | ||||
| -rw-r--r-- | mod/mod-ci-github-gh.hxx | 6 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 63 |
3 files changed, 59 insertions, 14 deletions
diff --git a/mod/mod-ci-github-gh.cxx b/mod/mod-ci-github-gh.cxx index 6372ef0..6ab93d7 100644 --- a/mod/mod-ci-github-gh.cxx +++ b/mod/mod-ci-github-gh.cxx @@ -513,7 +513,7 @@ namespace brep { p.next_expect (event::begin_object); - bool ac (false), pr (false), rp (false), in (false); + bool ac (false), pr (false), bf (false), rp (false), in (false); // Skip unknown/uninteresting members. // @@ -526,6 +526,7 @@ namespace brep if (c (ac, "action")) action = p.next_expect_string (); else if (c (pr, "pull_request")) pull_request = gh_pull_request (p); + else if (c (bf, "before")) before = p.next_expect_string (); else if (c (rp, "repository")) repository = gh_repository (p); else if (c (in, "installation")) installation = gh_installation (p); else p.next_expect_value_skip (); @@ -542,6 +543,7 @@ namespace brep { os << "action: " << pr.action; os << ", pull_request { " << pr.pull_request << " }"; + os << ", before: " << (pr.before ? *pr.before : "null"); os << ", repository { " << pr.repository << " }"; os << ", installation { " << pr.installation << " }"; diff --git a/mod/mod-ci-github-gh.hxx b/mod/mod-ci-github-gh.hxx index b29904b..6ede697 100644 --- a/mod/mod-ci-github-gh.hxx +++ b/mod/mod-ci-github-gh.hxx @@ -183,6 +183,12 @@ namespace brep string action; gh_pull_request pull_request; + + // The SHA of the previous commit on the head branch before the current + // one. Only present if action is "synchronize". + // + optional<string> before; + gh_repository repository; gh_installation installation; diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index ba80ed6..119968d 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -406,8 +406,7 @@ namespace brep // synchronize // A pull request's head branch was updated from the base branch or // new commits were pushed to the head branch. (Note that there is - // no equivalent event for the base branch. That case gets handled - // in handle_check_suite_request() instead. @@ Not anymore.) + // no equivalent event for the base branch.) // // Note that both cases are handled the same: we start a new CI // request which will be reported on the new commit id. @@ -507,11 +506,13 @@ namespace brep // // While it would have been nice to cancel CIs of PRs with this branch as - // base not to waste resources, there are complications: Firsty, we can - // only do this for remote PRs (since local PRs may 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. + // 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. // @@ -1480,12 +1481,6 @@ namespace brep l3 ([&]{trace << "installation_access_token { " << *iat << " }";}); - // Note that similar to the branch push case above, while it would have - // been nice to cancel the previous CI job once the PR head moves (the - // "synchronize" event), due to the head sharing problem the previous CI - // job might actually still be relevant (in both local and remote PR - // cases). - // Distinguish between local and remote PRs by comparing the head and base // repositories' paths. // @@ -1494,6 +1489,48 @@ namespace brep ? service_data::local : service_data::remote); + // Note that similar to the branch push case above, while it would have + // been nice to cancel the previous CI job once the PR head moves (the + // "synchronize" event), due to the head sharing problem the previous CI + // job might actually still be relevant (in both local and remote PR + // cases). So we only do it for the remote PRs and only if the head is not + // shared (via tenant reference counting). + // + if (kind == service_data::remote && pr.action == "synchronize") + { + if (pr.before) + { + // Service id that will uniquely identify the CI tenant. + // + string sid (pr.repository.node_id + ':' + *pr.before); + + if (optional<tenant_service> ts = cancel (error, warn, + verb_ ? &trace : nullptr, + *build_db_, retry_, + "ci-github", sid, + true /* ref_count */)) + { + l3 ([&]{trace << "pull request " << pr.pull_request.node_id + << ": attempted to cancel CI of previous head commit" + << " (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 << "pull request " << pr.pull_request.node_id + << ": failed to cancel CI of previous head commit " + << "with tenant_service id " << sid;}); + } + } + else + { + error << "pull request " << pr.pull_request.node_id + << ": before commit is missing in synchronize event"; + } + } + // Note: for remote PRs the check_sha will be set later, in // build_unloaded_pre_check(), to test merge commit id. // |