felipepessoto commented on code in PR #12781:
URL: https://github.com/apache/gluten/pull/12781#discussion_r3788252935
##########
.github/workflows/delta_spark_ut.yml:
##########
@@ -139,19 +162,63 @@ env:
# Now that this is a standalone workflow (not called by velox_backend_x86.yml),
# `github.workflow` resolves to THIS workflow, so a concurrency group is both
safe
# and necessary: without it every push to a PR branch stacks another full ~2.5
h
-# Delta run instead of superseding the previous one. Keyed on the branch for
PRs
-# and the sha otherwise, matching velox_backend_x86.yml's group.
+# Delta run instead of superseding the previous one. Keyed on the PR number
when
+# there is one, so a `pull_request` run and a `/delta-test` run on the same PR
+# supersede each other -- `github.head_ref` is empty on `issue_comment`, which
+# would otherwise lump every commented-on PR into one shared group.
concurrency:
- group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{
github.workflow }}
+ group: ${{ github.repository }}-${{ github.event.pull_request.number ||
github.event.issue.number || github.head_ref || github.sha }}-${{
github.workflow }}
cancel-in-progress: true
jobs:
+ # Gate + acknowledgement for `/delta-test`, and nothing else: the ref to
check
+ # out is computed in `env.DELTA_CHECKOUT_REF` above, so this job has no
outputs
+ # and the rest of the pipeline just hangs off it via `needs`.
+ #
+ # Every other event passes straight through (the `if` short-circuits),
leaving
+ # their behaviour unchanged.
+ #
+ # Kept as its own job so that `pull-requests: write` -- needed to comment
back
+ # -- is never granted to a job that builds and runs the PR's code.
+ delta-test-requested:
+ # The PR author (the whole point: a fork author cannot label their own PR,
+ # but can always comment on it) or anyone with write access. `startsWith`,
+ # not `contains`, so quoting the command while discussing it does not spend
+ # ~11 job-hours.
Review Comment:
Good catch, and it turns out to be worse than a wording mismatch — on this
repo the allow-list was simply wrong, so I've removed it rather than reworded
it (d36c9dc).
ASF repos grant write access through Gitbox, not through GitHub
org/collaborator membership, so `author_association` doesn't track it at all.
Sampling recent `apache/gluten` PR review comments:
| association | who |
|---|---|
| `MEMBER` | zhouyuan, zhztheplayer |
| `COLLABORATOR` | *nobody* |
| `CONTRIBUTOR` | FelixYBW, JkSelf, KevinyhZou, QiangCai, baibaichen,
binmahone, jinchengchenghh, lgbo-ustc, liuneng1994, lviiii, marin-ma, rui-mo,
taiyang-li, weiting-chen, zhixingheyi-tian, zzcclp |
So `OWNER/MEMBER/COLLABORATOR` would have **rejected 16 of the 18
maintainers** while accepting any member of the `apache` org — the opposite of
the intent.
The gate is now just the PR author:
```yaml
github.event.comment.user.login == github.event.issue.user.login
```
That is exactly the gap this PR closes (a fork author cannot label their own
PR, but can always comment on it), it is precisely describable, and it is the
most conservative option for a ~11 job-hour run. A maintainer who wants a run
on someone else's PR asks the author to comment.
If reviewers would rather maintainers could trigger it directly, the correct
mechanism on an ASF repo is a real permission check — `GET
/repos/{owner}/{repo}/collaborators/{login}/permission` requiring
`write`/`maintain`/`admin` — which is a few lines in the gate job. Happy to add
it if wanted.
##########
.github/workflows/util/delta-spark-ut/README.md:
##########
@@ -97,12 +97,26 @@ longer shared between them, those PRs pay for the centos-7
native build twice
filter before creating the run, so an unrelated PR costs nothing at all.
Changes to general Velox/core/native code can also affect Delta offload, but
they're touched on most PRs, so per-PR they skip the suite — the nightly run
is
- the safety net.
+ the safety net, and `/delta-test` below forces a run on any PR the filter
+ skipped.
- **Nightly** — the **full** suite runs against the latest default branch on a
`schedule` (05:00 UTC), so regressions from general Velox/core changes are
still caught daily. The nightly run enforces the baseline **and** fails on
now-passing tests (`fail_on_fixed=true`), so baseline drift surfaces as a red
nightly — the signal to refresh `known-failures.txt`.
+- **On demand, from a PR** — comment **`/delta-test`** (as the first thing in
the
+ comment) to force a full run on a PR the `paths:` filter skipped. The **PR
+ author** — including from a fork, which is why this is a comment and not a
+ label — or anyone with write access can use it. It runs the PR's merge ref
with
+ the default settings and the baseline **enforced**; `update_baseline` stays
Review Comment:
Fixed in d36c9dc. Rather than reword the README to match the allow-list, I
removed the allow-list — on an ASF repo `author_association` doesn't track
write access at all (write comes from Gitbox), so `COLLABORATOR` matches nobody
here and 16 of the 18 `apache/gluten` maintainers I sampled report
`CONTRIBUTOR`. Details in the [thread on the workflow
file](https://github.com/apache/gluten/pull/12781#discussion_r3788252935).
The README now states the actual rule with no qualifiers: the **PR author**
comments `/delta-test`; a maintainer who wants a run on someone else's PR asks
the author to comment.
--
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]
