diff options
| -rw-r--r-- | mod/mod-ci-github.cxx | 138 |
1 files changed, 65 insertions, 73 deletions
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index 5401bc7..96cd536 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -552,109 +552,101 @@ namespace brep return true; } - // High-level description of PR handling: + // High-level description of pull request (PR) handling // - // - CI the merge commit but add checks to PR head ref + // - Some GitHub pull request terminology: // - // Recommended by the following blog posts by a GitHub employee who is one - // of the best sources on these topics: - // https://www.kenmuse.com/blog/shared-commits-and-github-checks/ and - // https://www.kenmuse.com/blog/creating-github-checks/. - // - // DO NOT ADD CHECKS TO THE MERGE COMMIT because: - // - // - The PR head ref is the only commit that can be relied upon to exist - // throughout the PR's lifetime. The merge commit, on the other hand, - // can get replaced during the PR process. When that happens the PR will - // look for checks on the new merge commit, effectively discarding the - // ones we had before. - // - // - Although some of the GitHub documentation makes it sound like they - // expect checks to be added to both the PR head and the merge commit, - // the PR UI does not react to the merge commit's checks - // consistently. It actually seems to be quite broken. - // - // The only thing it seems to do reliably is blocking the PR merge if - // the merge commit is not successful (overriding the PR head ref's - // checks). But the UI looks quite messed up generally in this state. + // - Fork and pull model: Pull requests are created in a forked + // repository. Thus the head and base repositories are different. // - // - PR Creation + // - Shared repository model: The pull request head and base branches are + // in the same repository. For example, from a feature branch onto + // master. // - // False green window problem - // - User creates new commit on feature branch - // - We receive check_suite; check runs start, run, succeed - // - User creates PR from feature branch to master - // - Because feature branch head has successful CRs, PR is green to merge - // - But merge commit has not been CI'd yet - // - False green until first merge commit CR is queued + // - CI the merge commit but add check runs to the pull request head commit // - // check_suite(requested, PR_head): [shared repo model] + // Most of the major CI integrations build the merge commit instead of the + // PR head commit. // - // - Starts CI with check runs on what will become the PR head ref + // Adding the check runs to the PR head commit is recommended by the + // following blog posts by a GitHub employee who is one of the best + // sources on these topics: + // https://www.kenmuse.com/blog/shared-commits-and-github-checks/ and + // https://www.kenmuse.com/blog/creating-github-checks/. // - // pull_request(opened) + // Do not add any check runs to the merge commit because: // - // - Fetch pull request to trigger start of merge commit job; ignore - // response + // - The PR head commit is the only commit that can be relied upon to + // exist throughout the PR's lifetime. The merge commit, on the other + // hand, can change during the PR process. When that happens the PR will + // look for check runs on the new merge commit, effectively discarding + // the ones we had before. // - // - To close false green window - // - if pull_request.head.repo.node_id == pull_request.base.repo.node_id - // - Replace all (or just one?) of the CRs on the PR head ref with new, - // queued ones - // - Fetch all check runs on PR head SHA - // - Find CRs with this PR in pull_requests[] - // - Create new CRs with same names as existing, thus effectively - // destroying the old ones + // - Although some of the GitHub documentation makes it sound like they + // expect check runs to be added to both the PR head commit and the + // merge commit, the PR UI does not react to the merge commit's check + // runs consistently. It actually seems to be quite broken. // - // - Create unloaded tenant + // The only thing it seems to do reliably is blocking the PR merge if + // the merge commit's check runs are not successful (i.e, overriding the + // PR head commit's check runs). But the UI looks quite messed up + // generally in this state. // - // - On callback - // Fetch pull request; if mergeable, start CI proper + // Note that, in the case of a PR from a forked repository (the so-called + // "fork and pull" model), GitHub will copy the PR head commit from the + // head repository (the forked one) into the base repository. So the check + // runs must always be added to the base repository, whether the PR is + // 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 // - // False green window + // @@ 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). // - // PR already exists so merge will be red initially after CS is - // received, but those CRs could theoretically all succeed before our CI - // of the merge commit starts. Either way the CS CRs are an inaccurate - // representation of what's being CI'd so need to be avoided if - // possible. + // Possible solution: ignore all check_suites with non-empty + // pull_requests[]. // - // check_suite(requested, PR_head) [shared repo model] + // => check_suite(requested, PR_head) [only in shared repo model] // - // Ignore if pull_requests[] is non-empty (which should be the case if - // the head branch is in the same repository). + // Note: check_suite.pull_requests[] will contain all PRs with this + // branch as head. // - // pull_request(synchronize) + // Note: check_suite.pull_requests[i].head.sha will be the new, updated + // PR head sha. // - // - Fetch pull request to trigger start of merge commit job; ignore - // response + // => pull_request(synchronize) // - // - Create unloaded tenant - // - // - On callback - // Fetch pull request; if mergeable, start CI proper + // Note: The check_suite and pull_request can arrive in any order. // // - New commits are added to PR base branch // - // check_suite(requested, PR_base) + // Note: In this case pull_request.base.sha does not change, but the merge + // commit will be updated to include the new commits to the base branch. + // + // - @@ TODO? PR base branch changed (to a different branch) + // + // => pull_request(edited) // - // Fetch all open PRs for base branch (triggering the merge commit jobs) + // - PR closed @@ TODO // - // Replace CRs on PR head refs to close false green window. + // => pull_request(closed) // - // Submit new unloaded tenants for all of the PRs. + // Cancel CI? // - // NOTE: pull_request.base.sha does not change. But merge commit (once - // updated) does try to merge PR head to new tip of PR base. + // - PR merged @@ TODO // - // - PR closed/merged/etc. @@ TODO + // => pull_request(merged) // - // pull_request(closed/merged/...) + // => check_suite(PR_base) // - // Note: when a PR is merged we will get a pull_request(closed) and then a - // check_suite for the base branch. + // Probably wouldn't want to CI the base again because the PR CI would've + // done the equivalent already. // bool ci_github:: handle_pull_request (gh_pull_request_event pr, bool warning_success) |