Hi Ozone developers, I am proposing a change to the way CI checks are triggered in Ozone repos. Please let me know what you think.
-Attila ## Proposal Stop triggering workflows on `pull_request` events. - Make CI run in fork "mandatory". Reviewers should verify CI results from the fork for each PR, even if created by committer. This will be _easier_ than before, since each commit carries its own status, which is visible directly in the PR until hidden by the PR's own status. This means we will no longer have to navigate to the fork to find the CI run. - Check PR title via scheduled job, which should add a "review" with the results in the PR: -1 or 0. Allow committers to manually trigger the same check for a specific PR. ## Details I will use `master` as an example below, but the same applies to other long-lived (feature or release) branches. ### Background - Workflows are triggered by events. The most important ones for our purposes: `push`, `pull_request`, `schedule`, manual. - They run in the repo where the event occurs, e.g. `push` to your fork, `pull_request` created in the Apache repo. - Runners execute the workflow's jobs in parallel within limits, and wait in queues until runners are available. - Merging a PR also results in `push` to the target repo. - Ozone has "selective checks" for PRs, which tries to limit the set of checks to run based on what is being changed. - We completely skip CI for draft PRs. ### Apache vs. forks - ASF has a pool of few hundred runners shared among over 3.2k repos of 290+ Apache projects, with widely varying level of workflow usage. ASF runners are a rather limited resource. - Workflows in Apache repos are automatically triggered for authors who are committers on any Apache project. - External contributors' workflows in Apache repos need to be approved manually. - ASF has a policy on runner usage [1]. Per-project limits are not strongly enforced, we are trusted to limit our own usage, and rely on others to do the same. - Projects have no way to directly limit their usage to meet the policy, GitHub does not provide tools for that. - Free public repos (contributor forks) get 20 runners from GitHub, unrelated to the ASF. ### Why PR runs are not that useful Ozone CI is triggered for both `push` and `pull_request` events. The rationale was that the dev branch may not be fully up-to-date with `master`, but the `pull_request` takes those extra commits and runs checks on the combined state. However, it does so only as of the time the PR is opened or synchronized (new commit pushed). There may be further commits added to `master` between that time and when the PR is merged. Integration with these is not tested, so a passing PR CI run does not guarantee passing run on `master`. We do occasionally merge old PRs with outdated CI results only to realize it after the fact. More commits on `master` increase both the probability of problems and the time for PR CI to complete. If the dev branch is updated from `master` (usually due to conflicts), the `pull_request` run unnecessarily duplicates the `push` run's work. Thus, I don't think `pull_request` runs have much benefit in practice. ### What problems PR runs contribute to 1. They compete for limited resources, with both other projects and our own other workflows. ASF shared runners do not scale (with the increase in number of projects, contributors, more PRs per contributor, more checks, additional test cases, etc.) 2. Draft PR state is ambiguous: is work genuinely in progress, or is the PR draft just to defer CI? Draft PRs are more likely to get ignored. 3. Contributor workflow approval and "ready for review" events sometimes get mixed up, resulting in unexpected cancellations. 4. Committers from other Apache projects may be more trustworthy than external contributors, but their changes may still need more iterations, since they may be unfamiliar with Ozone. 5. On the flip side of (4), external contributors with more experience on Ozone but not yet committers still need manual approval. 6. Due to the need for workflow approval and the difference in event triggers, sometimes a "pass" result from the humble "pull request title" check is confused with full CI. ### How the proposal mitigates or fixes those problems 1. Reduces load on shared runners, since we eliminate a large portion of the runs. This frees up resources for other workflows, so we can merge PRs more frequently. It scales with the number of contributors. It is an incentive for contributors to pace their pull requests, for which reviewer capacity is also limited. It also helps other Apache projects. 2. No need to piggyback on draft state to skip PR CI. 3. No unexpected cancellation, since workflow does not need approval. 4-5. No distinction wrt. CI runs between committers from other Apache projects and external contributors. 6. PR title check result cannot be confused with full CI. ### Possible future improvements - We can add more information to the PR via scheduled job. Currently we have one to label PRs based on the paths being changed. It can be expanded to indicate the size of the change, how far the fork branch is behind `master`, etc. These would provide more information to reviewers at a glance in the PR list. - See if we can apply existing "selective checks" to reduce time needed for CI runs in fork. - Jarek in the Apache Airflow community is working [2] on sophisticated ways to inform PR status from fork CI status. When it is ready, we can explore if/how to adapt it. [1] https://infra.apache.org/github-actions-policy.html [2] https://github.com/apache/airflow/pull/73378 --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
