chore(infra): replace COMPOSE_FILE chain with profiles and sentinels#6339
Conversation
|
Claude finished @geovannewashington's task in 1m 56s —— View job Code Review CompleteReviewed 8 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (high severity). The overall approach is solid — replacing Issue found
If you've addressed the feedback and want a new review, tag |
a2fcadb to
4c16a35
Compare
4c16a35 to
856223c
Compare
|
/review |
|
Claude finished @otavio's task in 2m 25s —— View job Code Review CompleteReviewed 6 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (critical severity, 2 locations). The overall approach is clean — replacing Issue found
If you've addressed the feedback and want a new review, tag |
| SCRIPT_DIR=$(cd "$(dirname "$0")" && pwd -P) | ||
| cd "$SCRIPT_DIR/.." | ||
|
|
||
| SERVICES=$(docker compose ps -q | grep -v INFO: | xargs docker inspect -f '{{.State.Running}}' 2>/dev/null | grep -c 'true') |
There was a problem hiding this comment.
CRITICAL: grep -c exits 1 when count is 0, killing the script under set -e before the error message prints.
grep -c 'true' returns exit code 1 when zero lines match (POSIX behavior — exit status is based on whether any lines matched, regardless of -c). Since set -eu is active on line 3, the command substitution SERVICES=$(...) propagates that non-zero exit, and the shell terminates immediately at this line. The [ "$SERVICES" -le 0 ] guard and the "ShellHub is not running" error message on line 9 are never reached.
The old bin/utils had the same pipeline inside exit_if_not_running, but was safe because bin/utils used set -o allexport — not set -e.
Same issue in bin/setup:32.
| SERVICES=$(docker compose ps -q | grep -v INFO: | xargs docker inspect -f '{{.State.Running}}' 2>/dev/null | grep -c 'true') | |
| SERVICES=$(docker compose ps -q | grep -v INFO: | xargs docker inspect -f '{{.State.Running}}' 2>/dev/null | grep -c 'true' || true) |
| echo "" | ||
|
|
||
| exit_if_not_running | ||
| SERVICES=$(docker compose ps -q | grep -v INFO: | xargs docker inspect -f '{{.State.Running}}' 2>/dev/null | grep -c 'true') |
There was a problem hiding this comment.
CRITICAL: Same grep -c / set -e issue as bin/cli:8.
See the comment on bin/cli:8 for full details. The script silently exits instead of printing the "not running" error message.
| SERVICES=$(docker compose ps -q | grep -v INFO: | xargs docker inspect -f '{{.State.Running}}' 2>/dev/null | grep -c 'true') | |
| SERVICES=$(docker compose ps -q | grep -v INFO: | xargs docker inspect -f '{{.State.Running}}' 2>/dev/null | grep -c 'true' || true) |
Replace the manual COMPOSE_FILE string concatenation in bin/utils with a new bin/docker-compose wrapper that uses COMPOSE_PROFILES and SHELLHUB_INCLUDE_* sentinel variables. bin/utils is deleted. Its responsibilities are now handled directly by bin/docker-compose: - Env loading is delegated to Compose via COMPOSE_ENV_FILES instead of set -o allexport sourcing. - Feature flags are translated to COMPOSE_PROFILES for add-only services (agent, mongo, postgres). - Overlay files (dev, autossl, enterprise) are loaded via include: sentinels in docker-compose.yml instead of COMPOSE_FILE concatenation. - Cross-repo cloud includes are handled the same way. The Makefile's EXTRA_COMPOSE_FILE is renamed to SHELLHUB_INCLUDE_EXTRA to follow the same sentinel convention.
856223c to
53e6b9b
Compare
|
/review |
|
Claude finished @otavio's task in 4m 32s —— View job Code Review CompleteReviewed 6 files across code quality, security, testing, language patterns, and architecture — no issues found. The code looks good as-is. The overall approach is solid — replacing If you push additional changes and want a new review, tag |
What changed?
Replaces the manual
COMPOSE_FILEstring concatenation inbin/utilswith a new
bin/docker-composewrapper.bin/utilsis deleted.Env loading is now delegated to Compose via
COMPOSE_ENV_FILES.Feature flags are translated to
COMPOSE_PROFILESfor add-onlyservices. Overlay files are loaded via
include:sentinels indocker-compose.yml.EXTRA_COMPOSE_FILEin the Makefile is renamedto
SHELLHUB_INCLUDE_EXTRAto follow the same convention.Why
Part of #111. Steps 2, 3 and 4 from the migration plan are collapsed
into a single PR since they are too coupled to split safely.
How to test
./bin/docker-compose up -d- confirm all services healthy./bin/docker-compose down, setSHELLHUB_DATABASE=mongoin.env.override,./bin/docker-compose up -d- confirm mongopresent, postgres absent
SHELLHUB_DATABASE=migrate- confirm both databases presentSHELLHUB_DATABASE=invalid- confirm ❌ error and exitSHELLHUB_ENV=development- confirm agent present