Skip to content

chore: parameterize docker host ports and wire s2-lite by default#3642

Open
ericallam wants to merge 2 commits into
mainfrom
chore/s2-lite-oss-onboarding
Open

chore: parameterize docker host ports and wire s2-lite by default#3642
ericallam wants to merge 2 commits into
mainfrom
chore/s2-lite-oss-onboarding

Conversation

@ericallam
Copy link
Copy Markdown
Member

@ericallam ericallam commented May 16, 2026

Summary

Two papercuts new contributors hit running this repo locally:

  1. Fresh clones default to v1 (Redis-only) realtime streams, so Sessions and chat.agent error with "S2 configuration is missing", even though the s2 service is already in docker/docker-compose.yml and pre-seeds a trigger-local basin. Wire REALTIME_STREAMS_S2_* to it in .env.example so the new-contributor flow just works. (Also drop the s2 healthcheck: the image is distroless, so the wget check always reports unhealthy.)

  2. Two clones can't both run pnpm run docker because ports, project name, and container names are all hardcoded. Parameterize every host port as ${VAR:-default}, drive the project name via COMPOSE_PROJECT_NAME (with a top-level name: field as the default), prefix container names with ${CONTAINER_PREFIX:-}, and pass --env-file .env so compose reads the same root .env the webapp does. The "Running multiple instances side by side" block in .env.example lists every overridable knob.

Also split the optional services (electric-shard-1, ch-ui, toxiproxy, nginx-h2, otel-collector, prometheus, grafana) into docker-compose.extras.yml behind a new pnpm run docker:full script. The core stack keeps everything the webapp actually needs to boot: postgres, redis, electric, minio, clickhouse + migrator, s2-lite.

Defaults match every previous hardcoded value, so existing setups keep working without touching .env.

Test plan

  • pnpm run docker on a clean clone brings up the core services on the standard ports under the triggerdotdev-docker project name.
  • Setting COMPOSE_PROJECT_NAME=triggerdotdev-docker-alt + the *_HOST_PORT overrides in .env brings up a second stack alongside the default one with no port or container-name clashes.
  • Webapp boots cleanly against the default .env.example values; /healthcheck returns 200, no S2 errors.
  • s2-lite basin trigger-local accepts an append + read via the same REST endpoints the webapp uses.
  • pnpm run docker:full brings up the optional services alongside the core ones in the same project.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 16, 2026

⚠️ No Changeset found

Latest commit: c8ee6c4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 16, 2026

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds a commented S2 Realtime streams v2 block to .env.example and updates observability guidance. Parameterizes docker/docker-compose.yml (container names and host ports), removes the s2 wget healthcheck, and pins several images. Introduces docker/docker-compose.extras.yml with optional services and an observability stack. Updates package.json Docker scripts to use docker compose with --env-file .env. Updates AGENTS.md, CLAUDE.md, and CONTRIBUTING.md to document the changes and troubleshooting for running multiple local instances.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title accurately summarizes the two main changes: parameterizing Docker host ports for multi-instance support and wiring S2-lite as the default realtime backend.
Description check ✅ Passed The PR description covers the test plan, changelog, and summary sections required by the template, though the checklist items are not explicitly marked as completed.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/s2-lite-oss-onboarding

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ericallam ericallam force-pushed the chore/s2-lite-oss-onboarding branch 2 times, most recently from 83cc59f to 4dbfe1d Compare May 16, 2026 08:47
coderabbitai[bot]

This comment was marked as resolved.

@ericallam ericallam force-pushed the chore/s2-lite-oss-onboarding branch from 4dbfe1d to 123a8a6 Compare May 16, 2026 08:53
@ericallam ericallam changed the title chore: improve s2-lite local-dev onboarding chore: parameterize docker host ports and wire s2-lite by default May 16, 2026
coderabbitai[bot]

This comment was marked as resolved.

@ericallam ericallam force-pushed the chore/s2-lite-oss-onboarding branch from 123a8a6 to 56ba00e Compare May 16, 2026 09:08
@ericallam ericallam marked this pull request as ready for review May 16, 2026 09:10
coderabbitai[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Three small papercuts hitting new contributors:

1. .env.example was silent on realtime-streams S2 config, so fresh clones
   defaulted to v1 (Redis-only) streams and Sessions / chat.agent failed
   with "S2 configuration is missing". The `s2` service in compose
   pre-seeds a `trigger-local` basin (see docker/config/s2-spec.json) —
   wire the env vars to it by default.

2. The s2 healthcheck shelled `wget` on a distroless image, so docker
   always reported it unhealthy even when the API was live. Drop it; no
   service depends on s2.

3. Two clones could not run docker side by side: ports, project name,
   and container names were all hardcoded. Parameterize host ports with
   `${VAR:-default}`, drive the project name via `COMPOSE_PROJECT_NAME`
   (with a `name:` field for backward compat), prefix container names
   with `${CONTAINER_PREFIX:-}`, and pass `--env-file .env` so compose
   reads the same root `.env` the webapp does.

Also split the optional services (electric-shard-1, ch-ui, toxiproxy,
nginx-h2, otel-collector, prometheus, grafana) into
docker-compose.extras.yml + a `docker:full` pnpm script. The core stack
keeps everything the webapp actually needs (postgres, redis, electric,
minio, clickhouse + migrator, s2-lite).
@ericallam ericallam force-pushed the chore/s2-lite-oss-onboarding branch from 56ba00e to 6c39d95 Compare May 16, 2026 12:09
@ericallam ericallam enabled auto-merge (squash) May 16, 2026 12:12
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

View 6 additional findings in Devin Review.

Open in Devin Review

Comment thread package.json Outdated
Comment thread package.json Outdated
Replace the bash-only `$([ -f .env ] && echo --env-file .env)` substitution
in the docker scripts with `scripts/docker.mjs`, a small Node wrapper that
conditionally passes `--env-file <repo-root>/.env` when the file exists.
The substitution form failed on Windows (`cmd.exe` doesn't parse $(...)),
which pnpm uses by default when running scripts on native Windows.

Also routes `internal-packages/clickhouse/db:migrate` through the same
helper so the migrator picks up `COMPOSE_PROJECT_NAME` from `.env` instead
of the hardcoded `-p triggerdotdev-docker`, matching the multi-instance
workflow.
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.

3 participants