andygrove commented on code in PR #5885:
URL: https://github.com/apache/datafusion-comet/pull/5885#discussion_r3999747778
##########
.github/workflows/ci.yml:
##########
@@ -253,7 +256,10 @@ jobs:
spark_3_4:
name: Spark SQL Tests (Spark 3.4)
needs: changes
- # Queue-only by default; PRs need the `run-spark-3.4-tests` label.
+ # Spark 3.4 is deprecated, so this is the one test job the merge queue does
+ # not run: it needs the `run-spark-3.4-tests` label on a pull request, or a
+ # manual workflow_dispatch. It stays in `required_checks.needs` so that a
+ # labelled run still gates the pull request it was asked for.
Review Comment:
Good catch, and you're right — I had the mechanism backwards. Adding the
label fires `labeled`, the aggregator publishes `Required Checks (label run)`
on that event, and `.asf.yaml` only requires `Required Checks`, so keeping
`spark_3_4` in `needs` does nothing for the label run's own verdict. Dropping
the queue run means nothing picks it up afterwards either.
I looked at implementing the gating instead and don't think it's available
to us. On `labeled` the PR tier is deliberately skipped because it already ran
at that commit, so publishing that run as `Required Checks` would overwrite the
commit's real verdict with one that tested a single suite — exactly #5007. So
I've gone with documenting it.
Driving `compute-changes.py` for each event shape and pairing it with the
name the aggregator publishes:
| Event | `spark_3_4` runs | Aggregator name | Gates the merge |
| --- | --- | --- | --- |
| PR opened / pushed, no label | no | `Required Checks` | n/a |
| `run-spark-3.4-tests` just added | yes | `Required Checks (label run)` |
no, advisory |
| push with the label retained | yes | `Required Checks` | yes |
| PR opened with the label | yes | `Required Checks` | yes |
| `merge_group` | no | `Required Checks` | n/a |
`ac7405d3b` replaces the gating promise in the `spark_3_4` job comment, the
workflows README, `contributor-guide/ci.md` and
`contributor-guide/spark-sql-tests.md`. Each now says the label run is
advisory, that the next push with the label retained is what puts 3.4 into the
required verdict, and — since there's no queue run behind it — that the label
run's result has to actually be read before merging rather than relied on to
block. I added the caveat to `spark-sql-tests.md` too, since that's where the
`gh pr edit --add-label` command you quoted lives.
Also merged `apache/main` in. The conflicts were all with #5871, which moved
the Spark 4.1 `sql_hive` shards behind the queue and so edits the same tier
listings; resolved by keeping both moves.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
