LuciferYang commented on PR #12906:
URL: https://github.com/apache/gluten/pull/12906#issuecomment-5436585104
Good catch, thanks. Fixed in this PR too, at
`.github/workflows/velox_backend_ansi.yml`, and the title now covers both
workflows.
I took the second of your two options for both files, for the reason you
gave: a unique group fully isolates the unrelated comment, while conditioning
`cancel-in-progress` only downgrades the damage. Runs in one group never
overlap, so with cancellation off the comment run does not proceed alongside
the ANSI run, it sits `pending` until that run finishes and then skips anyway,
holding the group's single pending slot in the meantime. I had it that way
first and replaced it.
The suffix is appended only when the comment is not a command, so for
`workflow_dispatch`, for `pull_request` and for a real `/ansi-test` or
`/ansi-analyze` the group name stays byte-identical to what main has today.
That matters beyond tidiness: `pull_request` runs use the workflow file from
the PR head while `issue_comment` runs always use the default branch's, so
renaming the group for every trigger would have put PRs that are already open
into a window where a push run and a command run sit in different groups and
stop superseding each other. Appending nothing in those cases avoids it.
The negated condition is `check-comment`'s `if` copied verbatim, so the
grouping and the gate cannot disagree. That includes its `contains` test, which
means a comment merely mentioning `/ansi-test` counts as a command in both
places. Whether that test should be tightened to a first-token match, the way
`delta-test-requested` does it, seemed like a separate question from this one,
so I left it alone.
Three properties are checkable without running anything, and I verified them
for both files by parsing the YAML rather than reading it: `cancel-in-progress`
is still `true`, removing the appended `${{ }}` segment leaves a group string
equal to main's, and the text inside `!( ... )` equals the gate job's `if`
after collapsing whitespace.
You are right that neither fix can be exercised before merging.
`issue_comment` only ever runs the default branch's copy of a workflow, which
is also why your reproduction ran main's unfixed version.
--
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]
