Skip to content

ci: move the job routing policy out of ci.yml expressions and into compute-changes.py - #5850

Merged
andygrove merged 2 commits into
apache:mainfrom
andygrove:ci-compute-changes-policy
Sep 11, 2026
Merged

ci: move the job routing policy out of ci.yml expressions and into compute-changes.py#5850
andygrove merged 2 commits into
apache:mainfrom
andygrove:ci-compute-changes-policy

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

Part of #5838. Pure refactor — no behaviour change.

Rationale for this change

Every heavy job in ci.yml carries a four-line ${{ }} expression that ANDs together a path-filter output, an event-name test, an opt-in label test, and a special case for labeled events:

if: |
  needs.changes.outputs.spark_3_4 == 'true' &&
  (github.event_name == 'push' ||
   github.event_name == 'workflow_dispatch' ||
   (github.event_name == 'pull_request' &&
    contains(github.event.pull_request.labels.*.name, 'run-spark-3.4-tests') &&
    (github.event.action != 'labeled' ||
     github.event.label.name == 'run-spark-3.4-tests')))

Ten jobs, ten near-identical copies. Half the routing policy already lives in compute-changes.py; this is the other half, expressed in a language that can't be tested and has to be evaluated by hand to review.

I noticed it while sizing up the merge-queue work in #5838 — that change would have meant editing all ten of these. It's cheaper to fix the shape first.

What changes are included in this PR?

The event/label policy moves into dev/ci/compute-changes.py, next to the path filters it was already being ANDed with. Each gate becomes:

if: needs.changes.outputs.spark_3_4 == 'true'

and the routing is one table:

POLICY = {
    "spark_3_5": ["pr", "push"],
    "spark_3_4": ["push", "label:run-spark-3.4-tests"],
    ...
}

ci.yml loses 62 lines net.

The real payoff is that the policy is now testable. check-ci-config.py grows POLICY_CASES, pinning the expected job set for each event shape — including that a non-gating label like dependabot's dependencies starts nothing, which is #5007 and was previously only enforceable by reading YAML very carefully.

One thing that fell out of writing the tests: "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. check-ci-config.py now rejects that combination rather than letting it be a silent no-op.

How are these changes tested?

Differentially, against the code being replaced. I extracted the pre-refactor if: expressions from ci.yml at the merge base, translated them to Python, and evaluated them against the new POLICY over every combination of event name, pull_request action, label set and added label — 1464 (job, event) pairs, zero mismatches.

To confirm the harness has teeth, three seeded regressions are each caught: renaming a gating label (32 mismatches), dropping a label gate entirely (32), and dropping the labeled-event narrowing that #5007 was about (105).

POLICY_CASES is the part that ships, since the differential harness needs the old YAML to compare against. actionlint and prettier --check "**/*.md" are clean.

…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.
@andygrove
andygrove marked this pull request as ready for review September 10, 2026 21:42
@andygrove
andygrove merged commit 9388ecb into apache:main Sep 11, 2026
151 of 152 checks passed
andygrove added a commit to andygrove/datafusion-comet that referenced this pull request Sep 12, 2026
…directly

`Verify TPC-H Results` failed on apache#5850 at its `Build project` step, 105 seconds
in and before anything was compiled. The check-run annotation gives the cause:

  https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.6/apache-maven-3.9.6-bin.zip

That is `./mvnw` downloading the Maven distribution itself, not a dependency
and not a test.

`./.github/actions/java-test` already handles this: it caches the distribution
under `~/.m2/wrapper/dists` and retries `./mvnw --version` four times with
exponential backoff. But five jobs in pr_build_linux.yml never go through
java-test -- they invoke `./mvnw` directly -- so none of them had either the
cache or the retry:

  lint-java                          scalafix check
  build-spark-4-1                    compile, skip tests
  celeborn-reflection-compatibility  reflected-internals check
  verify-benchmark-results-tpch      the job that failed
  verify-benchmark-results-tpcds     same shape as TPC-H

Extract the cache/retry/save sequence into `./.github/actions/maven-bootstrap`
and call it from all five before their first Maven use. java-test keeps its
inline copy: a local action invoking another local action is deliberately
avoided in this repository, and the workflows README already says so about the
artifact-upload wrapper.

Register the new action in the Linux change filter. Without it, a later edit
confined to `.github/actions/maven-bootstrap/**` routes to nothing: the
`changes` gate reports `build_linux=false`, ci.yml skips the whole Linux
workflow, and the edit merges without any of the five consumers having run it.
This PR does not expose the gap, because it also edits pr_build_linux.yml. Pin
the routing with a case in dev/ci/check-ci-config.py so a filter deletion
cannot pass unnoticed either.

The workflows README claimed this failure mode was handled. It was, but only
for jobs routed through java-test; the wording is corrected.

Worth noting for apache#5838: under a merge queue these jobs gate the queue, so this
failure mode would block every merge rather than costing one PR a re-run.

Follow-up, not covered here: `ci.yml`'s RAT check and the direct `./mvnw` calls
in `pr_benchmark_check.yml`, `pyarrow_udf_test.yml` and
`iceberg_spark_test_reusable.yml` have the same gap. Left out to keep this
reviewable against the failure that prompted it.
peterxcli pushed a commit to peterxcli/datafusion-comet that referenced this pull request Sep 13, 2026
…directly (apache#5852)

`Verify TPC-H Results` failed on apache#5850 at its `Build project` step, 105 seconds
in and before anything was compiled. The check-run annotation gives the cause:

  https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.6/apache-maven-3.9.6-bin.zip

That is `./mvnw` downloading the Maven distribution itself, not a dependency
and not a test.

`./.github/actions/java-test` already handles this: it caches the distribution
under `~/.m2/wrapper/dists` and retries `./mvnw --version` four times with
exponential backoff. But five jobs in pr_build_linux.yml never go through
java-test -- they invoke `./mvnw` directly -- so none of them had either the
cache or the retry:

  lint-java                          scalafix check
  build-spark-4-1                    compile, skip tests
  celeborn-reflection-compatibility  reflected-internals check
  verify-benchmark-results-tpch      the job that failed
  verify-benchmark-results-tpcds     same shape as TPC-H

Extract the cache/retry/save sequence into `./.github/actions/maven-bootstrap`
and call it from all five before their first Maven use. java-test keeps its
inline copy: a local action invoking another local action is deliberately
avoided in this repository, and the workflows README already says so about the
artifact-upload wrapper.

Register the new action in the Linux change filter. Without it, a later edit
confined to `.github/actions/maven-bootstrap/**` routes to nothing: the
`changes` gate reports `build_linux=false`, ci.yml skips the whole Linux
workflow, and the edit merges without any of the five consumers having run it.
This PR does not expose the gap, because it also edits pr_build_linux.yml. Pin
the routing with a case in dev/ci/check-ci-config.py so a filter deletion
cannot pass unnoticed either.

The workflows README claimed this failure mode was handled. It was, but only
for jobs routed through java-test; the wording is corrected.

Worth noting for apache#5838: under a merge queue these jobs gate the queue, so this
failure mode would block every merge rather than costing one PR a re-run.

Follow-up, not covered here: `ci.yml`'s RAT check and the direct `./mvnw` calls
in `pr_benchmark_check.yml`, `pyarrow_udf_test.yml` and
`iceberg_spark_test_reusable.yml` have the same gap. Left out to keep this
reviewable against the failure that prompted it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ci CI/CD, GitHub Actions, build tooling build Build environment enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants