felipepessoto commented on PR #12781:
URL: https://github.com/apache/gluten/pull/12781#issuecomment-5349315405
For the record, since a reviewer will reasonably ask why this is stricter
than the repo's existing comment triggers — it is, and deliberately. Every
`issue_comment` workflow in the repo today:
| Workflow | Command | Who may trigger | Body match |
|---|---|---|---|
| `velox_backend_ansi.yml` | `/ansi-test`, `/ansi-analyze` | **anyone** — no
check | `contains()` |
| `take.yml` | `take` | **anyone** (correct — you're claiming an unassigned
issue) | `== 'take'` |
| `delta_spark_ut.yml` (this PR) | `/delta-test` | **PR author only** |
exact token |
Grepping `.github/workflows/` for `author_association` or a permission
lookup returns only the explanatory comments in this PR's own file — so there
is no existing precedent being followed here; this is the first authorisation
gate of any kind.
Notably `/ansi-test` is not cheap either (8 jobs: a native build plus four
Spark-UT matrices) and uses `contains()`, so merely quoting the command in a
sentence starts it, from any GitHub account.
**Why author-only is nonetheless the right scope here**, rather than adding
a maintainer path: an `author_association` allow-list is inaccurate on an ASF
repo (established earlier in this PR — it misclassifies 16 of 18 committers as
`CONTRIBUTOR`), and the accurate check, `GET
/repos/{owner}/{repo}/collaborators/{login}/permission`, would grant access to
people who already have it. Maintainers can start this suite from **Actions →
Run workflow** today. #12743 frames the gap precisely the other way round:
> The problem is the PR author might not be able to add it, only maintainers
can change the PR labels.
The author was the only party with no way in — labelling needs write/triage,
so a fork author cannot — which is exactly what this trigger restores, and
nothing more. A maintainer wanting a run on someone else's PR uses
`workflow_dispatch`, or asks the author to comment.
Rationale recorded in the workflow and README in `239c929` (comments only,
no logic change). If reviewers would prefer to match `/ansi-test` and drop the
check, or to add the `collaborators/…/permission` lookup so maintainers can
trigger it directly on a fork PR, both are small changes — say the word.
--
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]
