refactor(ci): auto-run workflow for fork PRs#4287
refactor(ci): auto-run workflow for fork PRs#4287aicam wants to merge 18 commits intoapache:mainfrom
Conversation
…exera into ci/AutomaticWorkflowExecution
… that modify the build workflow
Ci/automatic workflow execution
aicam
changed the title
|
Prior discussion of review process found at pr: #4278 |
|
@Yicong-Huang, please review. I had to have a committer raise the PR, or else it ended up being a chicken-and-egg problem. |
|
Here is also an action I ran on my forked repo with the changes in the main working branch ( |
Yicong-Huang
changed the title
| # Override these to use a different registry or version | ||
| imageRegistry: ghcr.io/apache | ||
| imageTag: latest | ||
| imageTag: nightly |
There was a problem hiding this comment.
is this change related?
There was a problem hiding this comment.
No, that is related to a different pr. @aicam can remove it.
| issue_comment: | ||
| types: [created] |
There was a problem hiding this comment.
this would trigger CI actions every time some one posts a comment right? I think it will be a waste of resources. github action is free with a cap.
There was a problem hiding this comment.
Yes, but this is the only plausible workaround I could think of for this comment (#4278 (comment)). The problem is that either:
- We have a label that allows runs to pass through editing workflows (causing potential security issues)
- We have committers run workflows manually from the actions tab to bypass security checks (not sure if this is even possible as forked prs I don't think show up on the github action page and there is no guarrantee the pr will pick up the run)
- We only allow committers to make changes to workflows
- We allow committers to run the workflow from a comment
Committers can't just rerun the failed workflow, as it will trigger a GitHub run with the pull request owners' credentials.
There was a problem hiding this comment.
how about this:
- we can use labels to satisfy this need, only committers would be able to apply labels.
- Currently, all labels are assigned automatically by CI (auto labeler). we could modify it to respect the labels manually added by committers.
- label changes are less frequent than comments changes
Does that satisfy your need?
There was a problem hiding this comment.
Sounds good, I will add back the label method and update the corresponding issue with a diagram of the current GitHub action flow.
There was a problem hiding this comment.
As mentioned, we might need to modify the auto labeler logic first. do you want to do it as well?
There was a problem hiding this comment.
Pull request overview
This PR adjusts default deployment and CI behavior by switching the Helm chart’s default Texera image tag to a nightly build and enhancing the GitHub Actions build workflow to support a /safe-to-test approval flow via PR comments.
Changes:
- Update Helm values to use
nightlyas the default Texera image tag. - Add
issue_commenttriggering and acheck-permissionsgate to the build workflow, and wire all build jobs to use the resolved SHA.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
bin/k8s/values.yaml |
Switches the default Texera image tag from latest to nightly. |
.github/workflows/github-action-build.yml |
Adds /safe-to-test comment-triggered runs and a permissions gate; ensures jobs checkout a resolved SHA. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} |
There was a problem hiding this 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.
| SHA=$(gh api repos/${{ github.repository }}/pulls/${{ github.event.issue.number }} --jq '.head.sha') | ||
| echo "sha=$SHA" >> $GITHUB_OUTPUT |
There was a problem hiding this 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.
| SHA=$(gh api repos/${{ github.repository }}/pulls/${{ github.event.issue.number }} --jq '.head.sha') | |
| echo "sha=$SHA" >> $GITHUB_OUTPUT | |
| REF="refs/pull/${{ github.event.issue.number }}/merge" | |
| echo "sha=$REF" >> $GITHUB_OUTPUT |
| 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) |
There was a problem hiding this 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.
| CHANGED=$(git diff --name-only origin/${{ github.base_ref }}...HEAD | grep -c "^\.github/workflows/github-action-build\.yml$" || true) | |
| CHANGED=$((git diff --name-only origin/${{ github.base_ref }}...HEAD 2>/dev/null || true) | grep -c "^\.github/workflows/github-action-build\.yml$" || true) |
|
|
||
| python: | ||
| needs: check-permissions | ||
| if: always() && needs.check-permissions.result == 'success' |
There was a problem hiding this comment.
nit: shall we use needs.has_permission == true?
This PR is the fork of #4278
What changes were proposed in this PR?
The build workflow now enforces a stricter permission model for fork PRs that modify the workflow file. A check-permissions job is added as a required predecessor to all build jobs (frontend, scala, python). If a PR modifies .github/workflows/github-action-build.yml and the author is not a committer (i.e., does not have write, maintain, or admin permissions), the workflow fails with an error requiring a committer to review and manually re-run it. PRs that do not modify the workflow file continue to run automatically as before.
Any related issues, documentation, discussions?
Closes: #4279
How was this PR tested?
Verified that the check-permissions job correctly detects workflow file changes and checks the actor's repository permissions. Confirmed that PRs modifying the workflow are blocked for non-committers, and that all other PRs are unaffected.
Was this PR authored or co-authored using generative AI tooling?
Resolved comments with Claude Sonnet 4.6