Skip to content

fix: skip commit-SHA tag on release builds and block mutable tags on prd#18

Draft
charly-bg wants to merge 4 commits into
mainfrom
fix/release-digest-orphan-and-prd-gating
Draft

fix: skip commit-SHA tag on release builds and block mutable tags on prd#18
charly-bg wants to merge 4 commits into
mainfrom
fix/release-digest-orphan-and-prd-gating

Conversation

@charly-bg

Copy link
Copy Markdown
  • build-quay-main re-tagged ${{ github.sha }} on every build, including release/versioned builds.
  • When a release rebuilt a commit the main pipeline had already published, the SHA tag moved off the main-built digest (which dev/biz pin by digest); that digest then went untagged, Quay GC reaped it, and tasks failed with CannotPullContainerError.
  • Release builds now skip the SHA tag (auto on release events, or via skip-commit-sha-tag), publishing the immutable version only.
  • Also hardens the manual/deploy-latest workflows: default to dev, reject mutable latest/next on prd, and fix the input.tag typo.

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review: fix: skip commit-SHA tag on release builds and block mutable tags on prd

Branch: fix/release-digest-orphan-and-prd-gatingmain
Files changed: 3 (+35 −4)
CI: ✅ passing


Summary

Solid fix for a real production issue — release builds were re-tagging ${{ github.sha }} onto a new digest, which moved the SHA tag off the main build's digest. Quay GC reaped the untagged main-built digest, and tasks pinned by that digest failed with CannotPullContainerError.

The fix has three parts:

  1. build-quay-main.yml — skip the SHA tag on release events (auto-detected) or via the new skip-commit-sha-tag input
  2. deploy-latest.yml — add mutable-tag guard for prd + fix the input.taginputs.tag typo (this was already broken on main)
  3. manual-deploy-service.yml — default to dev instead of prd + add the same mutable-tag guard

Findings

No P0 or P1 issues found.

P2 — Minor

  1. [Security] Script injection anti-pattern in shell case blocksdeploy-latest.yml:29 and manual-deploy-service.yml:39

    ${{ inputs.tag }} is interpolated directly into the shell script. If a crafted tag value contained shell metacharacters, it could inject commands. The risk is low (requires repo write access for workflow_dispatch, or a compromised caller for workflow_call), but the recommended pattern is to pass through an env var:

    env:
      TAG: ${{ inputs.tag }}
    run: |
      case "$TAG" in

    Non-blocking — just a defense-in-depth improvement.

  2. [Patterns] Duplicated mutable-tag guard — The identical case block appears in both deploy-latest.yml and manual-deploy-service.yml. Could be extracted into a shared composite action, but for 2 instances in simple workflows the duplication is perfectly acceptable.

  3. [Completeness] Mutable tag blocklist covers latest|next only — Other potentially mutable tags (e.g. dev, stg, edge) are not blocked on prd. If those tags don't exist in the current ecosystem, this is fine. Worth confirming this covers all mutable tags in use.

What's good

  • The conditional tag expression ${{ (github.event_name == 'release' || inputs.skip-commit-sha-tag) && inputs.docker-tag || format(…) }} is correct — operator precedence evaluates as expected and the skip-commit-sha-tag escape hatch handles non-release-event versioned builds
  • The input.taginputs.tag typo fix in deploy-latest.yml is a critical bug fix — the old expression resolved to empty string, making the workflow essentially broken
  • Defaulting to dev and reordering options in manual-deploy-service.yml is a good safety improvement
  • Comments are excellent — the inline explanation of why the SHA tag is skipped is clear and will help future maintainers
  • Backward-compatible: skip-commit-sha-tag defaults to false, so all existing callers (25+ repos) are unaffected

Consumer impact

This workflow is called by 25+ repos across the org (builder-server, atlas-server, marketplace-server, auth-server, etc.). All changes are backward-compatible — no callers need to be updated. The new skip-commit-sha-tag input is opt-in. Release callers that trigger via release events get the fix automatically.

Security review

No secrets, credentials, or sensitive data exposed. The P2 script injection note above is the only finding. No new dependencies introduced.

Verdict: ✅ APPROVE

No blocking issues. The three P2 notes are non-blocking improvements that can be addressed in a follow-up.


Reviewed by Jarvis 🤖 · Requested by Charly via Slack

Comment thread .github/workflows/deploy-latest.yml Outdated

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-review after feedback

The P2 script injection finding from the first review has been addressed in ff364b9 — both deploy-latest.yml and manual-deploy-service.yml now pass inputs.tag through an env var instead of interpolating directly into the shell. Clean fix.

No remaining findings. CI passing. Good to merge.


Reviewed by Jarvis 🤖 · Requested by Charly via Slack

@charly-bg
charly-bg force-pushed the fix/release-digest-orphan-and-prd-gating branch from ff364b9 to 354db91 Compare June 11, 2026 21:14
@charly-bg
charly-bg marked this pull request as draft June 12, 2026 13:42
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.

2 participants