felipepessoto commented on PR #12781: URL: https://github.com/apache/gluten/pull/12781#issuecomment-5347140672
Surfacing the 4 suppressed comments from [that review](https://github.com/apache/gluten/pull/12781#pullrequestreview-4974932067) (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. `startsWith` matches `/delta-test-foo` / `/delta-testers` — valid, fixed in `dfe2e66` Correct, and the forward-compat case is the one that worried me: add a `/delta-test-arm` later and it would fire *this* ~11 job-hour pipeline too. GHA expressions have no regex, so the token boundary is spelled out: ```yaml (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"')))) ``` `fromJSON` is the only way to write a control character in an expression. Both CR and LF are matched because GitHub stores comment bodies CRLF while the API can deliver bare LF — so `/delta-test` on its own first line of a multi-line comment still works. Truth table re-run, 13/13, including the new rejects (`/delta-test-arm`, `/delta-testers`) and accepts (CRLF and LF multi-line). ### 2. Maven save runs only on success, unlike the ccache save — declining, it's not a change this PR makes This is upstream #12820's behaviour, preserved exactly. Per the [expressions docs](https://docs.github.com/en/actions/reference/workflows-and-actions/expressions#status-check-functions): > A default status check of `success()` is applied **unless you include one of these functions**. `if: ${{ github.event_name != 'issue_comment' }}` contains no status-check function, so it evaluates as `success() && github.event_name != 'issue_comment'` — the same success-gating as #12820's no-`if:` version, plus my restriction. The ccache step differs only because #12820 wrote `always()` there and I kept it (`always() && …`), and the sbt step likewise keeps its explicit `success() && matrix.shard == 0`. So the asymmetry is pre-existing and deliberate on #12820's side. Changing the Maven save to `always()` would alter behaviour unrelated to this PR (and would stash a partial `.m2` from a failed build), so it belongs in its own PR if wanted. ### 3. `on: issue_comment` creates a skipped run for every comment repo-wide — real, but declining the split The observation is correct and I measured it. Of the last 100 `velox_backend_ansi.yml` runs (the existing `/ansi-test` listener), **100/100 are `issue_comment` / `skipped`** — its real runs are completely buried. So this is a genuine cost, not hypothetical. I'm still not splitting it, for one specific reason: the suggested "listener dispatches the heavy workflow" design **silently disables every security guard in this PR**. `workflow_dispatch` from `GITHUB_TOKEN` does work ([documented exception](https://docs.github.com/en/actions/using-workflows/triggering-a-workflow#triggering-a-workflow-from-a-workflow) to the recursion rule), but the dispatched run arrives with `github.event_name == 'workflow_dispatch'`, so: - the three `github.event_name != 'issue_comment'` stash guards stop firing, and the run — still executing fork code — starts writing `main`-scoped stashes, including the ccache key shared with `velox_backend_x86.yml`; - `update_baseline`, currently gated on `github.event_name == 'workflow_dispatch'`, becomes reachable from a comment; - the untrusted-ness has to be re-encoded as an input and re-honoured in ~5 places, where forgetting one fails open and silently. Trading a tidier Actions list for a set of fail-open guards isn't a good deal at ~11 job-hours and a shared ccache. The noise is also consistent with the existing `/ansi-test` precedent, so this PR doesn't make the situation worse than the pattern already in the repo. If maintainers do want the run list cleaned up, the safe version is to keep the guards keyed on an explicit `untrusted` input rather than on `event_name` — happy to do that as a follow-up, and it would fix `/ansi-test`'s noise at the same time. It seemed wrong to fold that into this PR. -- 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]
