Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modifies the shared Go Makefile’s help target phony declaration in go.Makefile.
Changes:
- Updates the
.PHONYdeclaration for thehelptarget (currently to a mismatched name).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ##@ General | ||
|
|
||
| .PHONY: help | ||
| .PHONY: helppp |
Signed-off-by: Kevin Su <pingsutw@apache.org>
…s.DEPOT_PROJECT_ID Forks don't receive repo vars on pull_request events; pull_request_target runs in the base-repo context where vars are available. Explicitly checks out the PR head SHA to actually build PR code, and forces PUSH_IMAGES=false for fork PRs so secrets like FLYTE_BOT_PAT remain unreachable. Signed-off-by: Kevin Su <pingsutw@apache.org>
pingsutw
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Updates build automation by modifying the Go makefile’s help target declaration and changing the Flyte binary CI workflow to run in pull_request_target context (with PR-head checkout) while restricting image pushing to base-repo branches/PRs.
Changes:
- Changed
.PHONYdeclaration ingo.Makefile(help target). - Switched CI trigger from
pull_requesttopull_request_targetand added PR-head SHA checkout. - Updated
PUSH_IMAGESlogic to prevent image pushes from fork PRs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| go.Makefile | Adjusts .PHONY declaration for the help target. |
| .github/workflows/flyte-binary-v2.yml | Changes PR trigger to pull_request_target, checks out PR head SHA, and tightens PUSH_IMAGES for forks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ##@ General | ||
|
|
||
| .PHONY: help | ||
| .PHONY: helppp |
Comment on lines
+11
to
+17
| # SECURITY: pull_request_target runs in the BASE repo context so repo | ||
| # variables (vars.*) are available for fork PRs. The tradeoff is that this | ||
| # job checks out and builds the PR's head ref — i.e. fork-controlled code — | ||
| # while having access to those vars. We do NOT expose repo secrets here: | ||
| # PUSH_IMAGES is forced false for fork PRs below, so FLYTE_BOT_PAT login and | ||
| # any image push only run for branches in the base repo. | ||
| pull_request_target: |
Comment on lines
22
to
+26
| env: | ||
| DEPOT_PROJECT_ID: ${{ vars.DEPOT_PROJECT_ID }} | ||
| # Push images on push to main, manual dispatch, OR on a PR labeled | ||
| # `test-push-image` (apply the label to a PR to push test images). | ||
| PUSH_IMAGES: ${{ github.event_name == 'push' || github.event_name == 'workflow_dispatch' || (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'test-push-image')) }} | ||
| # `test-push-image` from a branch in the base repo (never from forks). | ||
| PUSH_IMAGES: ${{ github.event_name == 'push' || github.event_name == 'workflow_dispatch' || (github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.full_name == github.repository && contains(github.event.pull_request.labels.*.name, 'test-push-image')) }} |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Kevin Su <pingsutw@gmail.com>
The metadata-action 'enable' expression checked github.event_name == 'pull_request', which (a) never matches pull_request_target runs and (b) was already dead even pre-switch since github.ref is never refs/heads/master on pull_request events. Update to push-to-main to match the other image's nightly rule (push to v2 -> nightly for devbox-bundled). Signed-off-by: Kevin Su <pingsutw@apache.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the Flyte single-binary v2 image build workflow so fork-based PRs can run Depot builds by switching from pull_request to pull_request_target, while gating any image-push behavior to base-repo PRs only.
Changes:
- Switch workflow trigger from
pull_requesttopull_request_targetto allow access tovars.DEPOT_PROJECT_IDfor fork PRs. - Ensure PR code is built by checking out
github.event.pull_request.head.sha(fallbackgithub.sha) in all jobs. - Tighten image-push gating logic (
PUSH_IMAGES) and add minimal job permissions fortest-bootstrap.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
9
to
13
| branches: | ||
| - main | ||
| pull_request: | ||
| pull_request_target: | ||
| branches: | ||
| - main |
Comment on lines
18
to
+20
| # Push images on push to main, manual dispatch, OR on a PR labeled | ||
| # `test-push-image` (apply the label to a PR to push test images). | ||
| PUSH_IMAGES: ${{ github.event_name == 'push' || github.event_name == 'workflow_dispatch' || (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'test-push-image')) }} | ||
| # `test-push-image` from a branch in the base repo (never from forks). | ||
| PUSH_IMAGES: ${{ github.event_name == 'push' || github.event_name == 'workflow_dispatch' || (github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.full_name == github.repository && contains(github.event.pull_request.labels.*.name, 'test-push-image')) }} |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Kevin Su <pingsutw@gmail.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Kevin Su <pingsutw@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the Flyte single-binary v2 build workflow so that fork-based PRs can access vars.DEPOT_PROJECT_ID and run Depot-backed build verification, while preventing fork PRs from pushing images.
Changes:
- Switched the workflow trigger from
pull_requesttopull_request_targetto allow fork PRs to read repo variables. - Updated
actions/checkout@v4steps to check out the PR head SHA (instead of the base ref) underpull_request_target. - Tightened
PUSH_IMAGESgating logic and adjusted the nightly tagging enablement condition.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
7
to
13
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
| pull_request: | ||
| pull_request_target: | ||
| branches: | ||
| - main |
Comment on lines
156
to
+159
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha || github.sha }} |
Comment on lines
28
to
+31
| - uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha || github.sha }} | ||
| persist-credentials: false |
AdilFayyaz
approved these changes
Apr 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The
Build & Push Flyte Single Binary Images v2workflow validates and usesvars.DEPOT_PROJECT_IDto drive Depot builds. GitHub does not pass repository variables (or secrets) to workflows triggered bypull_requestevents from forks. As a result, every PR opened from a fork (e.g.pingsutw/flyte) failed theValidate Depot project idstep with:Branches in
flyteorg/flyteworked fine; only fork PRs failed. We want fork PRs to run the same build verification as in-repo branches.What changed
pull_request→pull_request_targetfor this workflow.pull_request_targetruns in the base repo context, sovars.DEPOT_PROJECT_ID(and any non-secret config) is available even for fork PRs.actions/checkout@v4steps now explicitly check outgithub.event.pull_request.head.sha || github.sha. By defaultpull_request_targetchecks out the base ref, which would test the wrong code; this makes the workflow actually build the PR's contents.PUSH_IMAGEStightened: now requirespull_request_targetandhead.repo.full_name == github.repositoryand thetest-push-imagelabel. Fork PRs can never trigger an image push, sosecrets.FLYTE_BOT_PATis never reachable from fork-controlled code.on:documenting the tradeoff.Security considerations
pull_request_target+ checking out the PR head means fork-authored code (Dockerfile, Makefile, build scripts) executes with access to whatever the base-repo job exposes. Mitigations applied:vars.DEPOT_PROJECT_ID, which is a public Depot project identifier (not a credential). Auth to Depot uses OIDC withid-token: write— a malicious fork could consume Depot build minutes on this project, but cannot exfiltrate credentials.If consuming Depot minutes from fork PRs is unacceptable, an alternative is to skip the depot job entirely on fork PRs (
if: github.event.pull_request.head.repo.full_name == github.repository) instead of switching topull_request_target.Test plan
test-2, in-repo branch) still passes withDEPOT_PROJECT_IDpopulated.Validate Depot project idpasses and the Depot build runs.PUSH_IMAGESevaluates tofalseon fork PRs (no GHCR login attempt).