From adf5a836fc548d0c874e20aa79bbcf03b6c64fa6 Mon Sep 17 00:00:00 2001 From: "Kai (Tam Nhu) Tran" <61256810+kaitranntt@users.noreply.github.com> Date: Sun, 17 May 2026 05:01:15 -0400 Subject: [PATCH] fix: address reviewer findings round 2 (#1261 loop 2) (#1272) * fix(docker): parameterize image with CCS_IMAGE env so smoke-test exercises new digest docker/compose.yaml hardcoded image: ghcr.io/kaitranntt/ccs:latest, causing network-contract.sh to always use the old published image regardless of what IMAGE_OVERRIDE was passed. Switching to \${CCS_IMAGE:-ghcr.io/kaitranntt/ccs:latest} lets CI pass CCS_IMAGE= so the smoke-test actually exercises the just-built image. Default end-user behaviour is unchanged. Resolves REV4 (reviewer loop 2, PR #1261). * fix(test): image-size.sh fails loudly on manifest inspection failure The --platform branch previously exited 0 when imagetools inspect returned empty or zero bytes, silently passing the budget check. A broken image could reach production undetected if buildx had any inspection issue. Changed to exit 1 with a clear diagnostic message explaining possible causes (old buildx, manifest format mismatch, image not yet pushed). No --allow-inspect-failure escape hatch is provided; CI must fix the root cause. Resolves REV5 (reviewer loop 2, PR #1261). * test(docker): cover --platform branch in image-size-logic tests Prior tests only exercised the local docker image inspect path. The --platform branch (imagetools inspect) had zero coverage, meaning the REV5 silent-pass regression would not have been caught by CI. Added 5 mock-based test cases for the --platform branch: - pass when platform-scoped compressed size < budget - fail when platform-scoped compressed size > budget - fail (exit 1) when imagetools inspect errors (REV5 guard) - fail (exit 1) when reported size is "0" (REV5 guard) - fail (exit 1) when size output is empty (REV5 guard) Mocks follow the existing pattern (override docker in PATH with a temp wrapper), extended to handle buildx imagetools inspect subcommand. Resolves REV6 (reviewer loop 2, PR #1261). * fix(test): compose-parity image_name() handles \${VAR:-default} syntax After parameterizing compose.yaml's image field with \${CCS_IMAGE:-...}, the image_name() sed pipeline cut at the first colon in the shell variable syntax (:-) instead of the tag separator, returning '\${CCS_IMAGE' and failing the kaitranntt/ccs grep. Added a sed step to unwrap \${VAR:-default} by extracting just the default value before stripping the tag. Existing plain image: name:tag references are unaffected. --- docker/compose.yaml | 2 +- tests/docker/compose-parity.sh | 9 ++- tests/docker/image-size-logic.test.sh | 81 +++++++++++++++++++++++++++ tests/docker/image-size.sh | 7 ++- 4 files changed, 93 insertions(+), 6 deletions(-) diff --git a/docker/compose.yaml b/docker/compose.yaml index e4616cba..7f08f9b7 100644 --- a/docker/compose.yaml +++ b/docker/compose.yaml @@ -14,7 +14,7 @@ services: ccs: - image: ghcr.io/kaitranntt/ccs:latest + image: ${CCS_IMAGE:-ghcr.io/kaitranntt/ccs:latest} restart: unless-stopped ports: - "3000:3000" diff --git a/tests/docker/compose-parity.sh b/tests/docker/compose-parity.sh index 6da741d4..9d1af093 100644 --- a/tests/docker/compose-parity.sh +++ b/tests/docker/compose-parity.sh @@ -25,14 +25,19 @@ ok() { printf '[OK] %s\n' "$*"; } fail() { printf '[X] %s\n' "$*" >&2; fail=1; } # --------------------------------------------------------------------------- -# Helper: extract image name (repo path without tag) for a service +# Helper: extract image name (repo path without tag) for a service. +# Handles both plain image references and ${VAR:-default} shell variable +# syntax (e.g. image: ${CCS_IMAGE:-ghcr.io/kaitranntt/ccs:latest}). # --------------------------------------------------------------------------- image_name() { local file="$1" service="$2" - # Match lines like: image: ghcr.io/owner/repo:tag or image: name:tag + # Match lines like: + # image: ghcr.io/owner/repo:tag + # image: ${CCS_IMAGE:-ghcr.io/owner/repo:tag} grep -A 50 "^ ${service}:" "$file" \ | grep -m1 '^\s*image:' \ | sed 's/.*image:\s*//' \ + | sed 's/\${[^:-]*:-\([^}]*\)}/\1/' \ | sed 's/:.*//' \ | tr -d ' ' } diff --git a/tests/docker/image-size-logic.test.sh b/tests/docker/image-size-logic.test.sh index 63ae23f5..1fdcdd4e 100755 --- a/tests/docker/image-size-logic.test.sh +++ b/tests/docker/image-size-logic.test.sh @@ -120,6 +120,87 @@ else (( FAIL++ )) || true fi +# ------------------------------------------------------------------ +# --platform branch tests (multi-arch via imagetools) +# ------------------------------------------------------------------ +# Mock docker that handles `docker buildx imagetools inspect` by echoing +# a fixed layer-size string (space-separated byte counts as imagetools does). +# The mock ignores the --format flag and just prints precomputed sizes. +# ------------------------------------------------------------------ + +make_mock_docker_platform() { + local layer_output="$1" # space-separated byte values, e.g. "100000000 50000000" + cat > "${MOCK_DIR}/docker" <<'MOCK_EOF' +#!/usr/bin/env bash +# Mock docker for --platform branch tests +if [[ "$1" == "buildx" && "$2" == "imagetools" && "$3" == "inspect" ]]; then +MOCK_EOF + # Inject the layer_output value into the mock script + printf ' echo "%s"\n' "$layer_output" >> "${MOCK_DIR}/docker" + cat >> "${MOCK_DIR}/docker" <<'MOCK_EOF' + exit 0 +fi +# image inspect / pull / other sub-commands: succeed silently +exit 0 +MOCK_EOF + chmod +x "${MOCK_DIR}/docker" +} + +make_mock_docker_platform_fail() { + # imagetools inspect returns nothing (simulates buildx incompatibility) + cat > "${MOCK_DIR}/docker" <<'MOCK_EOF' +#!/usr/bin/env bash +if [[ "$1" == "buildx" && "$2" == "imagetools" && "$3" == "inspect" ]]; then + exit 1 +fi +exit 0 +MOCK_EOF + chmod +x "${MOCK_DIR}/docker" +} + +run_platform_test() { + local name="$1" + local expected_exit="$2" + local max_bytes="$3" + # mock docker already set by caller + + local actual_exit=0 + PATH="${MOCK_DIR}:${PATH}" bash "$SCRIPT" "mock-image:tag" "$max_bytes" \ + --platform linux/amd64 > /dev/null 2>&1 || actual_exit=$? + + if [[ "$actual_exit" -eq "$expected_exit" ]]; then + echo "[OK] ${name}" + (( PASS++ )) || true + else + echo "[X] ${name}: expected exit ${expected_exit}, got ${actual_exit}" + (( FAIL++ )) || true + fi +} + +echo "" +echo "Running --platform branch tests..." +echo "" + +# --platform pass: two layers summing to 150 MB, budget 200 MB +make_mock_docker_platform "100000000 57671680" +run_platform_test "--platform: pass when platform-scoped size < budget" 0 "209715200" + +# --platform fail: two layers summing to 250 MB, budget 200 MB +make_mock_docker_platform "150000000 112000000" +run_platform_test "--platform: fail when platform-scoped size > budget" 1 "209715200" + +# --platform inspect failure → must exit 1 (REV5 regression guard) +make_mock_docker_platform_fail +run_platform_test "--platform: fail loudly when imagetools inspect fails (REV5 guard)" 1 "209715200" + +# --platform returns "0" → must exit 1 (REV5 guard for zero-byte output) +make_mock_docker_platform "0" +run_platform_test "--platform: fail loudly when reported size is 0 (REV5 guard)" 1 "209715200" + +# --platform returns empty string → must exit 1 (REV5 guard for empty output) +make_mock_docker_platform "" +run_platform_test "--platform: fail loudly when size output is empty (REV5 guard)" 1 "209715200" + # ------------------------------------------------------------------ # Summary # ------------------------------------------------------------------ diff --git a/tests/docker/image-size.sh b/tests/docker/image-size.sh index c82ed09e..f1449bec 100755 --- a/tests/docker/image-size.sh +++ b/tests/docker/image-size.sh @@ -80,9 +80,10 @@ if [[ -n "$PLATFORM" ]]; then fi if [[ -z "$ACTUAL_BYTES" || "$ACTUAL_BYTES" == "0" ]]; then - echo "[!] Could not determine size for ${IMAGE} platform=${PLATFORM} — skipping budget check" >&2 - echo " (imagetools may not support format templates on this buildx version)" >&2 - exit 0 + echo "[X] Could not determine size for ${IMAGE} platform=${PLATFORM}" >&2 + echo " Possible causes: buildx version too old, manifest format unsupported, image not pushed yet" >&2 + echo " Refusing to silently pass — fix the inspection or push the image first" >&2 + exit 1 fi else # Local-image path: use docker image inspect (host architecture only)