diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 16f0d333731..935e11272d8 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -51,7 +51,7 @@ is a `workflow_call` reusable invoked from the umbrella. ## What runs when -| Job in `ci.yml` | Triggered by | Path filter source | +| Job in `ci.yml` | Triggered by | Routing rule | | -------------------- | --------------------------------------------------- | ----------------------------------- | | `preflight` | every PR / push to main / dispatch / PR label added | none (always runs) | | `changes` | every PR / push to main / dispatch / PR label added | runs `dev/ci/compute-changes.py` | @@ -90,9 +90,9 @@ Two rules keep those runs from corrupting the PR's status: `preflight` on the label name used to let any unrelated label overwrite the commit run's real `Preflight` verdict with `skipped`, see [#5007](https://github.com/apache/datafusion-comet/issues/5007). -- Every heavy job excludes `labeled` events unless the label just added is the - one that gates it. Without that, applying a single label re-ran the entire - heavy pipeline at a commit that had already been tested. +- On a `labeled` event, `POLICY` reports false for every job the new label does + not gate. Without that, applying a single label re-ran the entire heavy + pipeline at a commit that had already been tested. `run-spark-4.1-tests` gates nothing: `spark_4_1` already runs on every PR. @@ -122,13 +122,30 @@ umbrella doesn't watch, or operate independently of the rest of CI: | `spark_sql_test_reusable.yml` | `spark_3_4`, `spark_3_5`, `spark_4_0`, `spark_4_1` | | `iceberg_spark_test_reusable.yml` | `iceberg_1_8`, `iceberg_1_9`, `iceberg_1_10`, `iceberg_1_11` | -## Modifying path filters +## Changing what runs when -Each long workflow's "what files trigger me" rules live in the `FILTERS` -dict at the top of `dev/ci/compute-changes.py`. The `changes` job in -`ci.yml` invokes that script and the gate `if:` on each long job consumes -`needs.changes.outputs.`. When adding a new test suite or moving -sources, update the relevant filter entry there. +Every heavy job in `ci.yml` is gated on exactly one thing: + +```yaml +if: needs.changes.outputs.spark_3_5 == 'true' +``` + +That single boolean folds together two separate decisions, both of which live +in `dev/ci/compute-changes.py`: + +- **`FILTERS`** — which files the job covers. Pattern semantics match + dorny/picomatch (`**` spans path segments, `*` stays within one, a leading + `!` excludes). +- **`POLICY`** — which events may run it. `"pr"` for every pull request, + `"push"` for push to main, `"label:"` for opt-in on a labelled pull + request. `"pr"` and `"label:"` are mutually exclusive. `workflow_dispatch` + always runs everything. + +So adding a suite, moving sources, or changing when something runs is an edit +to one of those two tables, not to ten `${{ }}` expressions. Keeping the policy +in Python is also what makes it testable: GitHub expressions cannot be +exercised outside a real workflow run, whereas `POLICY_CASES` in +`dev/ci/check-ci-config.py` pins the expected job set for each event shape. A file that a job reads but that no filter lists is silent: the job skips, and the edit merges with only `preflight` having looked at it. The shared diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bdc1b1a14f4..f53d854ba69 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -54,7 +54,8 @@ jobs: # `Preflight` verdict with `skipped` (issue #5007). Running it unconditionally # costs about a minute of ubuntu-slim time per label event and keeps the # reported verdict truthful. Skipping the redundant work is the heavy jobs' - # job, and they filter `labeled` events themselves below. + # job; POLICY in dev/ci/compute-changes.py drops every job a `labeled` event + # does not gate. # --------------------------------------------------------------------------- preflight: name: Preflight @@ -141,20 +142,23 @@ jobs: id: compute shell: bash env: - EVENT_NAME: ${{ github.event_name }} - PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} - PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - PUSH_BEFORE: ${{ github.event.before }} - PUSH_AFTER: ${{ github.sha }} + EVENT_NAME: ${{ github.event_name }} + EVENT_ACTION: ${{ github.event.action }} + LABEL_NAME: ${{ github.event.label.name }} + PR_LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }} + PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} + PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + PUSH_BEFORE: ${{ github.event.before }} + PUSH_AFTER: ${{ github.sha }} run: | set -euo pipefail + : > changed_files.txt if [[ "$EVENT_NAME" == "workflow_dispatch" ]]; then - for key in build_linux build_macos benchmark docs spark_3_4 spark_3_5 spark_4_0 spark_4_1 iceberg_1_8 iceberg_1_9 iceberg_1_10 iceberg_1_11; do - echo "${key}=true" >> "$GITHUB_OUTPUT" - done - exit 0 - fi - if [[ "$EVENT_NAME" == "pull_request" ]]; then + # No meaningful base to diff against; compute-changes.py forces + # every output true for this event so a manual run can exercise + # any gated job. + : + elif [[ "$EVENT_NAME" == "pull_request" ]]; then git diff --name-only "$PR_BASE_SHA"..."$PR_HEAD_SHA" > changed_files.txt else # push to main; first push to a branch has all-zero before sha @@ -169,70 +173,47 @@ jobs: python3 dev/ci/compute-changes.py changed_files.txt >> "$GITHUB_OUTPUT" # --------------------------------------------------------------------------- - # Heavy jobs: each is a thin caller of an existing reusable workflow. The - # `if:` expressions encode the same event/label/path criteria the - # standalone trigger workflows used to encode in their `on:` blocks. + # Heavy jobs: each is a thin caller of an existing reusable workflow, gated + # on the one `changes` output that covers it. # - # On a `labeled` event only the job that the newly added label gates runs. - # Every other heavy job already ran on the opened/synchronize event at the - # same commit, so letting them through would duplicate an entire pipeline - # each time somebody applies a label. + # That single output already folds in everything these gates used to spell + # out inline: which files the job cares about, which events may run it, and + # which opt-in label it needs on a pull request. All of it lives in FILTERS + # and POLICY in dev/ci/compute-changes.py, where it is one table instead of + # ten near-identical `${{ }}` expressions, and where dev/ci/check-ci-config.py + # can actually test it. # --------------------------------------------------------------------------- pr_build_linux: name: PR Build (Linux) needs: changes - if: | - needs.changes.outputs.build_linux == 'true' && - (github.event_name == 'push' || - github.event_name == 'workflow_dispatch' || - (github.event_name == 'pull_request' && - github.event.action != 'labeled')) + if: needs.changes.outputs.build_linux == 'true' uses: ./.github/workflows/pr_build_linux.yml pr_build_macos: name: PR Build (macOS) needs: changes - if: | - needs.changes.outputs.build_macos == 'true' && - (github.event_name == 'push' || - github.event_name == 'workflow_dispatch' || - (github.event_name == 'pull_request' && - github.event.action != 'labeled')) + if: needs.changes.outputs.build_macos == 'true' uses: ./.github/workflows/pr_build_macos.yml pr_benchmark_check: name: PR Benchmark Check needs: changes - if: | - needs.changes.outputs.benchmark == 'true' && - (github.event_name == 'push' || - github.event_name == 'workflow_dispatch' || - (github.event_name == 'pull_request' && - github.event.action != 'labeled')) + if: needs.changes.outputs.benchmark == 'true' uses: ./.github/workflows/pr_benchmark_check.yml docs: name: Deploy Comet site needs: changes # docs deploys to asf-site, so only run on push-to-main (or a manual dispatch). - if: | - needs.changes.outputs.docs == 'true' && - (github.event_name == 'push' || github.event_name == 'workflow_dispatch') + if: needs.changes.outputs.docs == 'true' uses: ./.github/workflows/docs.yaml spark_3_4: name: Spark SQL Tests (Spark 3.4) needs: changes # Main-only by default; PRs need the `run-spark-3.4-tests` label. - 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'))) + if: needs.changes.outputs.spark_3_4 == 'true' uses: ./.github/workflows/spark_sql_test_reusable.yml with: spark-short: '3.4' @@ -242,12 +223,7 @@ jobs: spark_3_5: name: Spark SQL Tests (Spark 3.5) needs: changes - if: | - needs.changes.outputs.spark_3_5 == 'true' && - (github.event_name == 'push' || - github.event_name == 'workflow_dispatch' || - (github.event_name == 'pull_request' && - github.event.action != 'labeled')) + if: needs.changes.outputs.spark_3_5 == 'true' uses: ./.github/workflows/spark_sql_test_reusable.yml with: spark-short: '3.5' @@ -260,14 +236,7 @@ jobs: # Main-only by default; PRs need the `run-spark-4.0-tests` label. Swapped # with spark_4_1 on the `oom` branch to validate the memory caps against # Spark 4.1 by default. - if: | - needs.changes.outputs.spark_4_0 == 'true' && - (github.event_name == 'push' || - github.event_name == 'workflow_dispatch' || - (github.event_name == 'pull_request' && - contains(github.event.pull_request.labels.*.name, 'run-spark-4.0-tests') && - (github.event.action != 'labeled' || - github.event.label.name == 'run-spark-4.0-tests'))) + if: needs.changes.outputs.spark_4_0 == 'true' uses: ./.github/workflows/spark_sql_test_reusable.yml with: spark-short: '4.0' @@ -277,12 +246,7 @@ jobs: spark_4_1: name: Spark SQL Tests (Spark 4.1) needs: changes - if: | - needs.changes.outputs.spark_4_1 == 'true' && - (github.event_name == 'push' || - github.event_name == 'workflow_dispatch' || - (github.event_name == 'pull_request' && - github.event.action != 'labeled')) + if: needs.changes.outputs.spark_4_1 == 'true' uses: ./.github/workflows/spark_sql_test_reusable.yml with: spark-short: '4.1' @@ -293,14 +257,7 @@ jobs: name: Iceberg Spark SQL Tests (Iceberg 1.8) needs: changes # Main-only by default; PRs need the `run-iceberg-tests` label. - if: | - needs.changes.outputs.iceberg_1_8 == 'true' && - (github.event_name == 'push' || - github.event_name == 'workflow_dispatch' || - (github.event_name == 'pull_request' && - contains(github.event.pull_request.labels.*.name, 'run-iceberg-tests') && - (github.event.action != 'labeled' || - github.event.label.name == 'run-iceberg-tests'))) + if: needs.changes.outputs.iceberg_1_8 == 'true' uses: ./.github/workflows/iceberg_spark_test_reusable.yml with: iceberg-short: '1.8' @@ -313,14 +270,7 @@ jobs: name: Iceberg Spark SQL Tests (Iceberg 1.9) needs: changes # Main-only by default; PRs need the `run-iceberg-tests` label. - if: | - needs.changes.outputs.iceberg_1_9 == 'true' && - (github.event_name == 'push' || - github.event_name == 'workflow_dispatch' || - (github.event_name == 'pull_request' && - contains(github.event.pull_request.labels.*.name, 'run-iceberg-tests') && - (github.event.action != 'labeled' || - github.event.label.name == 'run-iceberg-tests'))) + if: needs.changes.outputs.iceberg_1_9 == 'true' uses: ./.github/workflows/iceberg_spark_test_reusable.yml with: iceberg-short: '1.9' @@ -334,14 +284,7 @@ jobs: needs: changes # Main-only by default; PRs need the `run-iceberg-tests` label. Iceberg 1.11 # (Spark 4.1) is the PR-gated Iceberg job; 1.10 covers the Spark 3.5 path. - if: | - needs.changes.outputs.iceberg_1_10 == 'true' && - (github.event_name == 'push' || - github.event_name == 'workflow_dispatch' || - (github.event_name == 'pull_request' && - contains(github.event.pull_request.labels.*.name, 'run-iceberg-tests') && - (github.event.action != 'labeled' || - github.event.label.name == 'run-iceberg-tests'))) + if: needs.changes.outputs.iceberg_1_10 == 'true' uses: ./.github/workflows/iceberg_spark_test_reusable.yml with: iceberg-short: '1.10' @@ -354,12 +297,7 @@ jobs: name: Iceberg Spark SQL Tests (Iceberg 1.11) needs: changes # Runs on every PR: Iceberg 1.11 is our only Spark 4.1 Iceberg coverage. - if: | - needs.changes.outputs.iceberg_1_11 == 'true' && - (github.event_name == 'push' || - github.event_name == 'workflow_dispatch' || - (github.event_name == 'pull_request' && - github.event.action != 'labeled')) + if: needs.changes.outputs.iceberg_1_11 == 'true' uses: ./.github/workflows/iceberg_spark_test_reusable.yml with: iceberg-short: '1.11' diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index fb48400175b..5b8c9d6e0cf 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -15,14 +15,20 @@ # specific language governing permissions and limitations # under the License. -# Guards two CI invariants that are silent when broken: +# Guards three CI invariants that are silent when broken: # # 1. Change-filter routing. dev/ci/compute-changes.py decides which heavy # jobs run. A file that a job depends on but that no filter lists makes # that job skip, so the edit merges with only preflight having looked at # it. The table below pins the routing for the shared build inputs. # -# 2. Artifact-name uniqueness. Artifact names are scoped to the *run*, not +# 2. Event policy. The same script decides which events may run each job. +# That used to be a `${{ }}` expression on every job in ci.yml, where it +# could not be tested; POLICY_CASES below is the test it never had. The +# expected sets are transcribed from the `if:` expressions ci.yml carried +# before the policy moved, so a regression here is a behaviour change. +# +# 3. Artifact-name uniqueness. Artifact names are scoped to the *run*, not # to the calling workflow, and ci.yml calls the Spark SQL and Iceberg # reusable workflows several times in one run. Two producers sharing a # name make `download-artifact` pick by highest artifact ID rather than @@ -67,6 +73,72 @@ (["native/core/benches/parquet_read.rs"], {"benchmark"}), ] +# Event policy. Each case is (event, expected set of jobs allowed to run), +# where "allowed" ignores path filters. Transcribed from the `if:` expressions +# ci.yml carried before POLICY moved into compute-changes.py, so these pin the +# pre-refactor behaviour rather than restating the new code. +PR_TIER = {"build_linux", "build_macos", "benchmark", "spark_3_5", "spark_4_1", "iceberg_1_11"} +ICEBERG_OPT_IN = {"iceberg_1_8", "iceberg_1_9", "iceberg_1_10"} +ALL_JOBS = PR_TIER | ICEBERG_OPT_IN | {"docs", "spark_3_4", "spark_4_0"} + +POLICY_CASES = [ + # A manual run may exercise anything. + ({"name": "workflow_dispatch"}, ALL_JOBS), + # Push to main runs every job, docs included: it is the only event that + # may deploy the site. + ({"name": "push"}, ALL_JOBS), + # A plain pull request: the PR tier only. docs must never run here, and the + # opt-in suites stay off without their label. + ({"name": "pull_request", "action": "opened", "labels": []}, PR_TIER), + ({"name": "pull_request", "action": "synchronize", "labels": []}, PR_TIER), + # An opt-in label present on a pushed commit adds just that suite. + ( + {"name": "pull_request", "action": "synchronize", "labels": ["run-spark-3.4-tests"]}, + PR_TIER | {"spark_3_4"}, + ), + ( + {"name": "pull_request", "action": "synchronize", "labels": ["run-iceberg-tests"]}, + PR_TIER | ICEBERG_OPT_IN, + ), + # Applying a gating label runs only what that label gates. The PR tier + # already ran at this commit on opened/synchronize. + ( + { + "name": "pull_request", + "action": "labeled", + "label": "run-spark-4.0-tests", + "labels": ["run-spark-4.0-tests"], + }, + {"spark_4_0"}, + ), + ( + { + "name": "pull_request", + "action": "labeled", + "label": "run-iceberg-tests", + "labels": ["run-iceberg-tests"], + }, + ICEBERG_OPT_IN, + ), + # A label that gates nothing (dependabot's `dependencies`, a type label) + # must not start a second pipeline. This is issue #5007. + ( + {"name": "pull_request", "action": "labeled", "label": "dependencies", "labels": ["dependencies"]}, + set(), + ), + # ... not even when a gating label is already on the PR from earlier. + ( + { + "name": "pull_request", + "action": "labeled", + "label": "dependencies", + "labels": ["dependencies", "run-spark-3.4-tests"], + }, + set(), + ), +] + + # `uses:` values that publish an artifact, and the one that consumes it. UPLOAD_USES = re.compile(r"uses:\s*(\./\.github/actions/upload-artifact-retry|actions/upload-artifact@)") DOWNLOAD_USES = re.compile(r"uses:\s*actions/download-artifact@") @@ -100,6 +172,44 @@ def check_change_filters(): return not failures +def check_event_policy(): + module = load_filters() + failures = [] + for event, expected in POLICY_CASES: + actual = {job for job in module.POLICY if module.event_allows(job, event)} + if actual != expected: + label = event.get("name") + if event.get("action"): + label += f"/{event['action']}" + if event.get("label"): + label += f" +{event['label']}" + failures.append( + f"{label} labels={event.get('labels', [])}: " + f"unexpectedly allowed {sorted(actual - expected) or 'nothing'}, " + f"unexpectedly blocked {sorted(expected - actual) or 'nothing'} " + f"(see POLICY in dev/ci/compute-changes.py)" + ) + missing = sorted(set(module.FILTERS) - set(module.POLICY)) + if missing: + failures.append( + f"jobs in FILTERS with no POLICY entry: {', '.join(missing)}; " + f"they would never run on any event" + ) + # "pr" next to a "label:" tier reads as "runs on every PR, and also when + # labelled", but the label check wins and the "pr" is dead. Reject the + # combination so it cannot be written by accident. + for job, tiers in module.POLICY.items(): + if "pr" in tiers and module.gating_labels(job): + failures.append( + f"{job}: POLICY lists both 'pr' and a 'label:' tier. Those are " + f"mutually exclusive; drop 'pr' if the job is opt-in, or drop " + f"the label if it should run on every pull request" + ) + for failure in failures: + print(f"event policy: {failure}") + return not failures + + def artifact_names(path): """Return ([upload names], [download names]) for one workflow file.""" uploads, downloads = [], [] @@ -151,6 +261,7 @@ def check_artifact_names(): if __name__ == "__main__": ok = check_change_filters() + ok = check_event_policy() and ok ok = check_artifact_names() and ok if not ok: sys.exit(1) diff --git a/dev/ci/compute-changes.py b/dev/ci/compute-changes.py index 89d117b06c8..0eb4cc2446c 100644 --- a/dev/ci/compute-changes.py +++ b/dev/ci/compute-changes.py @@ -17,10 +17,23 @@ # Replacement for dorny/paths-filter, which is not on the apache org allow # list. Reads a list of changed files (one per line) and emits per-job -# "=true|false" lines suitable for $GITHUB_OUTPUT. Pattern semantics -# match dorny/picomatch: "**" spans path segments, "*" stays within a -# segment, and a leading "!" marks an exclude pattern. +# "=true|false" lines suitable for $GITHUB_OUTPUT. +# +# Each output folds together two independent questions: +# +# 1. Did the change touch files this job covers? FILTERS, below. Pattern +# semantics match dorny/picomatch: "**" spans path segments, "*" stays +# within a segment, and a leading "!" marks an exclude pattern. +# 2. Does this event permit the job to run at all? POLICY, below. +# +# Question 2 used to live in ci.yml as a four-line `${{ }}` expression +# repeated on every heavy job. Keeping it here instead means the whole +# routing policy is in one place, is readable without evaluating GitHub +# expression syntax in your head, and is covered by the cases in +# dev/ci/check-ci-config.py, which YAML expressions never could be. +import json +import os import re import sys from pathlib import Path @@ -285,6 +298,89 @@ ], } +# Which events may run each job, independent of the path filters above. +# +# "pr" every pull request +# "push" push to main +# "label:" a pull request carrying that label +# +# workflow_dispatch always runs everything, so it is not listed. "pr" and +# "label:" are mutually exclusive -- a job is either unconditional on pull +# requests or opt-in, never both -- and check-ci-config.py rejects a job that +# lists both rather than letting the label quietly win. +POLICY = { + "build_linux": ["pr", "push"], + "build_macos": ["pr", "push"], + "benchmark": ["pr", "push"], + # docs deploys to asf-site, so it must not run from a pull request. + "docs": ["push"], + "spark_3_4": ["push", "label:run-spark-3.4-tests"], + "spark_3_5": ["pr", "push"], + "spark_4_0": ["push", "label:run-spark-4.0-tests"], + "spark_4_1": ["pr", "push"], + "iceberg_1_8": ["push", "label:run-iceberg-tests"], + "iceberg_1_9": ["push", "label:run-iceberg-tests"], + "iceberg_1_10": ["push", "label:run-iceberg-tests"], + # Iceberg 1.11 is our only Spark 4.1 Iceberg coverage, so it is not opt-in. + "iceberg_1_11": ["pr", "push"], +} + + +def gating_labels(job): + return [t[len("label:"):] for t in POLICY[job] if t.startswith("label:")] + + +def event_allows(job, event): + """Does `event` permit `job` to run, ignoring which files changed? + + `event` is {"name", "action", "label", "labels"}: the workflow event name, + the pull_request action, the label just added on a `labeled` event, and the + labels currently on the pull request. + """ + tiers = POLICY[job] + name = event.get("name") + + if name == "workflow_dispatch": + return True + if name == "push": + return "push" in tiers + if name != "pull_request": + return False + + gates = gating_labels(job) + if gates: + if not any(label in event.get("labels", []) for label in gates): + return False + elif "pr" not in tiers: + return False + + # A `labeled` event fires at the same commit as the opened/synchronize run + # that already tested it, and GitHub cannot filter a pull_request trigger + # by label name. So on `labeled`, run only the job the new label gates; + # everything else would be duplicating a pipeline. See issue #5007 for what + # happens when this is expressed as a job-level `if:` instead. + if event.get("action") == "labeled": + return event.get("label") in gates + return True + + +def compute(files, event): + """Return {job: bool}, folding the path filter and the event policy.""" + return { + name: event_allows(name, event) and matches(patterns, files) + for name, patterns in FILTERS.items() + } + + +def event_from_env(): + labels = os.environ.get("PR_LABELS", "") + return { + "name": os.environ.get("EVENT_NAME", ""), + "action": os.environ.get("EVENT_ACTION", ""), + "label": os.environ.get("LABEL_NAME", ""), + "labels": json.loads(labels) if labels.strip() else [], + } + def glob_to_regex(pat): # Translate a picomatch-style glob to a regex. "**/" at the start or @@ -326,8 +422,14 @@ def matches(patterns, files): if __name__ == "__main__": + event = event_from_env() + # workflow_dispatch has no meaningful base to diff against, so the caller + # passes an empty list and every path filter is treated as matched. + if event["name"] == "workflow_dispatch": + for name in FILTERS: + print(f"{name}=true") + sys.exit(0) files_path = Path(sys.argv[1]) files = [line.strip() for line in files_path.read_text().splitlines() if line.strip()] - for name, patterns in FILTERS.items(): - flag = "true" if matches(patterns, files) else "false" - print(f"{name}={flag}") + for name, flag in compute(files, event).items(): + print(f"{name}={'true' if flag else 'false'}")