fix: sha256:8e5bf0f59b0dbd014580dec546aafd5eac90ea0915244fda362261505ebafc8f · gabriel/musehub

fix(#194): deploy.sh's ECR_REGISTRY now derives from ECR_IMAGE, not hardcoded

deploy.sh:36 hardcoded ECR_REGISTRY to the Nonproduction account (992382692655), used unconditionally by docker login/logout (lines 202-204). ECR_IMAGE (the thing docker actually pulls) is correctly passed per- environment by push.sh's caller -- but ECR_REGISTRY was a completely disconnected variable, so a "prod" deploy authenticated to Nonproduction's registry while trying to pull an image that only exists in Production's.

#159 closed with "deploy.sh needed no changes -- it already takes ECR_IMAGE as a parameter," which is true and irrelevant: the bug is in ECR_REGISTRY, a separate variable #159 never looked at. Not reopening #159 -- its closure was correct for what it covered; this ticket documents the gap it missed.

Fix: derive ECR_REGISTRY from ECR_IMAGE via bash parameter expansion (${ECR_IMAGE%%/*} -- everything before the first '/') instead of hardcoding a second, independently-maintained account ID. This is the same shape of fix as the underlying cause: two variables that must always agree were allowed to be set independently. Deriving one from the other makes drift structurally impossible instead of relying on remembering to keep two hardcoded values in sync (which is exactly what didn't happen here).

Tests (TDD, red confirmed before implementation): - tests/test_deployment.py::TestEcrRegistryDerivedFromImage (5 tests): executes the actual variable-derivation lines from deploy.sh in a real bash subprocess (not just a text/regex check, since this is a bash variable-scoping bug) -- staging ECR_IMAGE resolves to staging's registry, production ECR_IMAGE resolves to production's registry (the exact regression this closes), FULL_IMAGE still correctly combines image+tag, no-ECR_IMAGE-set falls back to staging (manual invocation), and a static guard against ECR_REGISTRY ever being re-hardcoded to a literal account ID.

34/34 in tests/test_deployment.py (29 pre-existing + 5 new), no regressions. Manually verified via bash -n (syntax) and direct invocation with both real account IDs.

aaronrene had a fix for this already branched locally (dev @ 8a1389ae, 2 files, 26 tests) but couldn't push it (blocked by #193, now fixed) -- his branch wasn't fetchable from any shared remote, so this is an independent reimplementation from the ticket's description, not his original diff.

Co-Authored-By: Claude Sonnet 5 <[email protected]>

symbols added

files changed

1073

files in snapshot

dead code introduced

Semantic Changes 9 symbols

~ deploy/deploy.sh .sh

~ tests/test_deployment.py .py 9 symbols added

+ TestEcrRegistryDerivedFromImage class class TestEcrRegistryDerivedFromImage L330–408

+ _extract_var_block method method _extract_var_block L339–351

+ _resolve method method _resolve L353–376

+ test_default_ecr_image_falls_back_to_staging_registry method method test_default_ecr_image_falls_back_to_staging_registry L393–397

+ test_full_image_matches_passed_ecr_image_and_tag method method test_full_image_matches_passed_ecr_image_and_tag L389–391

+ test_no_independently_hardcoded_production_or_staging_registry_constant method method test_no_independently_hardcoded_production_or_staging_registry_constant L399–408

+ test_production_registry_derived_from_ecr_image method method test_production_registry_derived_from_ecr_image L382–387

+ test_staging_registry_derived_from_ecr_image method method test_staging_registry_derived_from_ecr_image L378–380

+ subprocess import import subprocess L17–17

Files Changed

1073 in snapshot

~ deploy/deploy.sh .sh

~ tests/test_deployment.py .py

← Older Oldest on fix/194-deploy-sh-ecr-registry

All commits

Newer → Latest on fix/194-deploy-sh-ecr-registry

0 comments

No comments yet. Be the first to start the discussion.

To add a comment, use the Muse CLI: muse hub commit comment sha256:8e5bf0f59b0dbd014580dec546aafd5eac90ea0915244fda362261505ebafc8f --body "your comment"