fix: remove_unused_images empty SSH output on remote#8
Merged
Conversation
…ng bugs Reported on a real prod deploy: + OUTPUT_COMPOSE= with a side-by-side showing the same ssh command produces 7 image rows when run by hand. The stdout vanished when the gpd script ran the identical command — symptom of bash word-splitting and shell quoting bugs that this file dodged in PR #6. This file was the only one in functions/ that PR #6 didn't touch. It still had: - dual-branch local-vs-remote (8 sites). Calls `gpd ssh "${user}@${host}" 'cmd'` with hand-rolled string concatenation, which is exactly what the new helpers exist to replace. - `for u in \`seq 0 N-1\`` loops, broken on macOS BSD seq the same way _config_files.sh was. - This particular gem at line 89: 'docker images | grep "'"${CI_REGISTRY}"'/'"${CI_PROJECT_PATH}"'/" | sed '/"${OUTPUT_CLI_KEEP_VERSION}"/d'' Count the quotes. The `'/"${VERSION}"/d'` segment escapes to the remote shell as `/V/d` -- with literal double-quotes around the version, which is _not_ what `sed /pat/d` accepts. Whatever this pipeline actually executed on the remote was at best a fluke; on most setups it returned an empty string. Refactor: - Every dual-branch collapsed onto run_in_target / compose_in_target. Same helpers _deploy.sh switched to in PR #6, with printf %q quoting of every argument before they reach the SSH payload. - `for ((i=0; i<${#arr[@]}; i++))` and `while read` instead of seq-based index loops; works on BSD seq, GNU seq, no seq. - `docker image ls --format '{{.Repository}}:{{.Tag}}'` directly, so we never have to count columns or parse sed/awk pipelines. - `grep -vF ":${VERSION}"` (fixed-string, anchored to the tag suffix) in place of the broken `sed '/V/d'`. Same intent, no quoting trap. - `set -e` survival via `|| true` on every grep that may legitimately match nothing (no compose-managed images, no old custom builds). - `exit 0` -> `return 0` for consistency with the rest of the file set, and so the caller's control flow stays clean. CHANGELOG: noted under [Unreleased] -> Fixed. bats locally: 46/46 still green (this function isn't covered by the fixture; the smoke test exercises generate, not unused). End-to-end verification needs an actual remote host with images to clean — same gap the user just hit.
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
1 participant
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.
Symptom
Reported on a real prod deploy of
prodbc:with the user noting the same
ssh ... '{ docker compose --env-file …/.env images | sed 1d; }'produces 7 image rows when run by hand. The script captures empty stdout for the same command.Root cause
This file was the only
functions/_*.shthat PR #6 didn't touch. It still had every pattern that PR #6 fixed elsewhere:8 hand-rolled local-vs-remote dual-branches concatenating
'…'\"\${var}\"'…'-style SSH payloads.for u in \seq 0 N-1`loops broken on BSDseq` (counts down through -1 instead of producing nothing).This sed pipeline at line 89:
Count the quotes. The
/V/dends up with literal double-quotes around the version on the remote shell — not what sed accepts. Most invocations returned empty.Fix
Same refactor pattern PR #6 applied to
_deploy.sh:run_in_target/compose_in_target. Args are shell-quoted withprintf %qfor the SSH path, so quoting traps go away.for ((i=0; i<${#arr[@]}; i++))andwhile IFS= read -r line; noseq.docker image ls --format '{{.Repository}}:{{.Tag}}'directly, so column counting in awk/sed is replaced by precise filters.grep -vF ":\${VERSION}"(fixed-string) replaces the brokensed '/V/d'. Same intent, no quoting trap.set -esurvival via|| trueon every grep that may legitimately match nothing.exit 0→return 0for consistency with the rest of the file set.Test plan
bash -nclean.bats tests/bats/— 46/46 still green (this function isn't covered by the fixture; the smoke test exercises generate, not unused).make gpd-generateagainst the xyxyx/cloud parent — passes (no regression on the unrelated paths).gpd.sh -e prodbc -b /srv/docker -uagainst a real remote host with images to clean — needs you to verify, since the bug only fires on remote.