diff options
Diffstat (limited to 'mod')
| -rw-r--r-- | mod/mod-ci-github-gq.cxx | 19 | ||||
| -rw-r--r-- | mod/mod-ci-github-gq.hxx | 21 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.cxx | 6 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.hxx | 32 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 165 | ||||
| -rw-r--r-- | mod/mod-ci-github.hxx | 10 | ||||
| -rw-r--r-- | mod/tenant-service.hxx | 3 |
7 files changed, 203 insertions, 53 deletions
diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx index bcf9e55..1e9af74 100644 --- a/mod/mod-ci-github-gq.cxx +++ b/mod/mod-ci-github-gq.cxx @@ -265,6 +265,12 @@ namespace brep build_state rst (gh_from_status (rcr.status)); // Received state. + // Note that GitHub won't allow us to change a built check run to + // any other state (but all other transitions are allowed). + // + // @@ Are we handling the case where the resulting state (built) + // differs from what we expect? + // if (rst != build_state::built && rst != st) { error << "unexpected check_run status: received '" << rcr.status @@ -279,7 +285,7 @@ namespace brep if (!cr.node_id) cr.node_id = move (rcr.node_id); - cr.state = gh_from_status (rcr.status); + cr.state = rst; cr.state_synced = true; } } @@ -636,6 +642,8 @@ namespace brep ; // Still being generated; leave value absent. else { + // @@ Let's throw invalid_json. + // parse_error = "unexpected mergeable value '" + ma + '\''; // Carry on as if it were UNKNOWN. @@ -739,6 +747,15 @@ namespace brep // } // } // + // + // pullRequests (last:50 states:[OPEN] + // orderBy:{field:UPDATED_AT direction:ASC} + // baseRefName:"master") { + // + // updatedAt field changes on PR close, conversion to draft PR, merge + // conflict resolution, etc. So seems like it changes with any kind of PR + // update. + // static string gq_query_fetch_open_pull_requests (const string& rid, const string& br) { diff --git a/mod/mod-ci-github-gq.hxx b/mod/mod-ci-github-gq.hxx index 439f7b7..ad9797a 100644 --- a/mod/mod-ci-github-gq.hxx +++ b/mod/mod-ci-github-gq.hxx @@ -83,24 +83,6 @@ namespace brep build_state, optional<gq_built_result> = nullopt); - // Fetch a pull request's mergeability from GitHub and return it in first, - // or absent if the merge commit is still being generated. - // - // Return false in second and issue diagnostics if the request failed. - // - struct gq_pr_mergeability - { - // True if the pull request is auto-mergeable; false if it would create - // conflicts. - // - bool mergeable; - - // The ID of the test merge commit. Empty if mergeable is false. - // - string merge_commit_id; - }; - - // Fetch a pull request's mergeability from GitHub. Return absent value if // the merge commit is still being generated. Return empty string if the // pull request is not auto-mergeable. Otherwise return the test merge @@ -110,7 +92,8 @@ namespace brep // will be treated by the caller as still being generated). // // Note that the first request causes GitHub to start preparing the test - // merge commit. + // merge commit. (For details see + // https://docs.github.com/rest/guides/getting-started-with-the-git-database-api#checking-mergeability-of-pull-requests.) // optional<string> gq_pull_request_mergeable (const basic_mark& error, diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx index e2ed904..0f6e593 100644 --- a/mod/mod-ci-github-service-data.cxx +++ b/mod/mod-ci-github-service-data.cxx @@ -74,7 +74,10 @@ namespace brep { string* v (p.next_expect_member_string_null ("status")); if (v != nullptr) + { rs = bbot::to_result_status (*v); + assert (s == build_state::built); + } } check_runs.emplace_back (move (bid), move (nm), move (nid), s, ss, rs); @@ -185,7 +188,10 @@ namespace brep s.member_name ("status"); if (cr.status) + { + assert (cr.state == build_state::built); s.value (to_string (*cr.status)); + } else s.value (nullptr); diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx index 5573d90..89fcaab 100644 --- a/mod/mod-ci-github-service-data.hxx +++ b/mod/mod-ci-github-service-data.hxx @@ -43,12 +43,33 @@ namespace brep } }; + // We have two kinds of service data that correspond to the following two + // scenarios (those are the only possible ones, until/unless we add support + // for merge queues): + // + // 1. Branch push (via check_suite) plus zero or more local PRs (via + // pull_request) that share the same head commit id. + // + // 2. One or more remote PRs (via pull_request) that share the same head + // commit id (from a repository in another organization). + // + // Plus, for PRs, the service data may be in the pre-check phase while we + // are in the process of requesting the test merge commit and making sure it + // can be created and is not behind base. We do all this before we actually + // create the CI tenant. + // struct service_data { // The data schema version. Note: must be first member in the object. // uint64_t version = 1; + // Kind and phase. + // + enum {local, remote /*, queue */} kind; + bool pre_check; + bool re_request; // Re-requested (rebuild). + // Check suite settings. // bool warning_success; // See gh_to_conclusion(). @@ -73,9 +94,16 @@ namespace brep // optional<string> merge_node_id; - // The commit ID the check suite or pull request (and its check runs) are + // The commit ID the branch push or pull request (and its check runs) are + // building. This will be the head commit for the branch push as well as + // local pull requests and the test merge commit for remote pull requests. + // + string check_sha; + + // The commit ID the branch push or pull request (and its check runs) are // reporting to. Note that in the case of a pull request this will be the - // head commit (`pull_request.head.sha`) as opposed to the merge commit. + // head commit (`pull_request.head.sha`) as opposed to the test merge + // commit. // string report_sha; diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index 0ddc75d..ce191ca 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -29,6 +29,8 @@ // - check_run (action: rerequested): received when user re-runs a // specific check or all failed checks. // +// @@ TMP I have confirmed that the above is accurate. +// // Will need to extract a few more fields from check_runs, but the layout // is very similar to that of check_suite. // @@ -275,8 +277,8 @@ 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 decide - // whether to ignore them or to handle. + // is that we want be "notified" of new actions at which point we can + // decide whether to ignore them or to handle. // // @@ There is also check_run even (re-requested by user, either // individual check run or all the failed check runs). @@ -360,7 +362,7 @@ namespace brep // 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.) + // in handle_check_suite_request() instead. @@ Not anymore.) // // Note that both cases are handled the same: we start a new CI // request which will be reported on the new commit id. @@ -372,6 +374,8 @@ namespace brep // Ignore the remaining actions by sending a 200 response with empty // body. // + // @@ Ignore known but log unknown, as in check_suite above? + // return true; } } @@ -487,6 +491,8 @@ namespace brep } } + // @@ Not anymore (and may not need separate create_pull_request_ci()). + // // 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 (and, no, GitHub does not invalidate those CI @@ -526,7 +532,7 @@ namespace brep { // Recreate each PR's CI request. // - for (gh_pull_request& pr: *prs) + for (const gh_pull_request& pr: *prs) { service_data prsd (sd.warning_success, sd.installation_access.token, @@ -567,6 +573,8 @@ namespace brep error << "check suite " << nid << " (re-requested): unable to cancel"; } + // @@@ Use repo+head ad service id. + // Start CI for the check suite. // repository_location rl (cs.repository.clone_url + '#' + @@ -613,6 +621,8 @@ namespace brep build_state::built, move (br))) { + assert (cr.state == build_state::built); + l3 ([&]{trace << "updated check_run { " << cr << " }";}); } else @@ -673,34 +683,66 @@ namespace brep // from within the same repository or from a forked repository. The merge // and head commits will be at refs/pull/<PR-number>/{merge,head}. // - // - New commits are added to PR head branch + // - @@ TODO Shared repo model problems + // + // The root of all of these problems is that, in the shared repository + // model, for every PR we also receive a check_suite for the commit to + // the head branch. The situation is exacerbated by the fact that the + // PR and CS can arrive in any order. + // + // - There will be two CIs running concurrently, building different + // commits: the head commit (check_suite) vs merge commit + // (pull_request). + // + // - The CS and PR check_runs will all be added to the same commit + // SHA: pull_request.head.sha, check_suite.head_sha (see above for + // the reasons we don't put the check_runs on the merge + // commit). + // + // - Recall that creating a check_run named `A` will effectively + // replace any existing check_runs with that name. They will still + // exist on the GitHub servers but GitHub will only consider the + // latest one (for display in the UI or in determining the + // mergeability of a PR). + // + // - Some CS CRs may finish after the corresponding PR CRs, thus + // potentially inverting the true status of a PR (e.g., allow the + // merge of a PR with a bad merge commit). + // + // Thus we need a way to prevent any CS check_runs from being updated + // after having received a PR. // - // @@ TODO In this case we will end up creating two sets of check runs on - // the same commit (pull_request.head.sha and - // check_suite.head_sha). It's not obvious which to prefer but I'm - // thinking the pull request is more important because in most - // development models it represents something that is more likely to - // end up in an important branch (as opposed to the head of a feature - // branch). + // Problem 1: Create PR from feature branch // - // Note that in these two cases we are building different commit (the - // head commit vs merge commit). So it's not clear how we can have - // a single check_suite result represent both? + // - Receive check_suite for commit to feature branch + // - Receive pull_request // - // Possible solution: ignore all check_suites with non-empty - // pull_requests[]. + // Solution: When receive a PR, fetch all check_suites with that head + // SHA (curl REPO/commits/SHA/check-suites) and cancel their CI + // jobs. // - // => check_suite(requested, PR_head) [only in shared repo model] + // Thus there will be no more CS check_run updates. Note however that in + // most cases the PR will be received long enough after the CS for the + // latter's check_runs to all have completed already. // - // Note: check_suite.pull_requests[] will contain all PRs with this - // branch as head. + // Note that there will not be a merge CR on the head yet so the PR will + // never be green. // - // Note: check_suite.pull_requests[i].head.sha will be the new, updated - // PR head sha. + // Problem 2: New commits are added to PR head branch // - // => pull_request(synchronize) + // Note: The check_suite and pull_request can arrive in any order. // - // Note: The check_suite and pull_request can arrive in any order. + // - check_suite(requested, PR_head) + // + // Note: check_suite.pull_requests[] will contain all PRs with this + // branch as head. + // + // Note: check_suite.pull_requests[i].head.sha will be the new, + // updated PR head sha. + // + // - pull_request(synchronize) + // + // Solution: Ignore all check_suites with non-empty pull_requests[]. // // - New commits are added to PR base branch // @@ -713,6 +755,9 @@ namespace brep // // - PR closed @@ TODO // + // Also received if base branch is deleted. (And presumably same for head + // branch.) + // // => pull_request(closed) // // Cancel CI? @@ -779,8 +824,6 @@ namespace brep return true; } - // Note: only handles pull requests (not check suites). - // function<optional<string> (const tenant_service&)> ci_github:: build_unloaded (tenant_service&& ts, const diag_epilogue& log_writer) const noexcept @@ -798,6 +841,42 @@ namespace brep return nullptr; } + return sd.pre_check + ? build_unloaded_pre_check (move (ts), move (sd), log_writer) + : build_unloaded_load (move (ts), move (sd), log_writer); + } + + function<optional<string> (const tenant_service&)> ci_github:: + build_unloaded_pre_check ( + tenant_service&&, + service_data&&, + const diag_epilogue& log_writer) const noexcept + { + NOTIFICATION_DIAG (log_writer); + + // Note: PR only (but both local and remove). + // + // - Ask for test merge commit. + // - If not ready, get called again. + // - If not mergeable, behind, of different head, cancel itself and ignore. + // - Otherwise, create unloaded CI tenant (with proper duplicate mode + // based on re_request) and cancel itself. + + return nullptr; + } + + function<optional<string> (const tenant_service&)> ci_github:: + build_unloaded_load ( + tenant_service&& ts, + service_data&& sd, + const diag_epilogue& log_writer) const noexcept + { + NOTIFICATION_DIAG (log_writer); + + // @@@ TODO: load the tenant: should be the same for both branch push and + // PR. + // + // Get a new installation access token if the current one has expired. // const gh_installation_access_token* iat (nullptr); @@ -907,6 +986,8 @@ namespace brep build_state::built, move (br))) { + assert (cr.state == build_state::built); + return cr; } else @@ -1579,7 +1660,10 @@ namespace brep if (cr.state == build_state::built) { if (conclusion) + { + assert (cr.status); *conclusion |= *cr.status; + } } else conclusion = nullopt; @@ -1602,8 +1686,6 @@ namespace brep if (scr->state == build_state::built) return nullptr; - // Don't move from scr because we search sd.check_runs below. - // cr = move (*scr); } else @@ -1788,7 +1870,7 @@ namespace brep } } - if (cr.state == build_state::built) + if (cr.state_synced) { // Check run was created/updated successfully to built. // @@ -1798,6 +1880,27 @@ namespace brep // result_status into a gq_ function because converting to a GitHub // conclusion/title/summary is reasonably complicated. // + // @@@ We need to redo that code: + // + // - Pass the vector of check runs with new state (and status) set. + // - Update synchronized flag inside those functions. + // - Update the state to built if it's already built on GitHub -- + // but then what do we set the status to? + // + // @@ TMP This scenario can only arise for updates to building. + // For creations a new CR will always be created so the + // returned state will always be what we asked for; and we + // never update to queued. + // + // As for updates to building, if GH has already been updated + // to built then the build_built() lambda will soon save the + // built state and valid status so nothing more should need to + // be done. In fact, whenever updating to building we do stop + // immediately if it's already built on GH. + // + // - Maybe signal in return value (optional<bool>?) that there is + // a discrepancy. + // cr.status = b.status; // Update the conclusion check run if all check runs are now built. @@ -1831,6 +1934,8 @@ namespace brep build_state::built, move (br))) { + assert (cr.state == build_state::built); + l3 ([&]{trace << "updated check_run { " << cr << " }";}); } else @@ -1918,7 +2023,7 @@ namespace brep *build_db_, move (ts), chrono::seconds (30) /* interval */, chrono::seconds (0) /* delay */) - .has_value (); + .first.has_value (); // @@ TODO HACKED AROUND } string ci_github:: diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx index 489aac7..e919065 100644 --- a/mod/mod-ci-github.hxx +++ b/mod/mod-ci-github.hxx @@ -46,6 +46,16 @@ namespace brep build_unloaded (tenant_service&&, const diag_epilogue& log_writer) const noexcept override; + function<optional<string> (const tenant_service&)> + build_unloaded_pre_check (tenant_service&&, + service_data&&, + const diag_epilogue&) const noexcept; + + function<optional<string> (const tenant_service&)> + build_unloaded_load (tenant_service&&, + service_data&&, + const diag_epilogue&) const noexcept; + virtual function<optional<string> (const tenant_service&)> build_queued (const tenant_service&, const vector<build>&, diff --git a/mod/tenant-service.hxx b/mod/tenant-service.hxx index c46cb7b..6a982ea 100644 --- a/mod/tenant-service.hxx +++ b/mod/tenant-service.hxx @@ -127,7 +127,8 @@ namespace brep // This notification is only made on unloaded CI requests created with the // ci_start::create() call and until they are loaded with ci_start::load() - // or, alternatively, abandoned with ci_start::abandon(). + // or, alternatively, abandoned with ci_start::cancel() (in which case the + // returned callback should be NULL). // // Note: make sure the implementation of this notification does not take // too long (currently 40 seconds) to avoid nested notifications. Note |