test: build docker test services instead of bind-mounting their config#31732
Conversation
Bind mounts resolve on the Docker daemon's filesystem, not the client's. With the darwin CI runners moving to a sidecar-Docker model (daemon in a separate Linux VM, reached over DOCKER_HOST), the bind-mounted init-scripts and config files don't exist on the daemon side, so they silently don't apply (e.g. the postgres init scripts never run -> bun_sql_test role missing). Convert the four services that bind-mounted local files (postgres_plain, postgres_auth, autobahn, squid) to build their own images that COPY the init-scripts/config in. docker build's context ships to the daemon, so this works against both a local and a remote/sidecar daemon. Matches the existing build-based services (postgres_tls, mysql_tls, redis_unified). Verified against a real Linux Docker sidecar: all four build, postgres_plain and postgres_auth come up healthy and the baked init scripts run (bun_sql_test role present).
|
Updated 3:00 PM PT - Jun 2nd, 2026
❌ @alii, your commit 3657b19 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31732That installs a local version of the PR into your bun-31732 --bun |
|
Warning Review limit reached
More reviews will be available in 18 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughDocker test services are migrated from upstream images with mounted config files to self-contained local images embedding those configs; docker-compose is updated to build/use those images, and the test Docker helper now runs compose builds before starting services. ChangesContainerize test services from config files
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/docker/Dockerfile.autobahn`:
- Line 1: The Dockerfile currently uses an unpinned base image reference "FROM
crossbario/autobahn-testsuite" which resolves to latest; update the FROM
instruction to pin a specific tag or digest (e.g., use
"crossbario/autobahn-testsuite:25.10.1" or a content digest) to ensure
deterministic builds—replace the untagged image reference in the Dockerfile with
the chosen tag or digest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b2a417e-643d-4e40-8eaa-7ef973266946
📒 Files selected for processing (5)
test/docker/Dockerfile.autobahntest/docker/Dockerfile.postgres-authtest/docker/Dockerfile.postgres-plaintest/docker/Dockerfile.squidtest/docker/docker-compose.yml
Avoid the implicit :latest; matches the pinned tags the other compose services already use (postgres:15, mysql:8.4, redis:7-alpine, ubuntu/squid:...) for deterministic CI builds.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The build-service whitelists in index.ts only listed the TLS services, so the newly build-based postgres_plain/postgres_auth/autobahn/squid wouldn't be pre-baked by prepare-ci.ts and diverged from the TLS pattern. - buildServices(): bare `compose build` (builds every service with a build: section — nothing to keep in sync as services are added/converted). - doUp(): pre-build the specific service unconditionally; `compose build` is a no-op for image-only services, so no per-service whitelist is needed. Verified against a real sidecar: ensure() works for both an image-only service (redis_plain) and a build-based one (postgres_plain).
There was a problem hiding this comment.
LGTM — the whitelist concern is addressed (bare compose build + unconditional per-service build), and the rest is a mechanical bind-mount → COPY swap that mirrors the existing TLS-service pattern. Remaining README drift is a non-blocking nit.
Extended reasoning...
Overview
This PR converts four test-infrastructure services (postgres_plain, postgres_auth, autobahn, squid) from image: + bind-mounted config to build:-based images that COPY the same files in. It adds four trivial 2–3-line Dockerfiles, updates docker-compose.yml to reference them, and simplifies test/docker/index.ts by replacing the hardcoded build-service whitelist with a bare compose build (and an unconditional per-service build in doUp(), which is a no-op for image-only services). All changes are confined to test/docker/.
Security risks
None. This is test-only infrastructure with no auth, crypto, permissions, or production code paths involved. The Dockerfiles only COPY pre-existing repo files into stock base images.
Level of scrutiny
Low. The change is mechanical and follows the exact pattern already used by postgres_tls/mysql_tls/redis_unified in the same compose file. I cross-checked each new Dockerfile's COPY source/dest against the removed volumes: bind-mount entries — they match one-for-one. The index.ts simplification is the fix I suggested in my earlier review and is strictly more robust than the previous hardcoded list.
Other factors
- Both prior review comments (CodeRabbit's autobahn pin, my build-whitelist concern) have been addressed in d6aa2c5 and 3657b19 respectively; both threads are resolved.
- The CI failures reported by robobun are in
build-cpp/build-rustlinker steps and are unrelated to this PR, which touches onlytest/docker/. - The one remaining open thread is my own README-drift nit, which I explicitly flagged as non-blocking and documentation-only.
- The PR description includes verification against a real remote Docker daemon (the motivating use case).
The README's postgres_plain 'How It Works' example still showed image: + no build, and the architecture tree omitted the new Dockerfile.* files. Update both to match the build-based services this PR introduces.
There was a problem hiding this comment.
LGTM — thanks for addressing the whitelist + README feedback; the bare compose build approach is cleaner than maintaining the list.
Extended reasoning...
Overview
This PR converts four test/docker services (postgres_plain, postgres_auth, autobahn, squid) from image: + bind-mounted config to build:-based local images that COPY their config in. It adds four trivial 2-3 line Dockerfiles, updates docker-compose.yml to reference them, drops the hardcoded build-service whitelists in index.ts in favor of unconditional/bare compose build, and updates the README. Everything is confined to test/docker/ — test infrastructure only, no runtime/library code touched.
Security risks
None. These are local test-only Docker images derived from the same upstream bases the compose file previously pulled directly (postgres:15, crossbario/autobahn-testsuite:25.10.1, ubuntu/squid:5.2-22.04_beta), with the only addition being a COPY of config files that were already bind-mounted before. No new network exposure, credentials, or privilege changes.
Level of scrutiny
Low. This is a mechanical refactor of test scaffolding that follows the exact pattern already in the repo for postgres_tls/mysql_tls/redis_unified. The motivation (bind mounts resolve on the daemon's FS, which breaks against a remote/sidecar Docker host) is well-explained and the fix is the standard solution. The index.ts change replaces a hardcoded service list with a bare compose build (which builds every service that has a build: section) — strictly less brittle than what it replaces. compose build <service> on image-only services is a documented no-op, so the unconditional pre-build in doUp() is safe.
Other factors
All prior review feedback has been addressed in follow-up commits: the autobahn base image is now pinned (d6aa2c5), the hardcoded whitelists in index.ts were removed (3657b19), and the README example/architecture tree were updated (e9c0e64). All inline comments are resolved, the bug-hunting pass found nothing, and no CODEOWNERS rule covers test/docker/. The author verified the change against a real remote Docker daemon. No reason to hold this for further human review.
3 participants
What this does
Converts the four
test/dockerservices that bind-mounted local files —postgres_plain,postgres_auth,autobahn,squid— to build their own images (a small Dockerfile thatCOPYs the init-scripts/config in) instead ofimage:+volumes:bind mounts.Why
Bind mounts resolve on the Docker daemon's filesystem, not the client's. The darwin CI runners are moving to a sidecar-Docker model (the daemon runs in a separate Linux VM, reached over
DOCKER_HOST— Apple Silicon can't run accelerated Docker inside a macOS guest). In that model the bind-mounted./init-scripts/./configfiles don't exist on the daemon side, so they silently don't apply — e.g. the Postgres init scripts never run and thebun_sql_testrole is missing, which then looks like a confusing test failure rather than a setup problem.docker build's context, by contrast, ships to the daemon, so baking the files into the image works against both a local daemon and a remote/sidecar one.This follows the pattern the repo already uses for its TLS services (
postgres_tls,mysql_tls,redis_unifiedare allbuild:-based).Changes
Dockerfile.postgres-plain,Dockerfile.postgres-auth,Dockerfile.autobahn,Dockerfile.squid(eachFROMthe stock image +COPYthe scripts/config).build:+ abun-*:localimage tag; theirvolumes:bind mounts are removed.Compatibility
docker compose upjust builds the image on first run (cached after), then runs identically. Init scripts/config are baked in rather than mounted.docker compose buildto pick up (same as the existing TLS services).Verification
Ran the real compose against an actual Linux Docker sidecar VM (Tart) over
DOCKER_HOST:docker compose configvalid; all four images build (context shipped to the remote daemon).postgres_plain+postgres_authcome up healthy, and the baked init scripts run →bun_sql_testrole present on both (the exact thing that was silently missing with bind mounts).Context
Second piece of the darwin Tart-runner migration; pairs with #31731 (the
BUN_DOCKER_TEST_HOSThost indirection).