[GLUTEN-12743][CI] Let contributors trigger the Delta Spark UT with a /delta-test PR comment - #12781
[GLUTEN-12743][CI] Let contributors trigger the Delta Spark UT with a /delta-test PR comment#12781felipepessoto wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in /delta-test PR comment trigger to run the Delta Spark UT workflow even when the existing paths: filter would otherwise skip it, using a gated issue_comment entrypoint and a shared checkout ref (refs/pull/N/merge) so the rest of the pipeline runs the PR code safely with read-only default permissions.
Changes:
- Add
issue_commenttrigger and adelta-test-requestedgate job to authorize/delta-testand post a run link back to the PR. - Introduce
DELTA_CHECKOUT_REFand apply it to all checkouts so comment-triggered runs test the PR merge ref. - Split cache restore/save so comment-triggered runs can restore caches but never write to default-branch-scoped caches.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.github/workflows/delta_spark_ut.yml |
Adds /delta-test comment trigger, authorization gate job, PR-merge-ref checkout, cache save gating, and tighter default permissions. |
.github/workflows/util/delta-spark-ut/README.md |
Documents the new /delta-test “on demand” trigger and its behavior/constraints. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/workflows/delta_spark_ut.yml:186
- The PR description says
/delta-testcan be triggered by the PR author or anyone with write access, but the implemented gate only allows the PR author (by design per prior discussion). Please update the PR description to match the current authorization behavior, or expand the gate to include the intended additional authorized users.
# Only the PR author may spend a run on their own PR -- which is exactly the
# gap this closes: a fork author cannot label their own PR, but can always
# comment on it.
.github/workflows/delta_spark_ut.yml:220
- The acknowledgement step is part of the gating job; if
gh pr commentfails (e.g., transient API failure), the wholedelta-test-requestedjob fails and the entire pipeline is blocked even though the request is authorized. Since this comment is informational only, it should not be able to fail the workflow run.
- name: Acknowledge the request
if: ${{ github.event_name == 'issue_comment' }}
env:
|
Thanks — surfacing the two suppressed comments from that review, since they aren't visible in the diff: 1. The whole pipeline hangs off - name: Acknowledge the request
if: ${{ github.event_name == 'issue_comment' }}
continue-on-error: true2. |
625a3d7 to
190e1ef
Compare
… /delta-test PR comment The Delta Spark UT pipeline (apache#12388) only runs per-PR when the `paths:` filter matches. A Velox/core/native change does not match it but can still break Delta offload, and there was no way for a contributor to force a run. A `run-delta-ci` label was considered and dropped: changing labels needs write/triage permission, so a PR author working from a fork -- exactly the person who needs it -- cannot apply one, and a label cannot be expressed as a `paths:` filter, so it would need a gate job on every PR. Use an `issue_comment` slash command instead, as velox_backend_ansi.yml already does for `/ansi-test`. Anyone can comment, so the PR author can opt their own PR in; authorised commenters are the PR author or anyone with write access. It is an ADDITIONAL `on:` key, so the `paths:` filter is untouched and PRs that do not ask for a run still cost zero jobs. `startsWith`, not `contains`, so quoting the command while discussing it does not spend ~11 job-hours. An `issue_comment` run is created against the default branch, so `env. DELTA_CHECKOUT_REF` resolves the PR's merge ref and every job checks it out; it is empty on every other event, which is exactly actions/checkout's default, so their behaviour is unchanged. Such a run is also not attached to the PR's Checks tab, so the gate job replies with a link. `update_baseline` remains reachable only from `workflow_dispatch`, so no comment can rewrite the committed baseline. Cache writes are scoped to the run's GITHUB_REF, which for `issue_comment` is the default branch rather than the PR. Since these jobs build and execute code from the PR, comment-triggered runs now restore the ccache / Maven / sbt caches but never save to them, so a PR cannot plant an entry that the nightly on main later restores. Workflow permissions are pinned to `contents: read`, with `pull-requests: write` granted only to the gate job, which never checks out PR code. Generated-by: GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses review feedback that the `author_association` allow-list is not equivalent to write access. On an ASF repo it is worse than imprecise: write access comes from Gitbox, not from GitHub org/collaborator membership, so `COLLABORATOR` matches nobody on apache/gluten and 16 of the 18 committers sampled from recent PR review comments report `CONTRIBUTOR`. An OWNER/MEMBER/COLLABORATOR list would therefore have rejected nearly every maintainer while accepting any member of the apache org. Drop the list rather than reword it. `/delta-test` now requires the commenter to be the PR author, which is precisely the gap this closes -- a fork author cannot label their own PR but can always comment on it -- and is exactly describable, so the workflow comment and README no longer overstate who may trigger a run. A maintainer who wants a run on someone else's PR asks the author to comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… run The whole pipeline hangs off `delta-test-requested` via `needs`, so a transient `gh pr comment` failure in its acknowledgement step would fail the gate job and skip every downstream job -- silently dropping an authorised run. The comment is informational only, so mark the step `continue-on-error: true`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Follow-up to rebasing onto apache#12820, which replaced actions/cache with Apache Stash. Two things the migration changes for this feature: 1. `permissions:` must grant `actions: read`. Declaring the block at all sets every unlisted scope to `none`, and the Stash restore action reads caches through the artifacts REST API (`gh api repos/.../actions/artifacts` and `gh run download`), which 403s without it. The other Stash-using workflows declare no `permissions:` block and inherit the repo default, so they never had to say this. Without this the Stash restores would fail on every event, not just on comment runs. 2. The "restore but never save" guards still apply, and matter more. A stash is an artifact named `<key>-<github.ref_name>`, restored by matching `head_branch` + `head_repository_id` -- the same branch scoping actions/cache has. On `issue_comment` that branch is the default branch, so a save from a run executing PR code would land on `main`, and Stash's `overwrite: true` default means it replaces the existing entry rather than merely competing with it. apache#12820 also moved the ccache to the key `ccache-centos7-release-default-${{ hashFiles('ep/build-velox/src/**') }}`, which velox_backend_x86.yml restores from as well, so an unguarded save would reach beyond this pipeline. Reads are deliberately left unguarded: a comment run restores main's stashes and is therefore no slower than any other run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
190e1ef to
8911f45
Compare
|
Rebased onto The conflict itself was mostly a simplification. #12820 already splits every cache into It also required one change beyond conflict resolution (separate commit,
all of which 403 without And I re-verified the guards still have a job to do under Stash. They do, more so than before. A stash is an artifact named
Two things make it sharper than under Verified: actionlint clean; authorization and |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
.github/workflows/delta_spark_ut.yml:72
- Using
on: issue_commentwill still create a workflow run for every newly created comment across the repo (including non-command PR chatter and issue comments), even though the jobs will mostly be skipped by the gate. This can significantly clutter the Actions UI and create noisy 'skipped' runs. Consider moving the comment listener into a tiny dedicated workflow that only triggers a real run when the command matches (e.g., via a dispatch to this workflow), so non-matching comments do not create runs for the heavy workflow.
issue_comment:
types: [created]
.github/workflows/delta_spark_ut.yml:210
- Using
on: issue_commentwill still create a workflow run for every newly created comment across the repo (including non-command PR chatter and issue comments), even though the jobs will mostly be skipped by the gate. This can significantly clutter the Actions UI and create noisy 'skipped' runs. Consider moving the comment listener into a tiny dedicated workflow that only triggers a real run when the command matches (e.g., via a dispatch to this workflow), so non-matching comments do not create runs for the heavy workflow.
if: >-
github.event_name != 'issue_comment' ||
(github.event.issue.pull_request &&
startsWith(github.event.comment.body, '/delta-test') &&
github.event.comment.user.login == github.event.issue.user.login)
.github/workflows/delta_spark_ut.yml:209
startsWith(..., '/delta-test')will also match unintended prefixes like/delta-test-fooor/delta-testers, which can accidentally trigger a ~2.5h run. Tighten the condition to require an exact command token (e.g.,/delta-testfollowed by end-of-string or whitespace/newline) while still avoidingcontains.
startsWith(github.event.comment.body, '/delta-test') &&
.github/workflows/delta_spark_ut.yml:334
- Unlike the ccache save step (which uses
always()for trusted runs), this Maven save will run only on success due to the defaultif: success()behavior. That means a trusted run that fails late (e.g., during tests) won’t save updated dependencies, potentially slowing subsequent reruns. If the intent is to keep dependency caching warm even when later steps fail, consider using analways()-style condition here as well (still excludingissue_comment).
- name: Save Maven repository to Apache Stash
# Trusted runs only -- see the Save Ccache step above.
if: ${{ github.event_name != 'issue_comment' }}
uses: apache/infrastructure-actions/stash/save@0ba14156c9f4c3cfbe4b0c9f36339ab0f8d81e53
Addresses review feedback: `startsWith(body, '/delta-test')` also matches
`/delta-test-arm` and `/delta-testers`, so a longer command that merely starts
the same way -- or a future `/delta-test-<something>` -- would spend ~11
job-hours on this pipeline by accident.
GHA expressions have no regex, so spell out the ways the token can legally end:
end-of-body, a space, or a newline. `fromJSON('"\r"')` / `fromJSON('"\n"')` is
the only way to express a control character in an expression; GitHub stores
comment bodies with CRLF and the API can deliver bare LF, so both are matched.
This keeps multi-line comments working (`/delta-test` on its own first line)
while rejecting the longer-prefix cases.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Surfacing the 4 suppressed comments from that review (they're 3 distinct points — the first is duplicated at two lines). One was a real bug and is fixed; two I'm declining, with evidence. 1.
|
|
🔄 ANSI mode analysis started by @felipepessoto. View run |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/delta_spark_ut.yml:223
delta-test-requestedruns on all events (because the job-levelifshort-circuits true whengithub.event_name != 'issue_comment'), but it always receivespull-requests: writevia job-level permissions. That grants an unnecessary write-scopedGITHUB_TOKENto scheduled and ordinarypull_requestruns even though the only step that uses the token is guarded toissue_comment.
Consider splitting this into (1) a no-permissions pass-through gate job for non-issue_comment events and (2) an issue_comment-only acknowledgement job that has pull-requests: write, so write permission is only present when it’s actually needed.
if: >-
github.event_name != 'issue_comment' ||
(github.event.issue.pull_request &&
(github.event.comment.body == '/delta-test' ||
startsWith(github.event.comment.body, '/delta-test ') ||
startsWith(github.event.comment.body, format('/delta-test{0}', fromJSON('"\r"'))) ||
startsWith(github.event.comment.body, format('/delta-test{0}', fromJSON('"\n"')))) &&
github.event.comment.user.login == github.event.issue.user.login)
runs-on: ubuntu-22.04
permissions:
pull-requests: write
steps:
Tested end-to-end before merge
Two notes on the method: #6 shows the #7 is valid despite my cancelling the run. The step is The run log also confirms empirically the branch-scoping argument made above: On a comment run both the head and base lookups mung to The one thing that cannot be tested pre-merge is the workflow file itself being read from |
ANSI Mode Test Analysis Report (Spark 4.1)Note Expression-level ANSI mode offload coverage analysis.
ANSI Offload suites: 70 tests, 11648 records | Other suites: 3331 tests ANSI OffloadOverview (ANSI Offload Expression Records)
Per-Suite Summary
Failure Cause Analysis (1 failures)
Other (28 failures)
|
Comments only; no logic change. Records why this trigger is stricter than the
repo's other comment triggers (velox_backend_ansi.yml's /ansi-test and take.yml
gate on nothing at all), and why the accurate maintainer check --
collaborators/{user}/permission -- is deliberately absent: maintainers can
already start the suite via workflow_dispatch, so the PR author is the only
party that had no way in, which is the gap apache#12743 describes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
EOF
|
🔄 ANSI mode analysis started by @felipepessoto. View run |
Drop the PR-author restriction so this trigger behaves like the repo's existing comment triggers: velox_backend_ansi.yml (`/ansi-test`, `/ansi-analyze`) and take.yml both gate on the command alone, with no author or permission check. Being the only workflow in the repo with an authorization gate was inconsistent, and it blocked the case where a reviewer wants the Delta suite run against a contributor's PR before merging. The exact-token match is deliberately kept rather than following `/ansi-test`'s `contains()`: matching a mention mid-sentence, or a longer command such as a future `/delta-test-arm`, was raised in review and would spend ~11 job-hours by accident. This also promotes the "restore but never save" cache guards from belt-and-braces to load bearing, since any user can now start a run that builds and executes the PR's code. They were already in place and are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Changed in This supersedes my earlier reasoning in this thread, where I argued for author-only on the grounds that maintainers already have One deliberate difference from So the gate is now: if: >-
github.event_name != 'issue_comment' ||
(github.event.issue.pull_request &&
(github.event.comment.body == '/delta-test' ||
startsWith(github.event.comment.body, '/delta-test ') ||
startsWith(github.event.comment.body, format('/delta-test{0}', fromJSON('"\r"'))) ||
startsWith(github.event.comment.body, format('/delta-test{0}', fromJSON('"\n"')))))Worth flagging for reviewers: this promotes the three "restore but never save" cache guards from belt-and-braces to load bearing, since any user can now start a run that builds and executes the PR's code — and the ccache key is shared with Truth table re-run, 12/12: accepted from any user (bare, with trailing text, and as the first line of a CRLF or LF multi-line comment); still rejecting |
|
🔄 ANSI mode analysis started by @felipepessoto. View run |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/workflows/delta_spark_ut.yml:221
- The
Acknowledge the requeststep posts a PR comment viagh pr comment, but this job-levelpermissionsblock only grantspull-requests: write. Because PR comments are created through the Issues API, the token may lack theissues: writescope and the acknowledgement can 403;continue-on-error: truewould hide that failure and leave contributors without the run link.
permissions:
pull-requests: write
.github/workflows/util/delta-spark-ut/README.md:114
- Minor grammar: “cannot opt their own PR in” is awkward; consider “cannot opt into their own PR”.
author — the person who most needs this — cannot opt their own PR in. It runs the
Apply the valid grammar suggestion from the suppressed review: use "opt into their own PR" consistently in the workflow comment and README. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressing the 2 suppressed comments from review 4978060920:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/workflows/delta_spark_ut.yml:221
gh pr commentposts an issue comment on the PR, but this job only grantspull-requests: write. With the workflow-levelpermissions:block,issuesdefaults tonone, so this step is likely to 403 and (because ofcontinue-on-error) you’ll silently lose the “reply with a link” UX that the PR description relies on.
permissions:
pull-requests: write
.github/workflows/delta_spark_ut.yml:216
- This
issue_commenttrigger currently allows any GitHub user who can comment on the PR to start a full ~2.5h, multi-job run that builds/executes PR code (only gated on the command token). That increases exposure to CI resource abuse and widens the blast radius compared to limiting the trigger to the PR author and/or trusted users.
if: >-
github.event_name != 'issue_comment' ||
(github.event.issue.pull_request &&
(github.event.comment.body == '/delta-test' ||
startsWith(github.event.comment.body, '/delta-test ') ||
Part of #12743.
What changes are proposed in this pull request?
The Delta Spark UT pipeline added in #12388 runs per-PR only when its
paths:filter matches (gluten-delta/**,backends-velox/src-delta*/**, and the pipeline's own files). A change to general Velox/core/native code does not match that filter but can still break Delta offload, and today there is no way for a contributor to force a run —workflow_dispatchneeds write access to this repo, and the nightly is not always soon enough.#12743 lists a
run-delta-cilabel as the candidate. That does not work for the people who need it most: changing labels requires write/triage permission, so a PR author working from a fork cannot opt their own PR in. A label also cannot be expressed as apaths:filter, so it would need a gate job runninggit diffon every PR.This PR adds a
/delta-testPR comment instead — the same mechanismvelox_backend_ansi.ymlalready uses for/ansi-test:velox_backend_ansi.yml's/ansi-testandtake.yml), so a reviewer can also run it against someone else's PR. Noauthor_associationcheck: it does not track write access on an ASF repo (write comes from Gitbox), so anOWNER/MEMBER/COLLABORATORallow-list would reject 16 of the 18apache/glutenmaintainers sampled while accepting anyapacheorg member.on:key, not a change to thepull_requesttrigger, so thepaths:filter is untouched and PRs that don't ask for a run still cost zero jobs — no per-PR gate job.startsWith, notcontains, so quoting the command while discussing it doesn't spend ~11 job-hours.How it works
An
issue_commentrun is created against the default branch, sogithub.refpoints atmain, not the PR. One workflow-levelenvresolves what to check out:Every job then checks out
ref: ${{ env.DELTA_CHECKOUT_REF }}. It is empty on every other event, which is exactly whatactions/checkoutdoes by default, sopull_request,scheduleandworkflow_dispatchbehave precisely as before. Using the PR's merge ref (whatpull_requestitself tests) avoids any API call, job outputs or SHA plumbing.A small
delta-test-requestedjob holds the authorisationif:and posts a link to the run — anissue_commentrun belongs to the default branch, so GitHub cannot attach it to the PR's Checks tab, and without the link the contributor sees nothing happen for ~2.5 h. It is a separate job so thatpull-requests: writeis never granted to a job that builds and runs the PR's code. Everything else hangs off it vianeeds, so a comment that isn't authorised skips the whole pipeline.update_baselinestays reachable only fromworkflow_dispatch, so no comment can rewrite the committed baseline.Cache scoping (why the cache steps changed)
A cache write is scoped to the run's
GITHUB_REF. Forpull_requestthat isrefs/pull/N/merge, isolated to the PR — but forissue_commentit is the default branch, shared with every trusted run. Since these jobs compile and execute the PR's code, saving there would let a PR plant a ccache/Maven/sbt entry that the nightly onmainlater restores (the prefixedrestore-keysmake it reachable), i.e. attacker-controlled compiler output in a trusted build.So the three caches are split into restore + save, and comment-triggered runs restore but never save. They still read the shared caches, so they are no slower; they just don't contribute back. Workflow permissions are additionally pinned to
contents: read.Files
.github/workflows/delta_spark_ut.ymlissue_commenttrigger,DELTA_CHECKOUT_REF,delta-test-requestedgate job,ref:on the 4 checkouts, cache restore/save split,permissions, PR-number concurrency key..github/workflows/util/delta-spark-ut/README.md/delta-testunder "When it runs".58 functional lines (the rest of the diff is comments and docs).
How was this patch tested?
This change is CI, and the trigger can only be exercised once the workflow is on the default branch. Verified statically instead:
delta_spark_ut.yml.pull_request/schedule/workflow_dispatchall pass through; a comment on an issue (not a PR), the command quoted mid-sentence, a longer command (/delta-test-arm,/delta-testers), and ordinary PR chatter are all rejected;/delta-testis accepted from any user, bare, with trailing text, or as the first line of a multi-line comment (CRLF and LF).DELTA_CHECKOUT_REFresolves to''onpull_request/schedule/workflow_dispatchand torefs/pull/N/mergeonissue_comment; confirmed againstactions/checkout's source that an emptyrefreproduces its default (github.context.ref+github.context.sha), so existing events are unaffected.if:).saveis gated onissue_comment, everyrestoreis not.github.event.comment.*/github.event.issue.*) is interpolated into anyrun:block; it is passed viaenv:only.actions/checkout'sassertSafePrCheckoutguard only fires onpull_request_target/workflow_run, soissue_commentneeds noallow-unsafe-pr-checkout.Once merged,
/delta-teston any PR is the end-to-end test.Was this patch authored or co-authored using generative AI tooling?
Generated-by: GitHub Copilot CLI