Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions .github/workflows/github-action-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,76 @@ on:
- 'ci-enable/**'
- 'main'
pull_request:
types: [opened, synchronize, reopened]
issue_comment:
types: [created]
Comment on lines +30 to +31
Copy link
Contributor

@Yicong-Huang Yicong-Huang Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Ma77Ball Ma77Ball Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this is the only plausible workaround I could think of for this comment (#4278 (comment)). The problem is that either:

  1. We have a label that allows runs to pass through editing workflows (causing potential security issues)
  2. 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)
  3. We only allow committers to make changes to workflows
  4. 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.

Copy link
Contributor

@Yicong-Huang Yicong-Huang Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I will add back the label method and update the corresponding issue with a diagram of the current GitHub action flow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned, we might need to modify the auto labeler logic first. do you want to do it as well?

workflow_dispatch:

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}
Comment on lines 34 to 36
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

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
Comment on lines +59 to +60
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
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)
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)

Copilot uses AI. Check for mistakes.
if [ "$CHANGED" -gt "0" ]; then
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::This PR modifies the build workflow. A committer must comment '/safe-to-test' on this PR to approve it."
exit 1
fi
fi

frontend:
needs: check-permissions
if: always() && needs.check-permissions.result == 'success'
runs-on: ${{ matrix.os }}
strategy:
matrix:
Expand All @@ -50,6 +112,8 @@ jobs:
steps:
- name: Checkout Texera
uses: actions/checkout@v5
with:
ref: ${{ needs.check-permissions.outputs.sha }}
- name: Setup node
uses: actions/setup-node@v5
with:
Expand Down Expand Up @@ -81,6 +145,8 @@ jobs:
run: yarn --cwd frontend run build:ci

scala:
needs: check-permissions
if: always() && needs.check-permissions.result == 'success'
strategy:
matrix:
os: [ ubuntu-22.04 ]
Expand All @@ -106,6 +172,8 @@ jobs:
steps:
- name: Checkout
uses: actions/checkout@v5
with:
ref: ${{ needs.check-permissions.outputs.sha }}
- name: Setup JDK
uses: actions/setup-java@v5
with:
Expand Down Expand Up @@ -145,6 +213,8 @@ jobs:
run: sbt test

python:
needs: check-permissions
if: always() && needs.check-permissions.result == 'success'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: shall we use needs.has_permission == true?

strategy:
matrix:
os: [ ubuntu-latest ]
Expand All @@ -153,6 +223,8 @@ jobs:
steps:
- name: Checkout Texera
uses: actions/checkout@v5
with:
ref: ${{ needs.check-permissions.outputs.sha }}
- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v6
with:
Expand Down
2 changes: 1 addition & 1 deletion bin/k8s/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ texera:
# Container image registry and tag for all Texera services
# Override these to use a different registry or version
imageRegistry: ghcr.io/apache
imageTag: latest
imageTag: nightly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this change related?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that is related to a different pr. @aicam can remove it.


global:
# Required by Bitnami sub-charts (postgresql, minio) to allow custom images
Expand Down
Loading