feat(docker): add production compose stack - #563
Conversation
The README told deployers to hand-convert the dev compose file, which predates the TimescaleDB analytics split and left migration ordering, aggregate backfill, and env wiring as manual steps. docker-compose.prod.yml now runs the full stack (app, TimescaleDB with the app and analytics databases, Redis) with a one-shot job that applies both Alembic lineages and backfills continuous aggregates before the app starts. Connection URLs are built from POSTGRES_* at interpolation time, so the make prod.* targets pass both .env and .env.local to compose. The app service forces SERVE_FRONTEND=true because the env_file value would otherwise override the image ENV and disable the bundled frontend.
|
@claude-review |
|
Claude finished @hamza-56's task in 4m 51s —— View job Review: production compose stack
This is a well-designed, self-contained stack — nice attention to detail throughout. Migrations gate app startup via I left 3 inline comments. None are hard blockers, but the first is a real footgun in the PR's own test instructions. Findings🟠 Medium — 🟡 Low/Medium — silent fallback to the committed dev password ( 🟡 Low — first-boot healthcheck race ( Notes (non-blocking)
Overall: approve-worthy once the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| .PHONY: prod.logs | ||
| prod.logs: ## Tail production stack logs (make prod.logs [service]) | ||
| $(COMPOSE_PROD) logs -f $(ARGS) |
There was a problem hiding this comment.
[Medium] make prod.logs migrations collides with the real migrations target.
ARGS is derived from MAKECMDGOALS (line 5), and the extra goal is also handed to make as a target. For service names like db/redis that's harmless — the %: catch-all (line 277) swallows them. But migrations is a real target (line 56), and explicit targets win over the catch-all. So make prod.logs migrations — which the PR's own How to Test step 5 recommends — will:
- run
prod.logs(tailslogs -f, blocks until Ctrl-C), then - run the
migrationstarget →uv run alembic upgrade headagainst the local/native DB from.env(localhost:5432).
That's a surprising, potentially destructive side effect from what looks like a read-only "tail logs" command.
Simplest fix: rename the compose service migrations → migrate (no target collision), then make prod.logs migrate is clean. Update the README/PR test steps to match.
| DATABASE_URL: postgresql://${POSTGRES_USER:-sparkth}:${POSTGRES_PASSWORD:-sparkth_password}@db:5432/${POSTGRES_DB:-sparkth} | ||
| ANALYTICS_DATABASE_URL: postgresql://${POSTGRES_USER:-sparkth}:${POSTGRES_PASSWORD:-sparkth_password}@db:5432/sparkth_analytics |
There was a problem hiding this comment.
[Low/Medium] Silent fallback to the committed dev password in production.
${POSTGRES_PASSWORD:-sparkth_password} means that if an operator forgets to set POSTGRES_PASSWORD in .env.local, the whole stack comes up with the well-known committed default rather than failing. The DB port isn't published in this stack (good — internal-only), so the blast radius is limited, but for a file whose entire purpose is production, defense-in-depth argues for failing fast:
DATABASE_URL: postgresql://${POSTGRES_USER:-sparkth}:${POSTGRES_PASSWORD:?POSTGRES_PASSWORD must be set in .env.local}@db:5432/${POSTGRES_DB:-sparkth}Same reasoning applies to the app's SECRET_KEY / LLM_ENCRYPTION_KEY, which are loaded via env_file from the committed .env defaults — the "MUST change in production" checklist documents them but nothing enforces it. A fail-fast here turns a forgotten override into a startup error instead of a silently-insecure deployment.
| healthcheck: | ||
| # $$ defers expansion to the container shell, which always has | ||
| # POSTGRES_* from env_file (compose interpolation may not). | ||
| test: ["CMD-SHELL", "pg_isready -U $${POSTGRES_USER:-sparkth} -d $${POSTGRES_DB:-sparkth}"] |
There was a problem hiding this comment.
[Low] First-boot race: the healthcheck can go green before the analytics DB exists.
On a fresh volume the Postgres entrypoint runs docker/initdb/01-create-analytics-db.sh while the server listens on the unix socket only (TCP disabled). pg_isready here connects over that socket, so it can report accepting connections during the init phase — before sparkth_analytics has been created and before TCP is open. The migrations service depends only on db: service_healthy and immediately connects over TCP to db:5432 to run alembic -c alembic_analytics.ini upgrade head, which needs sparkth_analytics to already exist.
start_period: 30s masks this most of the time, but TimescaleDB first-init can be slow. Options to harden:
- have the migrations command retry the connection (e.g. wrap in a short
until pg_isready -h db ...; do sleep 1; doneloop before running alembic), or - make the healthcheck assert the analytics DB exists, e.g.
pg_isreadyon-d sparkth_analytics(only meaningful once created).
Not blocking, but worth confirming on a cold docker volume rm + make prod.up.
regisb
left a comment
regisb
left a comment
There was a problem hiding this comment.
Review pending. Please wait until I review this fully.
| # Both --env-file flags are required so the POSTGRES_* values from .env.local | ||
| # reach the interpolated connection URLs below. | ||
|
|
||
| name: sparkth-prod |
There was a problem hiding this comment.
This is unnecessary, let's remove this line.
|
|
||
| name: sparkth-prod | ||
|
|
||
| x-logging: &default-logging |
There was a problem hiding this comment.
No need to override default logging. Let users decide that for themselves. We should remove this section and all "logging" settings.
There was a problem hiding this comment.
Removed the logging section and all logging settings.
| max-file: "5" | ||
|
|
||
| # .env.local overrides .env (later entries win) and may be absent. | ||
| x-env-files: &env-files |
There was a problem hiding this comment.
I have a big issue with how we handle environment files here. It really boils down to the fact that .env.local is mandatory. So we should explicitely load both .env and .env.local in all services:
env_file:
- .env
- .env.local
No need to check for presence in Makefile: let's just crash if .env.local is not present.
Also, no need to create x-env-files: we can just duplicate the .env/.env.local list in all services.
There was a problem hiding this comment.
Done, every service now lists .env and .env.local explicitly and compose crashes if .env.local is missing.
|
|
||
| # In-network URLs for the bundled services, overriding the localhost defaults | ||
| # committed in .env. POSTGRES_PASSWORD is embedded, so it must be URL-safe. | ||
| x-app-env: &app-env |
There was a problem hiding this comment.
Wait, why is this necessary? I suggest we add the list of explicit env variables to the sparkth service.
There was a problem hiding this comment.
Dropped the anchor, the connection URLs now go in .env.local and the sparkth service only sets SERVE_FRONTEND.
|
|
||
| # Applies both Alembic lineages, then backfills TimescaleDB continuous | ||
| # aggregates (idempotent). The app starts only after this succeeds. | ||
| migrations: |
There was a problem hiding this comment.
Let's not run migrations automatically on every application start. Instead, add instructions in the README that explain how to run migrations. The fact that we need to run 3 different commands should tell us that we need to simplify the way we run migrations...
There was a problem hiding this comment.
Removed the auto-run job and collapsed the three commands into a single sparkth migrate CLI command, documented in the README.
| shm_size: 256mb | ||
| env_file: *env-files | ||
| volumes: | ||
| - postgres_data:/var/lib/postgresql |
There was a problem hiding this comment.
I personally prefer bind-mounted data volumes, but let's stick with named volumes for consistency with the dev environment (nothing to change here).
| # The prod compose file builds the app's connection URLs from POSTGRES_*, so | ||
| # compose interpolation must read .env.local (when present) on top of .env; | ||
| # these targets pass both --env-file flags for that reason. | ||
| COMPOSE_PROD = docker compose -f docker-compose.prod.yml --env-file .env$(if $(wildcard .env.local), --env-file .env.local) |
There was a problem hiding this comment.
This Makefile is going to replicate all the docker compose commands (logs, up, down, etc.) but without all the nice CLI options. So I suggest we don't add make commands. Instead, the README instructs to run docker compose -f docker-compose.prod.yml up -d, then apply migrations.
There was a problem hiding this comment.
Dropped the make targets, the README now uses plain docker compose commands.
Applying migrations took three commands (two alembic lineages plus the aggregate backfill), which was easy to get wrong in production. The new `sparkth migrate` command runs all three; `make migrations` now delegates to it, so the backfill is no longer a separate manual step.
Address review feedback on the production stack: - drop the project name override and all logging configuration, leaving log drivers to the operator - make .env.local mandatory: every service loads .env and .env.local explicitly, and compose fails to start when .env.local is missing - drop the x-env-files and x-app-env anchors; the connection URLs now live in .env.local (pointed at the bundled db and redis containers), and the sparkth service only overrides SERVE_FRONTEND - stop running migrations automatically on startup; the README documents the single `sparkth migrate` command instead - remove the make prod.* wrappers in favor of plain docker compose commands in the README
|
@claude-review |
|
Claude finished @hamza-56's task in 1m 52s —— View job Review: production compose stack (updated)
The stack was substantially simplified since the last round and reads much cleaner. The I couldn't run Previous review comments — status
Findings (all non-blocking)🟡 Low — first-boot race between 🟡 Low — 🟢 Nit — docstring vs. behavior in Notes
Overall: LGTM. All requested changes are addressed; the two remaining items are optional hardening, not blockers. |
…se-stack # Conflicts: # sparkth/cli/main.py
2 participants
Closes: Self-hosted production deployments need a production-grade Docker Compose stack
What
Adds a self-contained production Docker Compose stack that runs the whole platform (app, TimescaleDB with the app and analytics databases, Redis), plus a one-shot
sparkth migrateCLI command that applies both Alembic lineages and the continuous-aggregate backfill, replacing the README's "convert the dev compose yourself" instructions.Changes
.envand.env.localexplicitly, and compose fails to start if.env.localis missingmigratecommand that applies both Alembic lineages and backfills continuous aggregates in one step;make migrationsnow delegates to itdocker composecommands, migrations run explicitly afterup).env.localand how the migrate command covers the backfillHow to Test
make docker.build(or let compose pullghcr.io/edly-io/sparkth:latest).env.localwith the production overrides, pointingDATABASE_URL,ANALYTICS_DATABASE_URL, andREDIS_URLat the bundled containers (see the README's "Production" section)docker compose -f docker-compose.prod.yml up -ddocker compose -f docker-compose.prod.yml run --rm sparkth python -m sparkth.cli.main migrateshows both Alembic lineages applied and the aggregate backfill rundocker compose -f docker-compose.prod.yml psshowsdbandredishealthy andsparkthrunningdocker compose -f docker-compose.prod.yml down, thenup -dagain: data volumes survive.env.localtemporarily and re-runup -d: compose refuses to startuv run pytest tests/cli/test_migrate.pycovers the new CLI commandNotes
sparkth/cli/migrate.pycommand..env.localis now mandatory for the production stack and must carry the connection URLs (@db:5432,redis://redis:6379).SPARKTH_TAG(image tag, default latest),SPARKTH_HTTP_PORT(published port, default 7727),SPARKTH_HTTP_BIND(bind address; set to 127.0.0.1 behind a same-host reverse proxy).TRUSTED_PROXY_HOPS) and pointSMTP_*at a real provider in.env.local.