Skip to content

fix(ci): make test build work on pull requests from forks - #173

Open
Ganeshkumar Ashokavardhanan (ganeshkumarashok) wants to merge 1 commit into
mainfrom
fix/ci-fork-pr-registry-fallback
Open

fix(ci): make test build work on pull requests from forks#173
Ganeshkumar Ashokavardhanan (ganeshkumarashok) wants to merge 1 commit into
mainfrom
fix/ci-fork-pr-registry-fallback

Conversation

@ganeshkumarashok

Copy link
Copy Markdown
Collaborator

Problem

ci.yaml tags the test image with ${{ secrets.AZURE_REGISTRY_SERVER }}. Secrets are not exposed to pull_request workflows triggered from a fork, so on those runs the expression resolves to an empty string and the tag becomes /public/aks/<repo>:<version>, which buildx rejects:

ERROR: failed to build: invalid tag "/public/aks/aks-gpu-grid:570.211.01-...": invalid reference format

All six build jobs then fail within seconds. Pull requests from forks get no build signal at all, and the red checks are indistinguishable from a genuine breakage — which makes fork PRs hard to review.

This is long-standing rather than new. For example #161 (from a fork) failed this way, while #159 passed only because that run had Secret source: Actions.

Fix

Fall back to a placeholder registry when the secret is unavailable:

-t ${{ secrets.AZURE_REGISTRY_SERVER || 'localhost:5000' }}/public/aks/...

This workflow only builds images — it never runs azure/login, never runs az acr login, and never runs docker push (verified: zero occurrences of each in ci.yaml). The registry portion of the tag is therefore not meaningful here; it only has to be a syntactically valid reference for buildx to accept.

Runs that do have the secret are unaffected and keep tagging exactly as before, since || returns the first truthy operand and a populated secret is truthy.

Validation

  • actionlint: 7 pre-existing SC2086 findings on main, 7 on this branch — identical, no new findings introduced.
  • Tag resolution checked both ways: secret absent gives localhost:5000/public/aks/... (valid), secret present gives <registry>/public/aks/... (unchanged from today).
  • Scope confirmed: this is the only place in ci.yaml where the registry secret is consumed, and it is build-only.

Note

This is intentionally independent of #172 (the workflow expression-injection fix). Both touch the same docker buildx build lines, so whichever merges second will need a trivial conflict resolution — in #172 the fallback simply moves into the env: block as REGISTRY_SERVER: ${{ secrets.AZURE_REGISTRY_SERVER || 'localhost:5000' }}.

@ganeshkumarashok

Copy link
Copy Markdown
Collaborator Author

Verification

The CI run on this PR passes, but it runs from a branch on this repo, so secrets are present and it does not exercise the fallback. To validate the actual failing configuration I ran a controlled A/B in a fork with no repository secrets (identical to the Secret source: None condition a fork PR gets):

ref result
control unmodified main ❌ failure
treatment this branch ✅ success (all 6 images)

Control reproduced the reported error exactly:

ERROR: failed to build: invalid tag "/public/aks/aks-gpu-grid:570.211.01-...": invalid reference format
ERROR: failed to build: invalid tag "/public/aks/aks-gpu-cuda:595.71.05-...": invalid reference format

Treatment built every image against the placeholder:

localhost:5000/public/aks/aks-gpu-cuda
localhost:5000/public/aks/aks-gpu-cuda-arm64
localhost:5000/public/aks/aks-gpu-cuda-lts
localhost:5000/public/aks/aks-gpu-cuda-lts-arm64
localhost:5000/public/aks/aks-gpu-grid
localhost:5000/public/aks/aks-gpu-grid-v20

Notes:

ci.yaml tags the test image with ${{ secrets.AZURE_REGISTRY_SERVER }}. Secrets
are not exposed to `pull_request` workflows triggered from a fork, so on those
runs the expression resolves to an empty string and the tag becomes
`/public/aks/<repo>:<version>`, which buildx rejects:

    ERROR: failed to build: invalid tag
    "/public/aks/aks-gpu-cuda:595.71.05-...": invalid reference format

Every build job then fails within seconds, so pull requests from forks get no
build signal at all and the red checks are indistinguishable from a genuine
breakage.

Fall back to a placeholder registry when the secret is unavailable. ci.yaml
only builds images -- it never runs azure/login, az acr login, or docker push
(verified: zero occurrences of each) -- so the registry portion of the tag is
not meaningful here. It only has to be a syntactically valid reference for
buildx to accept.

Runs that do have the secret are unaffected and keep tagging exactly as before,
since `||` returns the first truthy operand.

Rebased onto main so the fallback lives in the `env:` blocks introduced by the
workflow expression-injection fix (d97b02b), rather than inline in the
`docker buildx build` arguments.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e525ff6d-7072-4f71-92b9-4d5a87808b20
@ganeshkumarashok

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main — this had gone CONFLICTING because #172 / d97b02b moved these expressions into env: blocks for the CWE-94 fix. The fallback now lives there instead of inline, exactly as anticipated in the "Note" above:

REGISTRY_SERVER: ${{ secrets.AZURE_REGISTRY_SERVER || 'localhost:5000' }}

Applied to all three build jobs (cuda, cuda-arm64, grid). PR is MERGEABLE again.

End-to-end validation under the real failure condition

Rather than add noise here, I dispatched ci.yaml via workflow_dispatch in a fork where AZURE_REGISTRY_SERVER genuinely does not exist — the same empty-secret condition as a fork PR. Run 32065857574: all 7 jobs green, including all 6 builds.

REGISTRY_SERVER: localhost:5000
+ image_ref=localhost:5000/public/aks/aks-gpu-cuda-arm64:595.71.05-20260817202720
+ docker buildx build ... -t localhost:5000/public/aks/aks-gpu-cuda-arm64:595.71.05-...
localhost:5000/public/aks/aks-gpu-cuda-arm64   595.71.05-...   f002fd5b6c17   39 seconds ago   1.17GB

So the tag is now valid, the build runs to completion, and a real image is produced — versus today's instant invalid reference format failure.

Live example of the bug this fixes: #178 (a fork PR, now superseded by #179) failed all six build jobs in seconds, with the grid job merely cancelled by matrix fail-fast — red checks that were indistinguishable from a genuine regression.

Other checks

  • actionlint: clean on this branch and on the main baseline — no new findings.
  • Scope re-verified against current main: ci.yaml contains zero occurrences of docker push, az acr login, and azure/login, so the registry prefix only needs to be syntactically valid here. main.yaml (which does push) is deliberately untouched.
  • Runs that do have the secret are unaffected: || returns the first truthy operand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant