Skip to content

chore(infra): docker-compose wrapper rework + bats tests#6348

Merged
gustavosbarreto merged 2 commits into
masterfrom
refactor/wrapper-and-env-cleanup
May 21, 2026
Merged

chore(infra): docker-compose wrapper rework + bats tests#6348
gustavosbarreto merged 2 commits into
masterfrom
refactor/wrapper-and-env-cleanup

Conversation

@gustavosbarreto
Copy link
Copy Markdown
Member

@gustavosbarreto gustavosbarreto commented May 21, 2026

Summary

  • Consolidate bin/docker-compose into an autonomous script; delete bin/utils. Replace set -o allexport sourcing with an env_var subshell helper so the wrapper does not pollute its shell, preserving COMPOSE_ENV_FILES last-wins semantics with the cloud env files.
  • Split enterprise-only defaults out of .env into a new .env.enterprise, loaded automatically when SHELLHUB_ENTERPRISE=true. Base .env becomes CE-only. The enterprise file lives in the public repo so a CE user can upgrade to Enterprise by flipping SHELLHUB_ENTERPRISE=true in .env.override and providing registry credentials, no re-clone needed.
  • Guard against SHELLHUB_CLOUD=true without SHELLHUB_ENTERPRISE=true, removing the silent coupling that used to come from cloud/.env hardcoding SHELLHUB_ENTERPRISE=true.
  • Keep the COMPOSE_FILE chain as the composition mechanism.
  • Add bats tests at tests/compose/ covering wrapper decisions (COMPOSE_FILE chain, COMPOSE_ENV_FILES composition, guard) without invoking Compose. Cloud-dependent scenarios live in with_cloud.bats and skip when ../cloud/ is absent. New compose-tests.yml workflow runs wrapper.bats on every PR that touches the wrapper, env files, or compose YAMLs.

The matching env cleanup on the cloud side is at shellhub-io/cloud#2375.

@gustavosbarreto gustavosbarreto requested a review from a team as a code owner May 21, 2026 15:07
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

Claude finished @gustavosbarreto's task in 3m 7s —— View job


Code Review Complete

Reviewed 13 files across code quality, security, testing, shell patterns, and architecture. 1 inline issue posted (1 high).

The overall approach is solid — the move from include: + profiles to an explicit COMPOSE_FILE chain simplifies the wrapper, the enterprise env split is clean, the cloud-without-enterprise guard closes a real gap, and the bats test suite is well-structured with a smart docker stub strategy.

The one issue found is an ordering regression in the COMPOSE_FILE chain: docker-compose.override.yml is placed before the database overlays, which means database files take precedence over user overrides — the opposite of the old auto-loading behavior.


To request another review round, comment /review.

@gustavosbarreto gustavosbarreto force-pushed the refactor/wrapper-and-env-cleanup branch from 66d9227 to 00f2325 Compare May 21, 2026 15:13
Comment thread bin/docker-compose Outdated
@gustavosbarreto
Copy link
Copy Markdown
Member Author

/review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

Claude finished @gustavosbarreto's task in 3m 17s —— View job


Code Review Complete

Reviewed 13 files across code quality, security, testing, shell patterns, and architecture. 2 inline issues posted (2 high).

The overall rework is a solid improvement — the explicit COMPOSE_FILE chain is much clearer than the include: + env-variable-to-/dev/null approach, the enterprise env split is clean, the cloud-without-enterprise guard closes a real gap, the env_var() subshell helper avoids polluting the wrapper's env, and the bats test suite is well-structured with a smart docker stub strategy. The override ordering fix from the first review round (ad3f3ea) is confirmed correct.

The two issues found are:

  1. Missing file-existence guards for cloud compose files — lines 44–46 append cloud overlay paths unconditionally, whereas master guarded them with [ -d ../cloud ] / [ -f ... ]. Enterprise devs without the cloud sibling repo will get a hard failure.

  2. Unwanted mongo container in enterprise + postgresdocker-compose.enterprise.yml defines a bare mongo: service (image override). On master this merged with the profiled definition from docker-compose.mongo.yml (always loaded via include:). Now that the mongo overlay is only loaded when DATABASE=mongo|migrate, the enterprise overlay's standalone mongo: creates an unintended container in production postgres deployments.


To request another review round, comment /review.

Comment thread bin/docker-compose Outdated
Comment thread bin/docker-compose
… env

Rewrite bin/docker-compose as a single autonomous script. bin/utils
and the readlink_f cross-platform workaround are gone. Env loading is
delegated to Compose v2.24+ via COMPOSE_ENV_FILES; a small env_var
helper reads flags from .env/.env.override in a subshell so the
wrapper's environment is not polluted (preserves last-wins for
COMPOSE_ENV_FILES).

Extract enterprise-only defaults out of .env into a new .env.enterprise:
admin credentials, SAML secret, web endpoints (8 vars), and object
storage (5 vars). The wrapper auto-loads .env.enterprise when
SHELLHUB_ENTERPRISE=true. Base .env now contains only CE settings.

Add a guard that aborts the wrapper when SHELLHUB_CLOUD=true without
SHELLHUB_ENTERPRISE=true, removing the silent enterprise auto-enable
that previously came from cloud/.env.

Drop the profiles annotations and /dev/null sentinel includes from
the previous refactor; the COMPOSE_FILE chain is kept.

Addresses team#111 problems 2 (silent cloud->enterprise coupling) and
3 (redundant SHELLHUB_CLOUD in cloud override).
Verify that bin/docker-compose builds the expected COMPOSE_FILE chain
and COMPOSE_ENV_FILES list for each deploy scenario, and that the
cloud-without-enterprise guard aborts. Tests stub `docker` in PATH so
the wrapper's exec is captured without invoking Compose — we test the
wrapper's decisions, not the Compose merge.

Scenarios that require the cloud/ sibling repo live in with_cloud.bats
and skip per test when ../cloud/ is absent. The compose-tests.yml
workflow runs wrapper.bats on every PR that touches the wrapper, the
env files, or the compose YAMLs.
@gustavosbarreto gustavosbarreto force-pushed the refactor/wrapper-and-env-cleanup branch from ad3f3ea to c6ae703 Compare May 21, 2026 17:32
@gustavosbarreto
Copy link
Copy Markdown
Member Author

All three review findings addressed in 8d96d7e (squashed into the wrapper commit):

  • docker-compose.override.yml now applies after the database overlays so user customizations win last.
  • Cloud overlays (../cloud/docker-compose*.yml) are now guarded with -f checks so enterprise dev without cloud/ cloned does not hard-fail.
  • The standalone mongo: image override was dropped from docker-compose.enterprise.yml; mongo is being phased out of the stack.

@gustavosbarreto
Copy link
Copy Markdown
Member Author

/review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

Claude finished @gustavosbarreto's task in 2m 42s —— View job


Code Review Complete

Reviewed 14 files across code quality, security, testing, shell patterns, and architecture — no issues found.

The overall rework is well-executed. The three issues from the previous review rounds (override ordering, cloud file-existence guards, unwanted mongo container) are all confirmed fixed. Specific observations:

  • The explicit COMPOSE_FILE chain is clearer than the include: + profiles approach and correctly positions docker-compose.override.yml after database overlays for proper last-wins semantics.
  • The env_var() subshell helper cleanly avoids polluting the wrapper's environment, and the ENV_OVERRIDE hook enables clean test isolation.
  • The COMPOSE_ENV_FILES ordering change (loading cloud/.env for all enterprise deployments, not just cloud) is a behavioral change from master, but it's documented in the wrapper comment, explicitly validated by with_cloud.bats, and guarded by -f checks.
  • The enterprise env split into .env.enterprise is clean, and the object storage defaults close a real gap for non-cloud enterprise deployments.
  • The bats test suite is well-structured — the docker stub strategy avoids needing a running daemon, require_cloud skips gracefully, and capture_with keeps test declarations readable.
  • The cloud-without-enterprise guard closes a real gap that previously relied on cloud/.env silently setting SHELLHUB_ENTERPRISE=true.

To request another review round, comment /review.

@gustavosbarreto gustavosbarreto merged commit 8fe212b into master May 21, 2026
12 checks passed
@gustavosbarreto gustavosbarreto deleted the refactor/wrapper-and-env-cleanup branch May 21, 2026 17:52
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.

1 participant