feat(getting-started): Dockerfiles and a container e2e workflow - #42
Conversation
The quickstart had no Dockerfile, while the documentation described containerising a Connectum service — so the recommended container setup was never built, let alone run. It also meant `connectum init` handed users a project with no way to containerise it: the CLI clones this example and passes every file through except `pnpm-workspace.yaml` and `.pnpmfile.cjs`, so these Dockerfiles reach every scaffolded project too. Two Dockerfiles rather than one parameterised by build args: this example exists to be copied, and a file with five `ARG`s reads badly for that. Each is three stages — generate (`gen/` is not committed and `buf` is a devDependency), production dependencies, runtime — ending in a non-root user and an h2c-aware healthcheck. The new workflow closes a real gap. Everything in either repository exercises these services in-process; the existing e2e test opens a socket but keeps client and server together, so nothing covered the container itself. The scenario asserts response *bodies* against the documented contract: - the container reaches its own HEALTHCHECK - `/healthz` returns `SERVING` over h2c, and an unknown path returns 404 — the second assertion is what proves the probe is not the vacuous kind - reflection lists the greeter and health methods a client would look up - `SayHello` and `SayGoodbye` return the documented payloads - `grpc.health.v1.Health/Check` returns `SERVING` - SIGTERM as PID 1 exits 0 inside the grace window, rather than being SIGKILLed Both runtimes pass all eleven checks locally. The nightly schedule exists because the images install `@connectum/*` from npm at build time, so a published regression can break this without anything here changing. Two checks were written expecting behaviour this example does not have and were corrected rather than reported: reflection does not advertise itself, and the quickstart proto declares no constraints, so there is no validation path to assert. Not covered, named rather than silently dropped: the tsx execution model (tsx is a devDependency and there is no tsx Dockerfile), the other examples' images, and any broker-backed flow.
|
Warning Review limit reached
Next review available in: 43 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded Node and Bun Docker images for the getting-started service, a container E2E script, container usage documentation, and scheduled or event-driven GitHub Actions coverage. ChangesContainer E2E Coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant ContainerE2E as container-e2e.sh
participant Docker
participant Service as getting-started service
GitHubActions->>Docker: build Node or Bun image
GitHubActions->>ContainerE2E: run wire-level E2E test
ContainerE2E->>Docker: start image
Docker->>Service: launch service
ContainerE2E->>Service: validate health, HTTP/2, reflection, RPC, and gRPC health
ContainerE2E->>Docker: send SIGTERM
Docker-->>ContainerE2E: return exit code 0
GitHubActions-->>GitHubActions: print logs if the test fails
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. 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: 3
🤖 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 `@getting-started/.dockerignore`:
- Line 7: Stop excluding pnpm-lock.yaml in getting-started/.dockerignore, and
update both dependency-install stages in getting-started/Dockerfile (lines 14-15
and 23-24) and getting-started/Dockerfile.bun (lines 15-16 and 24-25) to use the
project lockfile with frozen pnpm installs for full and production dependencies.
In `@scripts/container-e2e.sh`:
- Around line 24-25: Update scripts/container-e2e.sh lines 24-25 so cleanup
preserves the e2e-* container on failure, or captures its logs before removal;
retain cleanup on successful exits. Update .github/workflows/container-e2e.yml
lines 64-66 to collect logs from the preserved container during failure
handling, then remove it if needed.
- Around line 78-83: Update the shutdown checks after docker stop in
scripts/container-e2e.sh to capture its result and verify the container’s
.State.Status is exited before evaluating exit_code or marking shutdown checks
successful. Treat a failed stop or non-exited status as a failure, and only
accept the existing exit-code and elapsed-time assertions after that validation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 41a3933a-b36c-4110-9491-01214002ba98
📒 Files selected for processing (6)
.github/workflows/container-e2e.ymlgetting-started/.dockerignoregetting-started/Dockerfilegetting-started/Dockerfile.bungetting-started/README.mdscripts/container-e2e.sh
| *.md | ||
| .gitignore | ||
| .pnpmfile.cjs | ||
| pnpm-lock.yaml |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files of interest =="
git ls-files | rg '(^|/)getting-started/(Dockerfile|Dockerfile\.bun|\.dockerignore)|^package\.json$|pnpm-lock\.yaml$'
echo
echo "== getting-started/.dockerignore =="
if [ -f getting-started/.dockerignore ]; then cat -n getting-started/.dockerignore; fi
echo
echo "== getting-started/Dockerfile =="
if [ -f getting-started/Dockerfile ]; then cat -n getting-started/Dockerfile; fi
echo
echo "== getting-started/Dockerfile.bun =="
if [ -f getting-started/Dockerfile.bun ]; then cat -n getting-started/Dockerfile.bun; fi
echo
echo "== root package manager files =="
for f in package.json pnpm-lock.yaml getting-started/package.json; do
[ -f "$f" ] && { echo "--- $f"; sed -n '1,220p' "$f"; }
done
echo
echo "== package scripts/packageManager fields =="
python3 - <<'PY'
import json, pathlib
for p in [pathlib.Path('package.json'), pathlib.Path('getting-started/package.json')]:
if not p.exists():
continue
data=json.loads(p.read_text())
print(p, "packageManager=", data.get("packageManager"), "scripts=", data.get("scripts"))
PYRepository: Connectum-Framework/examples
Length of output: 7485
🌐 Web query:
Bun package manager install lockfile mode lockfile only package.json lockfile generation
💡 Result:
To generate a lockfile in the Bun package manager without installing dependencies to node_modules, you can use the --lockfile-only flag [1][2]. When you run bun install --lockfile-only, Bun will resolve dependencies, update the lockfile, and save it to disk (named bun.lock) [1][3]. Note that this process still populates your global install cache with registry metadata and relevant dependency files (git/tarballs) to ensure accuracy [1][3]. Regarding lockfile generation and management: 1. Default Format: As of Bun v1.2.0, the default lockfile is the human-readable, text-based bun.lock [1][4]. 2. Migration: If you have an older binary lockfile (bun.lockb), you can migrate to the new text format by running bun install --save-text-lockfile --frozen-lockfile --lockfile-only, and then deleting the old bun.lockb file [1][2]. 3. Frozen Lockfile: To ensure reproducible builds, use the --frozen-lockfile flag (often used in CI environments with the equivalent command bun ci). This instructs Bun to use the existing lockfile versions exclusively and fail if the package.json and lockfile are out of sync [2][5]. 4. Alternative Formats: If you require a yarn.lock file for compatibility, you can generate it alongside your bun.lock by using the --yarn flag or by configuring it in your bunfig.toml file [1][6]. If you want to prevent the creation of a lockfile entirely during installation, you can use the --no-save flag [1][3].
Citations:
- 1: https://bun.com/docs/pm/lockfile
- 2: https://bun.com/docs/pm/cli/install
- 3: https://bun.sh/docs/pm/lockfile
- 4: https://bun.com/blog/bun-lock-text-lockfile
- 5: https://github.com/oven-sh/bun/blob/6618e7f7/docs/pm/cli/install.mdx
- 6: https://bun.com/docs/guides/install/yarnlock
Use a lockfile for the Docker dependency stages.
getting-started/.dockerignore excludes pnpm-lock.yaml, so both image stages resolve packages again from package.json. A rebuild can get a different dependency graph, and npm can generate a different lockfile than pnpm. Include a project lockfile in the build context, then install from it in frozen mode for both full and production dependency stages.
📍 Affects 3 files
getting-started/.dockerignore#L7-L7(this comment)getting-started/Dockerfile#L14-L15getting-started/Dockerfile#L23-L24getting-started/Dockerfile.bun#L15-L16getting-started/Dockerfile.bun#L24-L25
🤖 Prompt for 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.
In `@getting-started/.dockerignore` at line 7, Stop excluding pnpm-lock.yaml in
getting-started/.dockerignore, and update both dependency-install stages in
getting-started/Dockerfile (lines 14-15 and 23-24) and
getting-started/Dockerfile.bun (lines 15-16 and 24-25) to use the project
lockfile with frozen pnpm installs for full and production dependencies.
| cleanup() { docker rm -f "$NAME" >/dev/null 2>&1 || true; } | ||
| trap cleanup EXIT |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Retain failed test containers until log collection completes.
The EXIT trap removes every e2e-* container before the workflow failure step runs. The docker logs command therefore finds no container and produces no diagnostics.
scripts/container-e2e.sh#L24-L25: preserve the container when the script exits with a failure status, or capture its logs before removal..github/workflows/container-e2e.yml#L64-L66: collect the preserved container logs, then remove the container if required.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 24-24: This function is never invoked. Check usage (or ignored if invoked indirectly).
(SC2329)
📍 Affects 2 files
scripts/container-e2e.sh#L24-L25(this comment).github/workflows/container-e2e.yml#L64-L66
🤖 Prompt for 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.
In `@scripts/container-e2e.sh` around lines 24 - 25, Update
scripts/container-e2e.sh lines 24-25 so cleanup preserves the e2e-* container on
failure, or captures its logs before removal; retain cleanup on successful
exits. Update .github/workflows/container-e2e.yml lines 64-66 to collect logs
from the preserved container during failure handling, then remove it if needed.
…on failure Review found three issues; two were real defects in this script. **The shutdown check could pass on a container that never stopped.** It read `.State.ExitCode` without checking `.State.Status`, and `docker inspect` reports `ExitCode: 0` for a *running* container -- verified directly. So if `docker stop` failed or timed out at the daemon level, both shutdown assertions passed while the service was still up. That is exactly the vacuous-check class this harness exists to catch, so it now requires `docker stop` to succeed and the status to be `exited` before the exit code means anything. Verified both ways: a container ignoring SIGTERM is SIGKILLed and caught by the exit code (137), and a simulated failed stop is caught by the new status guard where the old logic passed. **A failing run destroyed its own evidence.** The `EXIT` trap removed the container, so the workflow's log-capture step -- and anyone debugging locally -- found nothing. Logs are now dumped before removal when a check has failed. Assertions moved through a single `check` helper, replacing the `A && B || C` pattern that is not if-then-else (SC2015). The third comment asked for a committed lockfile with frozen installs. Declined: car-sharing and hris deliberately ship without one so the images track the latest compatible `@connectum/*` rather than being bit-reproducible. Both Dockerfiles now carry that rationale, as those two already did, so the choice reads as deliberate.
|
Thanks — two of the three were real defects in the script, and one is a deliberate project convention. All addressed. Shutdown assertion could pass on a container that never stopped — valid, and the more serious of the two. I verified the premise directly: It now requires
Cleanup destroyed the evidence — valid. The Assertions also moved through a single Lockfile with frozen installs — declined, with the rationale now written into both Dockerfiles. |
1 participant
Two gaps this closes
The quickstart had no Dockerfile — while the documentation described containerising a Connectum service. The recommended setup was never built, let alone run. It also meant
connectum inithanded users a project with no way to containerise it: the CLI clones this example and passes every file through exceptpnpm-workspace.yamland.pnpmfile.cjs, so these Dockerfiles reach every scaffolded project too.Nothing tested a container. Everything in either repository exercises these services in-process — the existing e2e test opens a real socket but keeps client and server together — so the HEALTHCHECK, the production dependency closure with devDependencies stripped, and SIGTERM handling as PID 1 were all untested.
The Dockerfiles
Two files rather than one parameterised by build args: this example exists to be copied, and a file with five
ARGs reads badly for that. Each is three stages — generate (gen/is not committed andbufis a devDependency), production dependencies, runtime — ending in a non-root user and an h2c-aware healthcheck.The scenario
scripts/container-e2e.shruns from the host against the published port and asserts response bodies against the documented contract, not merely that a call did not fail:/healthzreturnsSERVINGover h2cSayHelloreturns"Hello, Ada!"grpc.health.v1.Health/CheckreturnsSERVINGSayGoodbyereturns"Goodbye, Ada!"Both runtimes pass all eleven assertions locally (Node and Bun images built and run).
The nightly schedule exists because the images install
@connectum/*from npm at build time, so a published regression can break this without anything in the repository changing.Honesty notes
Two checks were written expecting behaviour this example does not have, and were corrected rather than reported as findings: gRPC reflection does not advertise itself (normal), and the quickstart proto declares no constraints and the package has no validation dependency, so there is no validation path to assert.
Not covered, named rather than silently dropped: the tsx execution model (tsx is a devDependency and there is no tsx Dockerfile), the other examples' images, and any broker-backed flow.
This is the first test workflow in this repository — until now it only had
auto-label.Summary by CodeRabbit
New Features
Documentation
Tests