You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/09/10 18:13:51 UTC
[PR] ci: add a Required Checks aggregator job so main can have a required status check [datafusion-comet]
andygrove opened a new pull request, #5842:
URL: https://github.com/apache/datafusion-comet/pull/5842
## Which issue does this PR close?
Part of #5838.
## Rationale for this change
`main` requires no status checks today, only one approving review. Before we can add one — which #5838 needs, since a merge queue only ever waits on required checks — we need a name that is actually safe to require, and right now we don't have one.
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:
| Caller state | Check runs published |
| ---------------- | ----------------------------------------------------------------------------------------------------- |
| skipped by `if:` | one run named exactly `PR Build (Linux)`, conclusion `skipped` |
| ran | only `PR Build (Linux) / Spark 4.1, JDK 17 [exec]` and friends, and no bare `PR Build (Linux)` at all |
I 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_checks` job at the bottom of `ci.yml` that `needs:` every other job and publishes a single flat `Required Checks` context on every event. It runs `if: always()` and treats `skipped` as a pass, so it only goes red when an upstream job reports `failure` or `cancelled`.
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.yaml` names 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.py` also gains two invariants, in the same spirit as the ones already in there: every `ci.yml` job except `docs` has to appear in `required_checks.needs`, and once `.asf.yaml` does declare a required context, the job's `name:` has to keep matching it. Both sides of that pair are silent when broken.
## How are these changes tested?
`preflight` already runs `check-ci-config.py`, so the new invariants gate every PR from here on. I mutation-tested all four failure modes against a clean baseline — dropping a job from `needs`, renaming the aggregator job, typoing the context in `.asf.yaml`, and adding a new heavy job that forgets to register — and each one is caught with a specific message.
`actionlint` and `prettier --check "**/*.md"` are clean.
The job is also observable on this PR itself: `Required Checks` should show up in the checks list and be green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ci: add a Required Checks aggregator job so main can have a required status check [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5842:
URL: https://github.com/apache/datafusion-comet/pull/5842#discussion_r3989943377
##########
.github/workflows/ci.yml:
##########
@@ -305,3 +320,61 @@ jobs:
spark-short: '4.1'
spark-full: '4.1.3'
java: 17
+
+ # ---------------------------------------------------------------------------
+ # required_checks: one flat job that aggregates every other job's result, so
+ # that `main` has a single name it can safely require.
+ #
+ # Nothing requires it yet. It is landing first, on its own, so that we can
+ # watch it report on real pull requests before `.asf.yaml` names it in
+ # `required_status_checks` for `main`.
+ #
+ # None of the jobs above can be required directly, because the name a caller
+ # of a reusable workflow 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 no bare `PR Build (Linux)` at all
+ #
+ # So requiring the bare name would block every code change, and requiring a
+ # nested name would block every docs-only change. Both hang rather than fail,
+ # and a required context that never reports also locks `.asf.yaml` itself,
+ # which then needs an INFRA ticket to unwedge. Aggregating into one flat job,
+ # whose name is published on every event, avoids the whole class of problem.
+ #
+ # `if: always()` is what makes this work: without it the job inherits the
+ # default `success()` and is itself skipped the moment any dependency fails.
+ # ---------------------------------------------------------------------------
+ required_checks:
+ name: Required Checks
+ if: always()
Review 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 8074e0174 by making the check name depend on the event:
```yaml
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ci: add a Required Checks aggregator job so main can have a required status check [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #5842:
URL: https://github.com/apache/datafusion-comet/pull/5842
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ci: add a Required Checks aggregator job so main can have a required status check [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5842:
URL: https://github.com/apache/datafusion-comet/pull/5842#issuecomment-5635799556
Merged. Thanks @sunchao and @blaginin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ci: add a Required Checks aggregator job so main can have a required status check [datafusion-comet]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #5842:
URL: https://github.com/apache/datafusion-comet/pull/5842#discussion_r3982158864
##########
.github/workflows/ci.yml:
##########
@@ -367,3 +367,59 @@
spark-short: '4.1'
spark-full: '4.1.3'
java: 17
+
+ # ---------------------------------------------------------------------------
+ # required_checks: one flat job that aggregates every other job's result, so
+ # that `main` has a single name it can safely require.
+ #
+ # Nothing requires it yet. It is landing first, on its own, so that we can
+ # watch it report on real pull requests before `.asf.yaml` names it in
+ # `required_status_checks` for `main`.
+ #
+ # None of the jobs above can be required directly, because the name a caller
+ # of a reusable workflow 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 no bare `PR Build (Linux)` at all
+ #
+ # So requiring the bare name would block every code change, and requiring a
+ # nested name would block every docs-only change. Both hang rather than fail,
+ # and a required context that never reports also locks `.asf.yaml` itself,
+ # which then needs an INFRA ticket to unwedge. Aggregating into one flat job,
+ # whose name is published on every event, avoids the whole class of problem.
+ #
+ # `if: always()` is what makes this work: without it the job inherits the
+ # default `success()` and is itself skipped the moment any dependency fails.
+ # ---------------------------------------------------------------------------
+ required_checks:
+ name: Required Checks
+ if: always()
+ needs:
+ - preflight
+ - changes
+ - pr_build_linux
+ - pr_build_macos
+ - pr_benchmark_check
+ - spark_3_4
+ - spark_3_5
+ - spark_4_0
+ - spark_4_1
+ - iceberg_1_8
+ - iceberg_1_9
+ - iceberg_1_10
+ - iceberg_1_11
+ runs-on: ubuntu-slim
+ steps:
+ - name: Summarize upstream results
+ env:
+ NEEDS: ${{ toJSON(needs) }}
+ run: echo "$NEEDS"
+
+ # `skipped` is a pass: it means the change did not touch anything that
+ # job covers. Only `failure` and `cancelled` block the merge.
+ - name: Fail if any upstream job failed or was cancelled
+ if: contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled')
+ run: |
+ echo "::error::One or more upstream jobs did not succeed. See the results above."
+ exit 1
Review Comment:
## CodeQL / Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{}}
[Show more details](https://github.com/apache/datafusion-comet/security/code-scanning/147)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ci: add a Required Checks aggregator job so main can have a required status check [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5842:
URL: https://github.com/apache/datafusion-comet/pull/5842#discussion_r3989641133
##########
.github/workflows/ci.yml:
##########
@@ -305,3 +320,61 @@ jobs:
spark-short: '4.1'
spark-full: '4.1.3'
java: 17
+
+ # ---------------------------------------------------------------------------
+ # required_checks: one flat job that aggregates every other job's result, so
+ # that `main` has a single name it can safely require.
+ #
+ # Nothing requires it yet. It is landing first, on its own, so that we can
+ # watch it report on real pull requests before `.asf.yaml` names it in
+ # `required_status_checks` for `main`.
+ #
+ # None of the jobs above can be required directly, because the name a caller
+ # of a reusable workflow 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 no bare `PR Build (Linux)` at all
+ #
+ # So requiring the bare name would block every code change, and requiring a
+ # nested name would block every docs-only change. Both hang rather than fail,
+ # and a required context that never reports also locks `.asf.yaml` itself,
+ # which then needs an INFRA ticket to unwedge. Aggregating into one flat job,
+ # whose name is published on every event, avoids the whole class of problem.
+ #
+ # `if: always()` is what makes this work: without it the job inherits the
+ # default `success()` and is itself skipped the moment any dependency fails.
+ # ---------------------------------------------------------------------------
+ required_checks:
+ name: Required Checks
+ if: always()
Review 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](https://github.com/apache/datafusion-comet/issues/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](https://docs.github.com/en/repositories/configurin
g-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-status-checks-before-merging) under that name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org