Skip to content

refactor(ci): auto-run workflow#4278

Closed
Ma77Ball wants to merge 9 commits intoapache:mainfrom
Ma77Ball:ci/AutomaticWorkflowExecution
Closed

refactor(ci): auto-run workflow#4278
Ma77Ball wants to merge 9 commits intoapache:mainfrom
Ma77Ball:ci/AutomaticWorkflowExecution

Conversation

@Ma77Ball
Copy link
Contributor

@Ma77Ball Ma77Ball commented Mar 10, 2026

What changes were proposed in this PR?

The build workflow now runs automatically for all PRs. All three build jobs (frontend, scala, python) trigger directly on push, pull_request, and workflow_dispatch events, making CI faster and easier to reason about for all contributors.

Any related issues, documentation, discussions?

Closes: #4290

How was this PR tested?

Verified that all three build jobs (frontend, scala, python) trigger automatically on pull request open, push, and synchronize events.

Was this PR authored or co-authored using generative AI tooling?

Resolved comments with Claude Sonnet 4.6

@github-actions github-actions bot added the ci changes related to CI label Mar 10, 2026
@Ma77Ball Ma77Ball changed the title ci: enforce label check for fork workflow changes; trigger on opened… build: enforce label check for fork workflow changes; trigger on opened… Mar 10, 2026
@Ma77Ball Ma77Ball changed the title build: enforce label check for fork workflow changes; trigger on opened… ci: enforce label check for fork workflow changes; trigger on opened… Mar 10, 2026
@Yicong-Huang
Copy link
Contributor

The PR title is kind of broken, it will be nice to make it concise.

@Ma77Ball Ma77Ball changed the title ci: enforce label check for fork workflow changes; trigger on opened… ci: enforce label check for fork workflow changes Mar 10, 2026
@Ma77Ball Ma77Ball changed the title ci: enforce label check for fork workflow changes ci: auto-run workflow for fork PRs Mar 10, 2026
@Ma77Ball Ma77Ball force-pushed the ci/AutomaticWorkflowExecution branch from bfcf501 to 856b5cb Compare March 11, 2026 12:26
@Ma77Ball
Copy link
Contributor Author

@chenlica or @Yicong-Huang, please rerun the workflow. I removed the safe-to-test label.

@Yicong-Huang
Copy link
Contributor

I'm not sure I follow how this works now. The pr description sounds like it require a committer to add the safe to run label manually. It's still an additional step. It will be good if we can trigger CIs on PRs from anyone/ any fork, no matter if that is from committer or not, and no need for committer to manually approve it.

@Ma77Ball
Copy link
Contributor Author

@Yicong-Huang, I've updated the PR description and removed the safe-to-test label due to a potential security concern.

The original intent was to automatically run the GitHub workflow on any PR that didn't modify the workflow itself, with the safe-to-test label serving as a committer override for PRs that did. However, this approach places too much implicit trust in PRs that both run and modify the workflow, which could introduce vulnerabilities.

The updated design takes a simpler, safer approach: if the GitHub workflow has been modified, it will not run automatically and will require explicit committer review and approval before execution.

@Yicong-Huang
Copy link
Contributor

However, this approach places too much implicit trust in PRs that both run and modify the workflow, which could introduce vulnerabilities.

make sense. since we are running actions in our main repo, it is a good idea to let committer approve altered action/workflows.

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.

How did you verify though?

@Yicong-Huang
Copy link
Contributor

Screenshot 2026-03-11 at 10 31 07 PM also currently, how do I (with committer privilege), approve the modified actions to execute? the `check-permissions` step is failing.

@Ma77Ball
Copy link
Contributor Author

I checked by raising a PR in my local fork, and it worked. Let me look and submit a fix.

@Yicong-Huang
Copy link
Contributor

I checked by raising a PR in my local fork, and it worked. Let me look and submit a fix.

sg! It will be nice to include the link to your test PR in your fork, in the PR description. That way we have some reference of what's being tested. Thanks!

@Ma77Ball
Copy link
Contributor Author

If you rerun the build workflow, it should work, as I have edited the workflow; it will fail until a committer initiates the GitHub workflow. There should be a button to rerun the workflow, or you might need to go into the actions tab and run the build workflow on this pr manually.

@Ma77Ball
Copy link
Contributor Author

@Yicong-Huang, the current workflow fails because I triggered it and edited the build workflow.
image

After you run the workflow, it should pass.

@Yicong-Huang
Copy link
Contributor

Yicong-Huang commented Mar 12, 2026

After you run the workflow, it should pass.

Hmm, how do I run it? I don't think there is a button.. I could manually go to actions page to dispatch it, but it would be very inconvenient.

@Ma77Ball
Copy link
Contributor Author

I updated the code so that four events can trigger the workflow: a push to main or ci-enable/**, a pull request being opened/updated, a /safe-to-test comment on a PR, or a manual workflow_dispatch. The check-permissions job runs first in all cases. For push and dispatch, it passes straight through. For a pull request, it checks if the build workflow file was modified; if not, it passes; if yes, it checks whether the actor is a committer and fails with an error message directing them to comment /safe-to-test if not. For a /safe-to-test comment, it verifies the commenter is a committer and fails if not. Once check-permissions passes, the frontend, Scala, and Python jobs all run against the correct PR.
FYI: There is no way I know of to keep the run workflow button and build an auto check function in GitHub workflows. Now, committers for PRs that edit workflows can just comment /safe-to-test.

@Ma77Ball
Copy link
Contributor Author

I do not think the safe-to-test feature will work till merged into main. @Yicong-Huang, please try to re-run the workflow to test.

@aicam
Copy link
Contributor

aicam commented Mar 12, 2026

/safe-to-test

@aicam aicam changed the title ci: auto-run workflow for fork PRs refactor(ci): auto-run workflow for fork PRs Mar 12, 2026
@Ma77Ball
Copy link
Contributor Author

@Yicong-Huang This is a chicken-and-egg problem: a committer needs to raise the PR, or the workflows won't run. I had @aicam raise a pr of this branch. Please refer to this pr (#4287) from now on.

@Ma77Ball
Copy link
Contributor Author

After this is merged, commenting /safe-to-test will trigger the workflow for future contributors editing the build workflow.

@Ma77Ball Ma77Ball closed this Mar 12, 2026
@Ma77Ball Ma77Ball reopened this Mar 13, 2026
@Ma77Ball Ma77Ball changed the title refactor(ci): auto-run workflow for fork PRs refactor(ci): auto-run workflow Mar 13, 2026
@Ma77Ball
Copy link
Contributor Author

Ma77Ball commented Mar 13, 2026

@Yicong-Huang, please review this. This is the first PR to handle automatic workflow execution, so committers don't have to approve every run.

@Yicong-Huang
Copy link
Contributor

thanks will check later today

@Yicong-Huang
Copy link
Contributor

Yicong-Huang commented Mar 13, 2026

Ok. First, it might be better to open a new Pr instead of changing the existing one, as the context can be very confusing now.

So for the new problem, that we want to allow all PRs (even from forks of non-committers) can trigger CIs without explicit approval from committers. Can you explain how your change fix the issue? To me the only useful line change is to add types. What's the logic behind the fix?

For the context, all PRs from committers could automatically trigger CIs today. I don't understanding why adding more event types can help non committers trigger CI without approval?

@Ma77Ball Ma77Ball closed this Mar 13, 2026
@Ma77Ball Ma77Ball deleted the ci/AutomaticWorkflowExecution branch March 13, 2026 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci changes related to CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Auto-run CI build workflow for all PRs

3 participants