Copilot commented on code in PR #4287:
URL: https://github.com/apache/texera/pull/4287#discussion_r2927513311
##########
.github/workflows/github-action-build.yml:
##########
@@ -26,14 +26,76 @@ on:
- 'ci-enable/**'
- 'main'
pull_request:
+ types: [opened, synchronize, reopened]
+ issue_comment:
+ types: [created]
workflow_dispatch:
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}
Review Comment:
The concurrency group is based on `github.ref`. For `issue_comment` events,
`github.ref` resolves to the default branch ref (typically `refs/heads/main`),
so all `/safe-to-test` runs across different PRs will share the same
concurrency group and end up serialized (and potentially blocking each other).
Consider including the PR number and/or the resolved SHA in the
`concurrency.group` when the event is `issue_comment` so independent PR test
runs don't contend with each other.
##########
.github/workflows/github-action-build.yml:
##########
@@ -26,14 +26,76 @@ on:
- 'ci-enable/**'
- 'main'
pull_request:
+ types: [opened, synchronize, reopened]
+ issue_comment:
+ types: [created]
workflow_dispatch:
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}
jobs:
+ check-permissions:
+ if: |
+ github.event_name == 'push' ||
+ github.event_name == 'workflow_dispatch' ||
+ github.event_name == 'pull_request' ||
+ (
+ github.event_name == 'issue_comment' &&
+ github.event.issue.pull_request != null &&
+ contains(github.event.comment.body, '/safe-to-test')
+ )
+ runs-on: ubuntu-latest
+ outputs:
+ sha: ${{ steps.resolve.outputs.sha }}
+ steps:
+ - name: Resolve SHA
+ id: resolve
+ env:
+ GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+ run: |
+ if [ "${{ github.event_name }}" == "issue_comment" ]; then
+ SHA=$(gh api repos/${{ github.repository }}/pulls/${{
github.event.issue.number }} --jq '.head.sha')
+ echo "sha=$SHA" >> $GITHUB_OUTPUT
Review Comment:
For `/safe-to-test` runs you resolve and checkout the PR *head* SHA
(`.head.sha`). This means the build/tests run on the unmerged PR tip, which can
diverge from what will actually land (merge conflicts / base-branch
interactions), unlike the normal `pull_request` workflow which runs on the
merge commit. Consider resolving/checking out the PR merge ref/merge SHA (e.g.,
`refs/pull/<num>/merge` or the PR's merge SHA) so the `/safe-to-test` run
matches the `pull_request` behavior.
```suggestion
REF="refs/pull/${{ github.event.issue.number }}/merge"
echo "sha=$REF" >> $GITHUB_OUTPUT
```
##########
.github/workflows/github-action-build.yml:
##########
@@ -26,14 +26,76 @@ on:
- 'ci-enable/**'
- 'main'
pull_request:
+ types: [opened, synchronize, reopened]
+ issue_comment:
+ types: [created]
workflow_dispatch:
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}
jobs:
+ check-permissions:
+ if: |
+ github.event_name == 'push' ||
+ github.event_name == 'workflow_dispatch' ||
+ github.event_name == 'pull_request' ||
+ (
+ github.event_name == 'issue_comment' &&
+ github.event.issue.pull_request != null &&
+ contains(github.event.comment.body, '/safe-to-test')
+ )
+ runs-on: ubuntu-latest
+ outputs:
+ sha: ${{ steps.resolve.outputs.sha }}
+ steps:
+ - name: Resolve SHA
+ id: resolve
+ env:
+ GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+ run: |
+ if [ "${{ github.event_name }}" == "issue_comment" ]; then
+ SHA=$(gh api repos/${{ github.repository }}/pulls/${{
github.event.issue.number }} --jq '.head.sha')
+ echo "sha=$SHA" >> $GITHUB_OUTPUT
+ else
+ echo "sha=${{ github.sha }}" >> $GITHUB_OUTPUT
+ fi
+
+ - name: Checkout
+ uses: actions/checkout@v5
+ with:
+ ref: ${{ steps.resolve.outputs.sha }}
+ fetch-depth: 0
+
+ - name: Check committer permission for /safe-to-test
+ if: github.event_name == 'issue_comment'
+ env:
+ GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+ run: |
+ PERMISSION=$(gh api repos/${{ github.repository }}/collaborators/${{
github.actor }}/permission --jq '.permission' 2>/dev/null || echo "none")
+ if [[ "$PERMISSION" != "admin" && "$PERMISSION" != "maintain" &&
"$PERMISSION" != "write" ]]; then
+ echo "::error::Only committers can approve /safe-to-test."
+ exit 1
+ fi
+
+ - name: Check if build workflow was modified by non-committer
+ if: github.event_name == 'pull_request'
+ env:
+ GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+ run: |
+ CHANGED=$(git diff --name-only origin/${{ github.base_ref }}...HEAD
| grep -c "^\.github/workflows/github-action-build\.yml$" || true)
Review Comment:
This `git diff origin/${{ github.base_ref }}...HEAD` can fail because
`origin/${{ github.base_ref }}` may not exist in the checkout (even with
`fetch-depth: 0`), and any `git diff` failure will abort the step (only `grep`
is guarded with `|| true`). Consider diffing by explicit SHAs from the event
payload (e.g., PR base/head SHAs) and/or fetching the base ref before diffing,
and ensure the `git diff` invocation itself can't hard-fail the workflow due to
a missing ref.
```suggestion
CHANGED=$((git diff --name-only origin/${{ github.base_ref
}}...HEAD 2>/dev/null || true) | grep -c
"^\.github/workflows/github-action-build\.yml$" || true)
```
--
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]
