Uh oh!
There was an error while loading. Please reload this page.
ci: stop non-gating labels from skipping Preflight - #5784
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Reviewed 41957c37779f5e1caed11d674091e994daec77c3 against fefee03d94045ecd0ac5d3a1edb98a555f5ff21d. I found no verified P1/P2 issue. The change addresses two related behaviors: unrelated label events could leave the latest Preflight check skipped, while adding a gating label could repeat unrelated heavy jobs. Removing Preflight's event guard and moving label selection into each heavy caller preserves a real preflight verdict while narrowing the work scheduled by labels.
The predicates match the intended event policy. On a received label event, run-spark-3.4-tests selects Spark 3.4, run-spark-4.0-tests selects Spark 4.0, and run-iceberg-tests selects Iceberg 1.8/1.9/1.10. Existing labels do not cause unrelated families to run. Other labels, including the obsolete run-spark-4.1-tests, schedule only Preflight and change detection. Opens, pushes, synchronizations, reopens and manual dispatch retain their prior behavior. Path filters still apply, and the unchanged needs chain prevents heavy jobs from proceeding after a failed or skipped prerequisite.
The existing per-label concurrency key separates different label events from one another and from commit events. Triggers, permissions, checkout behavior and all six reusable workflows are unchanged. CI remains a pull_request workflow, while the separate pull_request_target labeler continues to use base-repository code. Removing a job condition does not change GitHub's fork approval policy. Also, labels added using the workflow's GITHUB_TOKEN do not create another labeled-event run, so the automatic labeler does not introduce a recursive CI loop. These boundaries follow the source and GitHub's event guidance and token-trigger rules. I did not execute a fork workflow or audit live organization settings.
The skipped-check limitation is described accurately: GitHub treats skipped jobs as passing. Heavy caller names can therefore still appear skipped, and this change does not turn them into enforcement checks. The repository's .asf.yaml declares an approving-review requirement for main and no required status-check list. That source declaration is not a claim about all live repository rules.
Validation covered the complete local workflow/action call graph, 180 offline event fixtures with label combinations, false/missing path outputs and prerequisite outcomes, and four path-classifier examples. The parsed predicates passed 120,960 assertions, including 67,200 comparisons showing unchanged behavior outside label events. These are local models of documented expression and dependency semantics, not GitHub runner executions. At September 9, 03:34 UTC, the current-head CI audit showed 68 successful and nine skipped checks, with no failures. Logs confirm successful Preflight and change detection at merge 36a236a6, whose parents are exactly the reviewed base/head and whose tree equals the head. This validates the ordinary CI run, not the label-event scenarios themselves.
Only workflow orchestration and its README change. Spark expression/operator behavior, types, nulls, ANSI handling and fallback semantics are unaffected, so no Spark runtime qualification or maintained-source compatibility claim is made.
Performance
The new guards avoid redundant builds and tests for unrelated suite families. Each received label event still pays for Preflight and change detection, including the full-history checkout used for path detection. In the observed ordinary run those jobs took 131 and 33 seconds respectively. Those are individual job durations, not a label-run benchmark or a measured estimate of net savings. No execution hot path or applicable expression microbenchmark changes.
Design
Keeping Preflight unconditional makes its result meaningful, while per-caller predicates express which expensive work a label requests. The existing success dependencies and path filters remain intact. The README now matches the Spark and Iceberg suite defaults and explains the remaining skipped caller names. I found no actionable design concern requiring a broader workflow split in this patch.
Abstraction & complexity
The change uses the existing orchestration structure and adds no new workflow, action or approval mechanism. Explicit label comparisons are easy to inspect at each caller. The repeated conditions remain small enough that another dispatcher abstraction would not clearly improve this change.
Uh oh!
There was an error while loading. Please reload this page.
Which issue does this PR close?
Closes#5007.
Closes#5333
Rationale for this change
ci.ymlfires onpull_request.types: [labeled], and GitHub cannot filter that trigger by label name.preflightguarded itself with anif:on the four gating labels, so any other label started a run in whichPreflight,Detect changesand all 12 heavy jobs reportedskipped.A job held back by
if:still publishes a check run under its own name, and the newest check run for a name is what the merge box,gh pr checksand required-status-check evaluation read. An unrelated label therefore overwrote the commit run's realPreflightverdict withskipped. Of the 170 open PRs today, 28 reportPreflight: skippedas their latest verdict, and 14 of those had an earliersuccessthat is no longer reachable. #5721 is one example: green on 2026-09-05, and now every CI row reads skipped.The same
if:chain had a second problem. A gating label re-ran the whole pipeline instead of just the job it gates. Run 34126169891 (run-iceberg-testson #5759) rebuilt Linux, macOS, Spark 3.5, Spark 4.1 and all four Iceberg jobs at a commit that had just been tested.What changes are included in this PR?
preflightloses itsif:guard and runs on every event, soPreflightalways reports a real verdict. Cost is about a minute ofubuntu-slimper label event.labeledevents unless the label just added is the one it gates, so applying a label no longer duplicates a pipeline..github/workflows/README.md: corrected five stale rows in "What runs when", added the missingiceberg_1_11, redrew the diagram, and added a "Label events" section recording whypreflightmust stay unguarded.Not addressed here: a non-gating label still starts a run, so the bare caller names (
PR Build (Linux), ...) still publishskippedrows. Those names never carry a passing verdict anyway, because a reusable workflow that runs publishes only its child jobs. Removing them needs the label trigger split into its own workflow.