ci: add a Required Checks aggregator job so main can have a required status check - #5842
Conversation
…mpute-changes.py
Every heavy job in `ci.yml` carried a four-line `${{ }}` expression combining
a path-filter output, an event-name test, an opt-in label test, and a special
case for `labeled` events. Ten jobs, ten near-identical copies, none of them
testable outside a real workflow run.
Fold that policy into `dev/ci/compute-changes.py` next to the path filters it
was already being ANDed with. Each job's gate becomes:
if: needs.changes.outputs.spark_3_5 == 'true'
and the routing lives in one readable table:
POLICY = {
"spark_3_5": ["pr", "push"],
"spark_3_4": ["push", "label:run-spark-3.4-tests"],
...
}
`ci.yml` loses 62 lines net. No behaviour changes: the `changes` job now
receives the event name, action, added label and PR labels, and applies the
same rules the expressions did.
The point of moving it is that it can now be tested. `check-ci-config.py`
grows `POLICY_CASES`, which pins the expected job set for each event shape --
including that a non-gating label such as dependabot's `dependencies` starts
nothing, which is issue apache#5007 and was previously only enforceable by reading
YAML carefully.
Writing `"pr"` alongside a `"label:"` tier reads as "runs on every PR, and also
when labelled" but the label check wins and the "pr" is dead, so
`check-ci-config.py` now rejects that combination rather than letting it be a
silent no-op.
Verified equivalent with a differential harness: the pre-refactor `if:`
expressions were extracted from ci.yml at the merge base, translated to Python,
and evaluated against the new POLICY over 1464 (job, event) combinations --
every event name, pull_request action, label set and added-label pairing. Zero
mismatches, and three seeded regressions (renaming a gating label, dropping a
label gate, dropping the labeled-event narrowing) are each caught.
Groundwork for apache#5838: adding the merge queue then means adding one tier to
POLICY rather than editing ten YAML expressions.
…status check
`main` currently requires no status checks, only one approving review. Adding
one is blocked by how the umbrella workflow reports: every heavy job in
`ci.yml` is a thin caller of a reusable workflow, and the check name a caller
publishes depends on whether it ran.
skipped by `if:` one check run named exactly `PR Build (Linux)`
actually ran only `PR Build (Linux) / Spark 4.1, JDK 17 [exec]` and
friends, and no bare `PR Build (Linux)` at all
Confirmed against bdd2aeb (md-only, everything skipped) and 36caf8e (heavy
jobs ran). No name is reported in both cases, so requiring the bare name would
block every code change and requiring a nested name would block every docs-only
change. Both hang waiting for a check that never arrives rather than failing,
and a required context that never reports also blocks the merge that would fix
`.asf.yaml` -- only INFRA can remove a required check by hand at that point.
Add `required_checks`, a flat job that `needs:` every other job and reports on
every event. It runs `if: always()` and treats `skipped` as a pass, so it goes
red only when an upstream job reports `failure` or `cancelled`.
Nothing requires it yet. This lands on its own so the check can be observed on
real pull requests before `.asf.yaml` names it, which is the step that is
expensive to get wrong.
`dev/ci/check-ci-config.py` gains two invariants: every `ci.yml` job except
`docs` must appear in `required_checks.needs`, and once `.asf.yaml` does
declare a required context, the job's `name:` must match it.
Part of apache#5838.
1c9c974 to
1cbcdf3
Compare
…up-3c01985b # Conflicts: # dev/ci/check-ci-config.py
The aggregator reads only the `needs` context, so it has no use for the
GITHUB_TOKEN. Declaring `permissions: {}` on the job drops the token
entirely and clears the CodeQL "Workflow does not contain permissions"
finding, without touching the token scope any other job in ci.yml gets.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BtAqq4YJsk8uk42c7vHk8B
Widen the job-id regexes to accept `-` and uppercase, which GitHub allows. A `spark-4-2:` job missing from `required_checks.needs` passed the coverage check silently before this change. Scope the `.asf.yaml` parse to the `main:` entry under `protected_branches`. It previously collected every `contexts:` list in the file, so a release branch requiring its own context produced a false failure that blamed main. Drop the em dashes from the new README prose. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtAqq4YJsk8uk42c7vHk8B
Once Required Checks is a required context, any red job evicts the PR from the merge queue. The first two runs on this branch each failed in a step that runs after the test verdict is known or before any test has started, and in neither case was a test involved. - java-test: mark the test-report upload continue-on-error. It runs on green jobs, nothing downstream consumes the reports, and a FinalizeArtifact 403 must not turn a passing run red. The failure-only uploads are unchanged. - iceberg_spark_test_reusable: route the shard-inventory upload through upload-artifact-retry. iceberg-spark-shard-coverage consumes it, so it already fell under the README rule for artifacts a later job downloads. - pr_build_linux: split Lint Scala (syntactic) into a retried no-op `cs launch scalafix -- --version` that populates the cache, then the real check under `--mode offline`, so a nonzero exit there is only ever a lint violation. - ci.yml preflight: retry the actionlint download and fetch the installer to a file instead of piping it into bash. Closes apache#5860 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtAqq4YJsk8uk42c7vHk8B
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Reviewed head e169d3968424aa418c4580e79211749a541492de against base 8320ae481b4eb916beebc9ed611473abb79c3e71. This addresses the unstable check names produced by reusable-workflow callers by adding a flat Required Checks job. The current .asf.yaml remains unchanged, so requiring the context and enabling the merge queue are deliberately separate steps.
Within one run, the dependency list covers all 13 other jobs except the documented site deployment. Including both preflight and changes prevents a failed prerequisite from disappearing behind skipped descendants. always() lets the aggregator inspect completed dependencies, and rejecting failure and cancelled is consistent with the documented dependency result states. The inner failure step's default success condition checks previous steps in this job. It does not undo the outer job's always(). GitHub dependency and expression semantics, status functions.
One P2 remains in the inline comment: label-triggered runs intentionally omit the normal PR suites but publish the same green aggregate name. A verdict based only on that run's needs does not establish that the commit's applicable tests passed. This needs to be addressed before this name can serve its intended role as the single required context. The needs context is limited to direct dependencies in the current run.
The new permissions: {} addresses the existing resolved CodeQL comment. The job needs no checkout, secrets, or token API access, including on fork PRs. Making diagnostic test-report upload nonfatal leaves Maven test failures fatal. The consumed Iceberg shard inventory retains fatal failure after its bounded retries. This PR changes no Spark expressions or operators, so it changes no Spark null, type, ANSI, error, or fallback semantics.
Validation: the current config checker passes locally. Eight in-memory guard cases cover dependency omission, stale names, a new hyphenated job, required-context matching and renames, and independent release-branch contexts. I also checked event coverage and all four documented status values for each dependency. These are local configuration checks, not a GitHub scheduler or branch-protection test. Preflight and syntactic Scala lint passed on merge bf8e498ae53f385f86cfd05cbb2ad384812f54b6, whose parents match this exact base/head and whose tree matches the head. The remaining CI is still running. No local native/JVM suites or live merge experiment ran.
Performance
The aggregator adds a short runner job after the selected suites, with no additional builds or API polling. Download warm-up retries are bounded and leave the actual lint invocation outside the retry loop. This limits repeated work without retrying test or lint failures. Current CI confirms the offline scalafix invocation succeeds, but no end-to-end time saving is claimed. There is no query-runtime change requiring a Spark microbenchmark.
Design
A flat status and a staged rollout before branch protection are appropriate for the reusable-workflow naming problem. The remaining design requirement is that every producer of that status represent complete applicable coverage, including repeated events on the same commit. The existing event policy was designed to avoid duplicate heavy work and now needs to be reconciled with the new aggregate verdict. The site deployment exemption is explicit and consistent with its post-merge purpose. Merge-group triggering belongs to the planned queue-enablement change and is not claimed to work here.
Abstraction & complexity
The result reducer is small and uses existing Actions dependency data. The config checker keeps job membership and the declared required name aligned without adding a runner dependency. Its line parser matches the current repository layout, and focused mutation checks exercise the new rules. The retry changes reuse the established upload action and keep the one-off tool warm-ups local. No additional abstraction is needed to resolve the event-coverage finding.
| name: Required Checks | ||
| if: always() |
There was a problem hiding this comment.
Correctness
[P2] Keep label-only runs from publishing an incomplete required verdict
Could every run publishing this shared name validate the applicable PR-tier checks? ci.yml also runs on labeled, while compute-changes.py::event_allows deliberately excludes the ordinary PR tier for that event. Those jobs are therefore skipped even when the changed files require them, and this aggregator reports success after just preflight and change detection, or after the one opt-in suite. Its needs cannot observe pending or failed jobs in the separate commit run. A green label-run result is therefore not evidence that the commit's applicable suites passed. The shared-name reporting problem is already described in #5007. Before this becomes the single required context, please ensure each producer validates the complete applicable test set and add a label-event coverage regression. Simply skipping this job on label events would still emit a passing skipped check under that name.
There was a problem hiding this comment.
Confirmed. The label run's needs only sees its own run, and GitHub keeps the most recent check run per name per commit, so a dependencies label landing a minute after a push would have published a green Required Checks on top of a still-running or red commit run. Same mechanism as #5007.
Fixed in 8074e01 by making the check name depend on the event:
name: ${{ github.event.action == 'labeled' && 'Required Checks (label run)' || 'Required Checks' }}Label runs now publish under a name nothing requires, so they can never overwrite the commit run's verdict, and they still give the committer an honest aggregate for the opt-in suite they asked for. Skipping the job on labeled was not an option for the reason you gave: a skipped check run still carries the name and counts as passing.
check-ci-config.py enforces the shape: the name must be that expression, the labeled branch must differ from the fallback, the fallback is what has to match the .asf.yaml context, and .asf.yaml may never require the label-run name. Mutation-tested each of those (literal name, identical branches, requiring the label-run name, renaming the fallback with a context declared) plus the earlier cases, all caught with a specific message.
Merging labeled back into the PR tier was the other option and I did not take it, because it re-creates the duplicate-pipeline cost the label policy exists to avoid.
ci.yml also fires on `labeled`, and on that event POLICY skips the PR tier because it already ran at the same commit. The label run's aggregate says nothing about the commit's applicable suites, but GitHub keeps only the most recent check run per name per commit, so publishing it as `Required Checks` would let a `dependencies` label that lands a minute after a push mark the commit green while the real run is still going (the mechanism from apache#5007). Make the aggregator's `name:` an expression that routes label runs to `Required Checks (label run)`. Skipping the job on `labeled` would not help: a skipped check run still carries the name and still counts as passing. check-ci-config.py now requires that expression shape, rejects a `labeled` branch equal to the required name, and rejects an `.asf.yaml` that requires the label-run name. The commit-run name is what has to match `.asf.yaml`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtAqq4YJsk8uk42c7vHk8B
Which issue does this PR close?
Part of #5838. Closes #5860.
Rationale for this change
mainrequires no status checks today, only one approving review. #5838 needs one, since a merge queue only ever waits on required checks. Before we can add one we need a name that is actually safe to require, and right now we don't have one.Every heavy job in
ci.ymlis a thin caller of a reusable workflow, and the check name a caller publishes depends on whether it ran:if:PR Build (Linux), conclusionskippedPR Build (Linux) / Spark 4.1, JDK 17 [exec]and friends, and no barePR Build (Linux)at allI confirmed both against real commits: bdd2aeb (md-only, everything skipped) and 36caf8e (heavy jobs ran). There's no name that reports in both cases, so requiring the bare name would block every code change, and requiring a nested name would block every docs-only change. Worse, both of those hang waiting for a check that never arrives rather than failing outright, and a required context that never reports also blocks the merge that would fix
.asf.yaml. At that point only INFRA can remove the check by hand. apache/datafusion ran into the same class of problem in apache/datafusion#17538 and had to revert it in apache/datafusion#17629.What changes are included in this PR?
A
required_checksjob at the bottom ofci.ymlthatneeds:every other job and publishes a single flatRequired Checkscontext on every event. It runsif: always()and treatsskippedas a pass, so it only goes red when an upstream job reportsfailureorcancelled.The job's
name:is an expression rather than a literal.ci.ymlalso fires onlabeled, and on that eventPOLICYdeliberately skips the PR tier because it already ran at the same commit. A label run's aggregate therefore says nothing about the commit's applicable suites, but GitHub keeps only the most recent check run per name per commit, so publishing it asRequired Checkswould let adependencieslabel landing a minute after a push mark the commit green while the real run was still going (the mechanism from #5007). Label runs publish asRequired Checks (label run)instead, a name nothing requires. Skipping the job onlabeledwould not help, since a skipped check run still carries the name and still counts as passing.Nothing requires it yet, and that's deliberate. It lands on its own so we can watch it report on real pull requests before
.asf.yamlnames it, since that's the step that's expensive to get wrong. The follow-up PR does that and turns the queue on.dev/ci/check-ci-config.pyalso gains two invariants, in the same spirit as the ones already in there: everyci.ymljob exceptdocshas to appear inrequired_checks.needs, and once.asf.yamldoes declare a required context formain, the job'sname:has to keep matching it. Both sides of that pair are silent when broken. The job-id regexes accept-and uppercase, which GitHub allows, and the.asf.yamlparse is scoped to themain:entry underprotected_branchesso a release branch requiring its own context does not produce a false failure. The checker also requires the aggregator'sname:to be the label-routing expression, with alabeledbranch that differs from the required name, and rejects an.asf.yamlthat requires the label-run name.This PR also closes #5860. Once
Required Checksis required, any red job evicts the PR from the merge queue, and the first two runs on this branch each failed in a step that had nothing to do with a test. So the steps that run after the verdict is known, or before any test starts, now retry or tolerate network failures:.github/actions/java-test: the test-report upload iscontinue-on-error: true. It runs on green jobs, nothing downstream consumes the reports, and aFinalizeArtifact403 should not turn a passing run red. The two failure-only uploads are unchanged.iceberg_spark_test_reusable.yml: the shard-inventory upload goes through./.github/actions/upload-artifact-retry.iceberg-spark-shard-coveragedownloads it, so it already fell under the README rule for artifacts a later job consumes.pr_build_linux.yml:Lint Scala (syntactic)is split into a retried no-opcs launch scalafix:0.14.6 -- --versionthat populates the coursier cache, then the real check undercs launch --mode offline. A nonzero exit from the check can now only be a lint violation.ci.ymlpreflight: the actionlint download is retried, and the installer is fetched to a file instead of piped intobash.The README's "Retrying flaky network operations" section documents all of this.
How are these changes tested?
preflightalready runscheck-ci-config.py, so the new invariants gate every PR from here on. I mutation-tested the failure modes against a clean baseline: dropping a job fromneeds, renaming the aggregator job while.asf.yamlrequires it, typoing the context in.asf.yaml, adding a new heavy job that forgets to register, adding a hyphenated job id that forgets to register, adding a required context tobranch-0.17only, reverting the aggregator name to a literal, giving both branches of the name expression the same literal, and requiring the label-run name in.asf.yaml. All are caught with a specific message except thebranch-0.17case, which correctly passes.actionlintandprettier --check "**/*.md"are clean. I ran the offline scalafix invocation locally against a warm coursier cache and it exits 0 on this tree, so--mode offlineis compatible with the pinnedscalafix:0.14.6app descriptor.The job is also observable on this PR itself. Over the first two runs it correctly went red on
pr_build_linux: failurewhile treating the skippedspark_3_4,spark_4_0, and older Iceberg legs as passes. Both of those failures were infra flakes in steps that run after the test verdict is known (an artifactFinalizeArtifact403, and a Maven Central connection reset inLint Scala). That is the sort of thing the watch period was meant to surface, and it is why #5860 is folded into this PR rather than left for later.🤖 Generated with Claude Code
https://claude.ai/code/session_01BtAqq4YJsk8uk42c7vHk8B