chore(repo): add docker smoke tests #7603
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Docker smoke-test GitHub Actions workflow that dynamically selects app targets based on changed paths, plus two Bash scripts: one to benchmark Docker builds (cold vs warm) and another to build, run, and probe containerized Next.js apps (web, admin, space, live) with up/down lifecycle and readiness checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer/PR
participant GA as GitHub Actions
participant PF as paths-filter
participant GS as github-script
participant SM as scripts/smoke.sh
participant DK as Docker
Dev->>GA: Push/PR triggers workflow
GA->>PF: Detect changed paths
PF-->>GA: Changed components map
GA->>GS: Build matrix (targets with config)
alt has_targets
GA->>SM: smoke up (per target)
SM->>DK: build images (DOCKER_BUILDKIT=1)
SM->>DK: run containers (port mapping)
SM->>SM: probe readiness (HTTP 2xx/3xx)
alt failure
SM->>DK: fetch container logs
SM-->>GA: non-zero exit
else success
SM-->>GA: success for target
end
GA->>SM: smoke down (cleanup)
else no targets
GA-->>Dev: Skip smoke job
end
sequenceDiagram
autonumber
actor Eng as Engineer
participant BB as scripts/bench-builds.sh
participant DK as Docker
participant BX as buildx (optional)
Eng->>BB: Run with selected services
BB->>DK: Validate Docker availability
opt prune (unless disabled)
BB->>BX: buildx prune/du (if available)
BB->>DK: docker system prune/df (fallback)
end
loop per service
BB->>DK: Cold build (no-cache) -> time, size
BB->>DK: Warm build (with cache) -> time
end
BB-->>Eng: Summary table (times, sizes, cache notes)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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 |
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
from
74c8564 to
62d51c9
Compare
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (15)
apps/live/Dockerfile.live (4)
57-59: Align user/group and make group explicitMinor hardening/nit:
- Ensure the
nextjsuser is in thenodejsgroup so UID/GID pairing is explicit.- Optionally set
USERby uid:gid for clarity.Proposed change:
RUN addgroup --system --gid 1001 nodejs -RUN adduser --system --uid 1001 nextjs +RUN adduser --system --uid 1001 -G nodejs nextjs ... -USER nextjs +USER 1001:1001Also applies to: 70-70
61-63: Consider pruning to production-only dependencies for a slimmer runtimeCurrently,
node_modulesfrom the installer stage likely includes dev deps. For this non-Next runtime, you can reduce image size by installing/pruning production-only deps in the installer stage before copying.Add this in the installer stage (outside this range):
# Prefer production-only deps in the final artifact # Option A: install with prod-only directly RUN --mount=type=cache,id=pnpm-store,target=/pnpm/store \ pnpm install --offline --frozen-lockfile --prod --store-dir=/pnpm/store # Option B: if you need dev deps for build, then prune after build: # RUN pnpm turbo run build --filter=live # RUN pnpm prune --prod
65-66: Set NODE_ENV=production in runnerAdds sensible defaults and can improve performance.
ENV TURBO_TELEMETRY_DISABLED=1 +ENV NODE_ENV=production ENV NEXT_TELEMETRY_DISABLED=1
68-73: Optional: Add a container HEALTHCHECKUseful for local smoke tests and orchestrators.
EXPOSE 3000 USER 1001:1001 ENTRYPOINT ["dumb-init", "--"] +HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=5 \ + CMD wget -qO- http://localhost:3000/live/health > /dev/null || exit 1 CMD ["node", "live/server.js"]apps/admin/Dockerfile.admin (3)
79-83: Make nextjs a member of nodejs group; ensure explicit uid:gidSmall hardening and clarity improvement.
RUN addgroup --system --gid 1001 nodejs -RUN adduser --system --uid 1001 nextjs +RUN adduser --system --uid 1001 -G nodejs nextjs # Ensure writable Next.js cache directory RUN mkdir -p /app/apps/admin/.next/cache/fetch-cache && chown -R 1001:1001 /app/apps/admin/.next -USER nextjs +USER 1001:1001
109-111: Set NODE_ENV=production in runnerHelps ensure Next.js runs optimized for production.
ENV NEXT_TELEMETRY_DISABLED=1 ENV TURBO_TELEMETRY_DISABLED=1 +ENV NODE_ENV=production
112-116: Optional: Add a HEALTHCHECKSimple HTTP probe to the admin base path.
EXPOSE 3000 ENTRYPOINT ["dumb-init", "--"] +HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=5 \ + CMD wget -qO- http://localhost:3000/god-mode > /dev/null || exit 1 CMD ["node", "apps/admin/server.js"]apps/space/Dockerfile.space (3)
80-82: Ensure nextjs joins nodejs group and use explicit uid:gidKeeps ownership semantics clear and consistent across files.
RUN addgroup --system --gid 1001 nodejs -RUN adduser --system --uid 1001 nextjs +RUN adduser --system --uid 1001 -G nodejs nextjs ... -USER nextjs +USER 1001:1001Also applies to: 92-92
112-114: Set NODE_ENV=production in runnerToggles Next.js into production mode explicitly.
ENV NEXT_TELEMETRY_DISABLED=1 ENV TURBO_TELEMETRY_DISABLED=1 +ENV NODE_ENV=production
115-120: Optional: Add a HEALTHCHECKBasic liveness for the space app:
EXPOSE 3000 ENTRYPOINT ["dumb-init", "--"] +HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=5 \ + CMD wget -qO- http://localhost:3000/spaces > /dev/null || exit 1 CMD ["node", "apps/space/server.js"]apps/web/Dockerfile.web (2)
129-131: Set NODE_ENV=production in runnerExplicitly set production mode for Next.js runtime.
ENV NEXT_TELEMETRY_DISABLED=1 ENV TURBO_TELEMETRY_DISABLED=1 +ENV NODE_ENV=production
132-136: Optional: Add a HEALTHCHECKAdds a basic HTTP liveness probe to the root path.
EXPOSE 3000 ENTRYPOINT ["dumb-init", "--"] +HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=5 \ + CMD wget -qO- http://localhost:3000/ > /dev/null || exit 1 CMD ["node", "apps/web/server.js"].github/workflows/docker-smoke.yml (3)
45-58: Paths filter patterns: drop leading './' for consistency and to avoid false negativesSome tools interpret leading './' differently. Using repo-relative globs without './' aligns with the trigger filters above and reduces risk of mismatches.
filters: | web: - - './apps/web/**' + - 'apps/web/**' space: - - './apps/space/**' + - 'apps/space/**' admin: - - './apps/admin/**' + - 'apps/admin/**' live: - - './apps/live/**' + - 'apps/live/**' common: - 'turbo.json' - 'pnpm-lock.yaml' - 'pnpm-workspace.yaml' - '.github/workflows/docker-smoke.yml'
105-109: Optionally pass build args to make smoke builds configurableIf you ever need to validate base paths/URLs in smoke builds, wire through the NEXT_PUBLIC_* args. Defaults keep current behavior.
- docker build -f ${{ matrix.dockerfile }} -t ${{ matrix.image }} . + docker build \ + -f ${{ matrix.dockerfile }} \ + --build-arg NEXT_PUBLIC_API_BASE_URL=${NEXT_PUBLIC_API_BASE_URL:-} \ + --build-arg NEXT_PUBLIC_ADMIN_BASE_URL=${NEXT_PUBLIC_ADMIN_BASE_URL:-} \ + --build-arg NEXT_PUBLIC_ADMIN_BASE_PATH=${NEXT_PUBLIC_ADMIN_BASE_PATH:-/god-mode} \ + --build-arg NEXT_PUBLIC_LIVE_BASE_URL=${NEXT_PUBLIC_LIVE_BASE_URL:-} \ + --build-arg NEXT_PUBLIC_LIVE_BASE_PATH=${NEXT_PUBLIC_LIVE_BASE_PATH:-/live} \ + --build-arg NEXT_PUBLIC_SPACE_BASE_URL=${NEXT_PUBLIC_SPACE_BASE_URL:-} \ + --build-arg NEXT_PUBLIC_SPACE_BASE_PATH=${NEXT_PUBLIC_SPACE_BASE_PATH:-/spaces} \ + --build-arg NEXT_PUBLIC_WEB_BASE_URL=${NEXT_PUBLIC_WEB_BASE_URL:-} \ + -t ${{ matrix.image }} .
115-136: Improve diagnostics on failures by printing response body on the last attemptHelps triage issues when status != 200.
for i in {1..60}; do STATUS="$(curl -sS -o /dev/null -w "%{http_code}" -L "${URL}" || true)" if [ "${STATUS}" = "200" ]; then echo "Success: HTTP ${STATUS} from ${URL}" exit 0 fi echo "Attempt ${i}: HTTP ${STATUS} (waiting 2s)" sleep 2 done echo "Failed to get HTTP 200 from ${URL}" + echo "::group::Last response body (${URL})" + curl -sS -L "${URL}" | sed -n '1,200p' || true + echo "::endgroup::" echo "::group::Container logs (${{ matrix.container }})" docker logs ${{ matrix.container }} || true echo "::endgroup::" exit 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/docker-smoke.yml(1 hunks)apps/admin/Dockerfile.admin(2 hunks)apps/live/Dockerfile.live(1 hunks)apps/space/Dockerfile.space(3 hunks)apps/web/Dockerfile.web(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and smoke test live
- GitHub Check: Build and smoke test web
- GitHub Check: Build and smoke test space
- GitHub Check: Build and smoke test admin
- GitHub Check: Analyze (javascript)
🔇 Additional comments (11)
apps/live/Dockerfile.live (1)
54-54: Good adoption of dumb-init and proper PID 1 handlingUsing dumb-init as ENTRYPOINT will correctly forward signals and reap zombies. This should improve shutdown behavior in orchestrators.
Also applies to: 72-73
apps/admin/Dockerfile.admin (3)
61-68: Build-time env propagation looks goodWriting NEXT_PUBLIC_* into .env.production (and copying to .env) is a pragmatic way to ensure values are present at build-time and runtime. Keep in mind these values are public by design, so this is safe.
76-76: Good: Using dumb-init and an ENTRYPOINT wrapperThis will improve signal handling and shutdown behavior of the Next.js server.
Also applies to: 114-115
87-90: COPY with ownership is correct for non-root runtimeEnsuring correct ownership on standalone, static, and public assets prevents permission issues after dropping privileges. Well done.
apps/space/Dockerfile.space (3)
61-68: Build-time NEXT_PUBLIC env generation is solidMirrors the web/admin pattern; good for consistent build-time values.
77-78: dumb-init and ENTRYPOINT look goodSignal forwarding and zombie reaping in place.
Also applies to: 117-118
85-88: COPY with --chown is correctPrevents permission issues after switching to non-root. Nice consistency with other apps.
apps/web/Dockerfile.web (3)
71-81: Build-time env generation for Next.js is well doneClear, explicit mapping of NEXT_PUBLIC_* values. This matches the approach in space/admin and keeps behavior consistent.
89-89: Good: dumb-init installed and used as ENTRYPOINTPID 1 handled properly; improves signal handling in containers.
Also applies to: 134-135
95-96: Permissions and ownership look correct for non-root runtime
- Preparing a writable Next.js cache avoids runtime write errors.
- COPY with --chown ensures the files are owned by the runtime user.
Also applies to: 101-104
.github/workflows/docker-smoke.yml (1)
28-34: Dynamic matrix generation is clear and robustGood use of dorny/paths-filter and fallback to web when no targets detected.
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
3 times, most recently
from
663c1be to
08e0ce7
Compare
lifeiscontent
requested review from
aaryan610,
prateekshourya29 and
sriramveeraghanta
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
5 times, most recently
from
59d118a to
c266814
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/live/src/server.ts (1)
26-29: Initialization race: async setupHocusPocus is not awaited before routes use itsetupHocusPocus() is async but not awaited. A WS connection that arrives before hocuspocusServer resolves will throw (handleConnection on undefined). Mount routes only after the server is ready.
Minimal change within current structure:
this.setupMiddleware(); - this.setupHocusPocus(); - this.setupRoutes(); + // Mount routes only after HocusPocus is ready + void this.setupHocusPocus(); // setupRoutes() will be called after initAnd in setupHocusPocus (see fix below on Lines 47-51) call setupRoutes() after successful initialization.
🧹 Nitpick comments (13)
scripts/bench-builds.sh (3)
176-191: Capture build logs on failure (optional), enable verbose mode for diagnosticsAll build output is redirected to /dev/null, making failures hard to debug. Consider a
--verbose(orVERBOSE=1) mode that streams logs, and on failures, save logs per service in a temp file and print its path.Example tweak:
- if [[ "$cold" == "true" ]]; then - DOCKER_BUILDKIT=1 docker build --no-cache --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR" >/dev/null - else - DOCKER_BUILDKIT=1 docker build --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR" >/dev/null - fi + local out="/dev/null" + [[ "${VERBOSE:-0}" == "1" ]] && out="/dev/stdout" + if [[ "$cold" == "true" ]]; then + DOCKER_BUILDKIT=1 docker build --no-cache --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR" >"$out" + else + DOCKER_BUILDKIT=1 docker build --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR" >"$out" + fi
247-254: Be forgiving with user input: trim spaces and allow case-insensitive service namesA trailing space in
--services "web, admin"currently causes a lookup failure. Normalize names to lowercase and trim spaces to reduce friction.Apply this diff:
# Normalize service list -IFS=',' read -r -a SELECTED <<< "$SERVICES" +IFS=',' read -r -a SELECTED <<< "$SERVICES" +# trim spaces and lowercase +for i in "${!SELECTED[@]}"; do + s="${SELECTED[$i]}" + s="${s//[[:space:]]/}" + SELECTED[$i]="${s,,}" +done
266-289: Pruning before every cold build isolates results but can be very slow; consider an opt-in “prune-once” modeYou already support
--no-pruneand--aggressive-prune. A--prune-onceoption could strike a balance: prune once at the start, then run cold builds for all services without re-pruning.If desirable, I can draft a follow-up patch adding
--prune-oncewith clear precedence over existing flags.scripts/smoke.sh (3)
28-33: Rename misleading comment: these are runtime envs, not build args.The header says “Build args”, but you’re exporting env vars to containers at runtime (not Docker build-args). This can confuse maintainers.
Apply this diff:
-# Build args (override to change the built-in defaults) -# e.g. ADMIN_BASE_PATH="/admin" SPACE_BASE_PATH="/my-space" +# Base path runtime env (override defaults) +# e.g. ADMIN_BASE_PATH="/admin" SPACE_BASE_PATH="/my-space"
136-146: Expose base-path env overrides in --help.Documenting the runtime base-path envs in the help output improves discoverability and aligns with how containers are started.
Apply this diff:
Images: WEB_IMAGE=${WEB_IMAGE}, ADMIN_IMAGE=${ADMIN_IMAGE}, SPACE_IMAGE=${SPACE_IMAGE}, LIVE_IMAGE=${LIVE_IMAGE} Names: WEB_CONT=${WEB_CONT}, ADMIN_CONT=${ADMIN_CONT}, SPACE_CONT=${SPACE_CONT}, LIVE_CONT=${LIVE_CONT} + Base: ADMIN_BASE_PATH=${ADMIN_BASE_PATH}, SPACE_BASE_PATH=${SPACE_BASE_PATH}, LIVE_BASE_PATH=${LIVE_BASE_PATH}
185-186: Also check Docker daemon availability, not just CLI presence.
have dockerpasses even if the daemon is not running, causing later commands to fail noisily. Add a lightweight daemon check.Apply this diff:
-have docker || die "Docker is required. Please install Docker and ensure the daemon is running." +have docker || die "Docker CLI is required. Please install Docker." +docker info >/dev/null 2>&1 || die "Docker daemon is not reachable. Start Docker and try again."apps/live/package.json (1)
41-41: I’ve run a deeper inspection of yourapps/livesetup:
Dependencies:
- pino: 9.9.0
- morgan: 1.10.1
- pino-http: not present in
dependenciesCode references:
- No
require('pino')orimport pinoanywhere.- No
require('pino-http')orimport pino-http.- No
require('morgan')ormorgan(calls in any.js/.tsfiles.This tells us:
- You’ve added pino to package.json, but haven’t wired it into the code yet.
- pino-http isn’t installed or referenced—if you intend HTTP middleware, you need to add and configure it.
- morgan is declared but unused in code and can be removed to avoid dead deps.
Next steps:
• If your goal is application-level logging only (no HTTP logger middleware):
- Remove
"morgan": "1.10.1"fromapps/live/package.json.- Import and instantiate pino where needed (e.g.
const logger = require('pino')()).• If you intend to use Pino for HTTP requests:
- Add
"pino-http": "^10.x"todependencies.- In your server setup, replace any morgan middleware with:
const pino = require('pino'); const pinoHttp = require('pino-http'); const logger = pinoHttp({ logger: pino(/* options */) }); app.use(logger);- Remove morgan afterward.
Please verify which approach matches your requirements, wire up pino (and pino-http if needed), and trim the unused morgan dependency.
apps/live/src/server.ts (6)
23-25: applyTo(router) is unused — either use router.ws(...) or drop applyToYou attach express-ws to both app and router but register the WS route on app. This extra applyTo is redundant unless you plan to use router.ws.
Apply one of the following diffs:
Option A — keep app.ws, drop applyTo:
- const wsInstance = expressWs(this.app); - wsInstance.applyTo(this.router); + expressWs(this.app);Option B — prefer router.ws (base path comes from the router mount):
- const wsInstance = expressWs(this.app); - wsInstance.applyTo(this.router); + const wsInstance = expressWs(this.app); + wsInstance.applyTo(this.router);…and update the route below (see comment on Lines 58-66).
58-66: Prefer router.ws("/collaboration") to avoid duplicating basePath logicYou already mount the router at LIVE_BASE_PATH and applied express-ws to the router. Using app.ws with a recomputed basePath duplicates configuration and can drift.
- const basePath = process.env.LIVE_BASE_PATH || "/live"; - this.app.ws(`${basePath}/collaboration`, (ws: any, req: Request) => { + this.router.ws(`/collaboration`, (ws: any, req: Request) => { try { this.hocuspocusServer.handleConnection(ws, req); } catch (err) { - logger.error(err, "WebSocket connection error:"); + logger.error(err, "WebSocket connection error"); ws.close(); } });
86-90: Solid error handler for convert endpointError-first logging plus a generic 500 response is appropriate. Consider attaching a correlation/request id from httpLogger for easier tracing, if available.
101-104: Startup log: include basePath and WS path for operabilityAdd structured context so logs show where to probe health and WS.
- logger.info(`Plane Live server has started at port ${this.app.get("port")}`); + const port = this.app.get("port"); + const basePath = process.env.LIVE_BASE_PATH || "/live"; + logger.info({ port, basePath, wsPath: `${basePath}/collaboration` }, "Plane Live server started");
109-115: Graceful shutdown exits with code 1 — consider making the exit code explicitDestroy always exits(1), which is fine for fatal paths (unhandled errors) but brittle if reused for normal shutdown. Pass an exitCode or let the caller decide.
- public async destroy() { + public async destroy(exitCode = 1) { // Close the HocusPocus server WebSocket connections await this.hocuspocusServer.destroy(); logger.info("HocusPocus server WebSocket connections closed gracefully."); // Close the Express server this.serverInstance.close(() => { logger.info("Express server closed gracefully."); - process.exit(1); + process.exit(exitCode); }); }And when called in error handlers (below), pass 1 explicitly; for future normal shutdown (e.g., SIGTERM), pass 0.
122-126: Global fatal handlers look good; consider also handling SIGTERM/SIGINTYou already cover unhandledRejection/uncaughtException. Adding SIGTERM/SIGINT improves graceful exits in containers and local dev.
Additional snippet (outside current ranges):
process.once("SIGTERM", async () => { logger.info("SIGTERM received — shutting down gracefully"); await server.destroy(0); }); process.once("SIGINT", async () => { logger.info("SIGINT received — shutting down gracefully"); await server.destroy(0); });Also applies to: 129-133
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.github/workflows/docker-smoke.yml(1 hunks)apps/live/Dockerfile.live(1 hunks)apps/live/package.json(2 hunks)apps/live/src/core/extensions/index.ts(6 hunks)apps/live/src/core/helpers/error-handler.ts(1 hunks)apps/live/src/core/helpers/logger.ts(1 hunks)apps/live/src/core/lib/authentication.ts(2 hunks)apps/live/src/core/lib/page.ts(4 hunks)apps/live/src/server.ts(8 hunks)apps/live/tsconfig.json(1 hunks)apps/live/tsup.config.ts(1 hunks)scripts/bench-builds.sh(1 hunks)scripts/smoke.sh(1 hunks)turbo.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/live/src/core/helpers/error-handler.ts
- apps/live/Dockerfile.live
- apps/live/tsconfig.json
- apps/live/tsup.config.ts
- apps/live/src/core/lib/page.ts
- turbo.json
- apps/live/src/core/helpers/logger.ts
- apps/live/src/core/extensions/index.ts
- .github/workflows/docker-smoke.yml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/bench-builds.sh
[warning] 120-120: unit appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (13)
scripts/bench-builds.sh (6)
296-307: Nice, concise summary outputThe fixed-width table plus humanized durations/sizes reads well and is CI-log friendly.
59-61: Nit: use stderr for die(), timestamps for log() — both already goodMinor kudos:
die()writes to stderr andlog()timestamps messages. No action needed.
33-38: Paths and tags mapping look consistent with the repo’s app setThe mapping aligns with web/admin/space/live and distinct bench tags.
193-213: Clear help text and examplesUsage and options are unambiguous; good developer ergonomics.
63-86: Timing functions verified and correctThe
now_nsandelapsed_mshelpers have been sanity-checked using the provided script, measuring ~105 ms for a 100 ms sleep on your environment. No changes are needed here.
113-132: Remove unusedunitfrombuild_cache_size_str
- scripts/bench-builds.sh (around line 120): update the
localdeclaration- local line size unit + local line sizeShellcheck no longer reports SC2034 after dropping
unit.scripts/smoke.sh (4)
50-55: Nice: small, reusable utilities with strict mode.The
have,log, anddiehelpers paired withset -Eeuo pipefailmake the script robust and readable. Good discipline.
187-199: Good: per-app Dockerfiles with explicit tags and idempotent rebuilds.Clear naming for images/containers and pre-run cleanup via
ensure_not_runningmakes iterative testing smooth.
211-225: Solid readiness probing with timeout and backoff.
wait_for_urlis clean, logs well, and leaves containers up for inspection. With the regex fix above, this will be reliable.
234-246: Output UX is helpful.The summarized
docker pstable and ready-to-use URLs are exactly what folks need during smoke testing.apps/live/src/core/lib/authentication.ts (1)
4-4: Centralized logger import looks goodImporting the shared logger via the ESM path with .js extension aligns with the new build/runtime layout. No issues.
apps/live/package.json (1)
8-8: Package privacy retained"private": true remains — good for a non-published package.
apps/live/src/server.ts (1)
37-37: HTTP logger middleware placement is fineMounted early in the chain and before routers — good for end-to-end request timing. Keep it before body parsers if you don’t need to log parsed bodies.
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
from
c266814 to
a28ee72
Compare
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
2 times, most recently
from
4568e40 to
f1e290e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
scripts/bench-builds.sh (5)
88-105: Simplify and harden human_bytes() (avoid awk log() pitfalls, handle 0 robustly)Current mix of integer loop + log recompute is brittle and can break on some awk variants.
human_bytes() { - # Convert integer bytes to human readable - local bytes="$1" - local units=("B" "KB" "MB" "GB" "TB" "PB") - local idx=0 - local value="$bytes" - while (( value >= 1024 && idx < ${#units[@]}-1 )); do - value=$(( value/1024 )) - idx=$(( idx+1 )) - done - # value is integer; but to preserve more precision, recompute with awk - awk -v b="$bytes" -v i="$idx" 'BEGIN { - split("B KB MB GB TB PB", u, " "); - val=b; - while (val>=1024 && i>0){ val/=1024; i-- } - printf("%.2f %s", val, u[(int((log(b)/log(1024)))+1)]); - }' 2>/dev/null || printf "%d %s" "$value" "${units[$idx]}" + # Convert integer bytes to human-readable (two decimals) + local bytes="${1:-0}" + awk -v b="$bytes" 'BEGIN { + if (b < 0) b = 0 + split("B KB MB GB TB PB", u, " ") + i = 1 + while (b >= 1024 && i < 6) { b /= 1024; i++ } + printf("%.2f %s", b, u[i]) + }' 2>/dev/null || printf "%s %s" "${bytes}" "B" }
218-241: Resolve conflicting flags: --no-prune vs --aggressive-pruneCurrently both can be set and behavior is implicit. Make precedence explicit.
done + +# Resolve conflicting options +if [[ "$AGGRESSIVE_PRUNE" == "true" && "$NO_PRUNE" == "true" ]]; then + log "Both --no-prune and --aggressive-prune specified; ignoring --no-prune." + NO_PRUNE="false" +fi
245-246: Validate Docker daemon availability, not just CLIThe CLI may exist while the daemon is down; fail fast with a clearer error.
-have docker || die "Docker is required. Please install Docker and ensure the daemon is running." +have docker || die "Docker is required. Please install Docker and ensure the daemon is running." +docker info >/dev/null 2>&1 || die "Docker daemon is not running or not reachable. Start Docker Desktop or your Docker service."
26-26: Require Bash ≥ 4 early (associative arrays in use)The script uses declare -A; macOS default bash 3.2 will fail.
set -Eeuo pipefail + +# Require bash >= 4 for associative arrays (macOS ships 3.2 by default) +if (( BASH_VERSINFO[0] < 4 )); then + echo "ERROR: bash >= 4 is required. On macOS, install via Homebrew: brew install bash && chsh -s /usr/local/bin/bash" >&2 + exit 1 +fi
63-70: now_ns() misdetects BSD date nanoseconds — returns “%N” and breaks mathOn macOS, date +%s%N echoes “%N” with exit 0. Validate numerically and add robust fallbacks.
now_ns() { - # Prefer nanoseconds if supported, fallback to seconds*1e9 - if date +%s%N >/dev/null 2>&1; then - date +%s%N - else - echo "$(( $(date +%s) * 1000000000 ))" - fi + # Prefer GNU date; then BSD date if it truly supports %N; else EPOCHREALTIME/perl/seconds + if command -v gdate >/dev/null 2>&1; then + local t + t="$(gdate +%s%N 2>/dev/null || true)" + [[ "$t" =~ ^[0-9]+$ ]] && { echo "$t"; return; } + fi + local t + t="$(date +%s%N 2>/dev/null || true)" + if [[ "$t" =~ ^[0-9]+$ ]]; then + echo "$t"; return + fi + if [[ -n "${EPOCHREALTIME-}" ]]; then + local sec="${EPOCHREALTIME%.*}" frac="${EPOCHREALTIME#*.}" + frac="${frac}000000000"; frac="${frac:0:9}" + printf '%s%s\n' "$sec" "$frac"; return + fi + if command -v perl >/dev/null 2>&1; then + perl -MTime::HiRes=time -e 'printf("%.0f\n", time()*1e9)'; return + fi + echo "$(( $(date +%s) * 1000000000 ))" }scripts/smoke.sh (3)
61-64: Fix 2xx/3xx detection; current regex has precedence bug (accepts “^2” OR “3xx”)Use a case pattern or anchored regex; also consider following redirects with -L.
- local code - code="$(curl -fsS -o /dev/null -w "%{http_code}" "$url" || true)" - [[ "$code" =~ ^2|3[0-9]{2}$ ]] && return 0 + local code + code="$(curl -fsSL -o /dev/null -w "%{http_code}" "$url" || true)" + case "$code" in + 2??|3??) return 0 ;; + *) return 1 ;; + esac
189-189: CLI --services should override the environment variable SERVICESCurrent precedence favors env over CLI, which is surprising.
-IFS=',' read -r -a SELECTED <<< "${SERVICES:-$services}" +IFS=',' read -r -a SELECTED <<< "${services:-$SERVICES}"
201-208: Add default branch in run loop to fail fast on unknown servicesWithout a default, typos are silently ignored during “run” when not building.
case "$svc" in web) run_container "$WEB_CONT" "$WEB_IMAGE" "$WEB_PORT" ;; admin) run_container "$ADMIN_CONT" "$ADMIN_IMAGE" "$ADMIN_PORT" -e NEXT_PUBLIC_ADMIN_BASE_PATH="${ADMIN_BASE_PATH}" ;; space) run_container "$SPACE_CONT" "$SPACE_IMAGE" "$SPACE_PORT" -e NEXT_PUBLIC_SPACE_BASE_PATH="${SPACE_BASE_PATH}" ;; live) run_container "$LIVE_CONT" "$LIVE_IMAGE" "$LIVE_PORT" -e LIVE_BASE_PATH="${LIVE_BASE_PATH}" ;; + *) die "Unknown service: $svc" ;; esac
🧹 Nitpick comments (8)
.github/workflows/docker-smoke.yml (5)
47-61: paths-filter patterns may not match due to leading './' — drop the prefix to be safedorny/paths-filter patterns are typically repo-root relative. The leading './' can prevent matches depending on how the action normalizes paths. This could degrade the matrix to default “web” only on PRs when other apps changed.
Apply:
web: - - './apps/web/**' + - 'apps/web/**' space: - - './apps/space/**' + - 'apps/space/**' admin: - - './apps/admin/**' + - 'apps/admin/**' live: - - './apps/live/**' + - 'apps/live/**' common: - 'turbo.json' - 'pnpm-lock.yaml' - 'pnpm-workspace.yaml' - '.github/workflows/docker-smoke.yml' - - './packages/**' + - 'packages/**'If you prefer to keep './', please confirm with a quick test run that the filters fire as expected on PRs.
124-133: Accept any 2xx HTTP for smoke readiness (not just 200)Some endpoints might legitimately return 204 or 206. You already follow redirects (-L), so broad 2xx is reasonable.
- if [ "${STATUS}" = "200" ]; then + case "${STATUS}" in + 2??) echo "Success: HTTP ${STATUS} from ${URL}" - exit 0 - fi + exit 0 + ;; + esac
108-112: Consider always pulling the latest base image in CI buildsAdding --pull ensures you’re not building atop stale base layers, improving reproducibility and security posture.
- docker build -f ${{ matrix.dockerfile }} -t ${{ matrix.image }} . + docker build --pull -f ${{ matrix.dockerfile }} -t ${{ matrix.image }} .
118-139: Broaden failure diagnostics (inspect, top, last 200 log lines)On failures, add concise diagnostics to speed root cause analysis.
echo "Failed to get HTTP 200 from ${URL}" echo "::group::Container logs (${{ matrix.container }})" - docker logs ${{ matrix.container }} || true + docker logs --tail=200 ${{ matrix.container }} || true echo "::endgroup::" + echo "::group::Container inspect (${{ matrix.container }})" + docker inspect ${{ matrix.container }} | sed -n '1,200p' || true + echo "::endgroup::" + echo "::group::Container processes (${{ matrix.container }})" + docker top ${{ matrix.container }} || true + echo "::endgroup::" exit 1
1-2: Tighten workflow permissions and prevent overlapping runsLeast-privilege token and a concurrency group help safety and save minutes.
name: Docker build and smoke test for apps + +permissions: + contents: read + +concurrency: + group: docker-smoke-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: truescripts/bench-builds.sh (1)
114-131: ShellCheck SC2034: remove unused ‘unit’ variable and minor parsing nitThe declared unit variable is unused; drop it to silence SC2034.
- local line size unit + local line sizescripts/smoke.sh (2)
184-186: Also verify Docker daemon availabilityMirror the bench script recommendation: check docker info, not just CLI presence.
-# Check Docker is available -have docker || die "Docker is required. Please install Docker and ensure the daemon is running." +# Check Docker is available +have docker || die "Docker is required. Please install Docker and ensure the daemon is running." +docker info >/dev/null 2>&1 || die "Docker daemon is not running or not reachable. Start Docker Desktop or your Docker service."
193-199: Next.js basePath usually bakes in at build time — passing NEXT_PUBLIC_ at runtime may be ineffective*If admin/space compute basePath at build (next build) from process.env, setting NEXT_PUBLIC_*_BASE_PATH via docker run won’t affect already-built output. If you rely on runtime injection, confirm the apps read env at runtime (e.g., server config or experimental runtime env).
Two options:
- Build-time args: add ARG/ENV in the Dockerfiles and pass --build-arg in build_image when svc is admin/space.
- Or switch to a runtime-friendly config pattern that reads env on boot.
Example (script-side build-time injection):
- admin) build_image "admin" "$ADMIN_IMAGE" "apps/admin/Dockerfile.admin" ;; - space) build_image "space" "$SPACE_IMAGE" "apps/space/Dockerfile.space" ;; + admin) DOCKER_BUILDKIT=1 docker build --progress=plain \ + --build-arg NEXT_PUBLIC_ADMIN_BASE_PATH="${ADMIN_BASE_PATH}" \ + -f "apps/admin/Dockerfile.admin" -t "$ADMIN_IMAGE" "$ROOT_DIR" ;; + space) DOCKER_BUILDKIT=1 docker build --progress=plain \ + --build-arg NEXT_PUBLIC_SPACE_BASE_PATH="${SPACE_BASE_PATH}" \ + -f "apps/space/Dockerfile.space" -t "$SPACE_IMAGE" "$ROOT_DIR" ;;And in the Dockerfiles:
ARG NEXT_PUBLIC_ADMIN_BASE_PATH ENV NEXT_PUBLIC_ADMIN_BASE_PATH=$NEXT_PUBLIC_ADMIN_BASE_PATHSame for SPACE. Please confirm how these apps source basePath today.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/docker-smoke.yml(1 hunks)apps/live/Dockerfile.live(1 hunks)scripts/bench-builds.sh(1 hunks)scripts/smoke.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/live/Dockerfile.live
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/bench-builds.sh
[warning] 120-120: unit appears unused. Verify use (or export if used externally).
(SC2034)
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
from
f1e290e to
9efe0ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
scripts/bench-builds.sh (5)
28-41: Good: Early Bash 4+ guard prevents associative-array breakage on macOS (Bash 3.2).This directly addresses portability concerns and avoids hard-to-debug runtime failures when
declare -Ais used.
78-124: Good: Robust nanosecond time source with validation and multi-tier fallbacks.The numeric-output validation fixes the BSD
date“%N” pitfall; fallbacks viaEPOCHREALTIME,perl Time::HiRes, then seconds×1e9 are solid.
142-159: Simplify human_bytes() and avoid awk log()/zero-edge pitfalls.Current implementation mixes integer division with a log-based awk expression and can misbehave for 0 bytes or awk variants without reliable
log(). Prefer a single-pass awk approach.Apply this diff:
-humann_bytes() { - # Convert integer bytes to human readable - local bytes="$1" - local units=("B" "KB" "MB" "GB" "TB" "PB") - local idx=0 - local value="$bytes" - while (( value >= 1024 && idx < ${#units[@]}-1 )); do - value=$(( value/1024 )) - idx=$(( idx+1 )) - done - # value is integer; but to preserve more precision, recompute with awk - awk -v b="$bytes" -v i="$idx" 'BEGIN { - split("B KB MB GB TB PB", u, " "); - val=b; - while (val>=1024 && i>0){ val/=1024; i-- } - printf("%.2f %s", val, u[(int((log(b)/log(1024)))+1)]); - }' 2>/dev/null || printf "%d %s" "$value" "${units[$idx]}" -} +human_bytes() { + # Convert integer bytes to human-readable (two decimals) + local bytes="${1:-0}" + awk -v b="$bytes" 'BEGIN { + if (b < 0) b = 0 + split("B KB MB GB TB PB", u, " ") + i = 1 + while (b >= 1024 && i < 6) { b /= 1024; i++ } + printf("%.2f %s", b, u[i]) + }' 2>/dev/null || printf "%s %s" "${bytes}" "B" +}
272-295: Resolve conflicting flags: --no-prune vs --aggressive-prune.If both are passed, current logic silently skips pruning, which contradicts “aggressive.” Make precedence explicit.
Add right after the parsing loop:
done +# Resolve conflicting options +if [[ "$AGGRESSIVE_PRUNE" == "true" && "$NO_PRUNE" == "true" ]]; then + log "Both --no-prune and --aggressive-prune specified; ignoring --no-prune." + NO_PRUNE="false" +fi
299-303: Good: Check Docker daemon reachability early.The
docker infoprobe saves time by failing fast when the daemon is down.scripts/smoke.sh (2)
206-215: Good: Unknown services are rejected in both build and run paths.Fast-failing on typos is the right UX, including when
--no-buildis used.
189-194: Fix precedence: CLI --services should override SERVICES env.The current split uses
${SERVICES:-$services}, giving the environment higher precedence than the parsed option. In CI and local runs this can cause surprising behavior.-IFS=',' read -r -a SELECTED <<< "${SERVICES:-$services}" +IFS=',' read -r -a SELECTED <<< "${services:-$SERVICES}"
🧹 Nitpick comments (3)
scripts/bench-builds.sh (1)
174-176: Remove unused local ‘unit’ variable (ShellCheck SC2034).Declared but never used; keep the declaration minimal.
- local line size unit + local line sizescripts/smoke.sh (2)
63-66: Tighten HTTP status match to exactly 3 digits (2xx/3xx).
2*|3*works but is overly permissive. Use2??|3??to match 200–399 precisely.- case "$code" in - 2*|3*) return 0 ;; - *) return 1 ;; - esac + case "$code" in + 2??|3??) return 0 ;; + *) return 1 ;; + esac
189-193: Add Docker daemon reachability check (not just CLI presence).Mirror the bench script: the CLI can exist while the daemon is down; fail fast with a clearer message.
have docker || die "Docker is required. Please install Docker and ensure the daemon is running." +# Verify Docker daemon is reachable +if ! docker info >/dev/null 2>&1; then + die "Docker daemon is not running or is not reachable. Please start Docker (e.g., Docker Desktop) and try again." +fi + cd "$ROOT_DIR"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/docker-smoke.yml(1 hunks)apps/live/Dockerfile.live(1 hunks)scripts/bench-builds.sh(1 hunks)scripts/smoke.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/live/Dockerfile.live
- .github/workflows/docker-smoke.yml
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/smoke.sh (1)
scripts/bench-builds.sh (3)
have(72-72)die(76-76)log(74-74)
🪛 Shellcheck (0.10.0)
scripts/bench-builds.sh
[warning] 174-174: unit appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and smoke test admin
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
scripts/smoke.sh (1)
196-204: LIVE_BASE_PATH is correctly consumed at runtime in the Live app—no build-time ARG neededI checked how
LIVE_BASE_PATHis used in apps/live:
- In apps/live/src/server.ts the router is mounted against
process.env.LIVE_BASE_PATH(or"/live"), so it’s read at runtime, not during build.- There is no
next.config.*under apps/live, confirming it’s not a Next.js build that bakes in abasePath.- The smoke script’s
run_container … -e LIVE_BASE_PATH="${LIVE_BASE_PATH}"correctly injects the env var for runtime use.Since Live isn’t using a Next.js
basePathat build-time, passingLIVE_BASE_PATHonly as a runtime-eis appropriate. No changes required to Dockerfile.live or the smoke test.
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
from
9efe0ec to
85ac151
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
scripts/bench-builds.sh (2)
142-159: Simplify human_bytes() and avoid awk log()/0 edge casesCurrent version mixes integer loop + awk with
log(b), which can misbehave for 0 and varies across awk variants. A single, simple awk is more portable.Apply this diff:
-human_bytes() { - # Convert integer bytes to human readable - local bytes="$1" - local units=("B" "KB" "MB" "GB" "TB" "PB") - local idx=0 - local value="$bytes" - while (( value >= 1024 && idx < ${#units[@]}-1 )); do - value=$(( value/1024 )) - idx=$(( idx+1 )) - done - # value is integer; but to preserve more precision, recompute with awk - awk -v b="$bytes" -v i="$idx" 'BEGIN { - split("B KB MB GB TB PB", u, " "); - val=b; - while (val>=1024 && i>0){ val/=1024; i-- } - printf("%.2f %s", val, u[(int((log(b)/log(1024)))+1)]); - }' 2>/dev/null || printf "%d %s" "$value" "${units[$idx]}" -} +human_bytes() { + # Convert integer bytes to human-readable (two decimals) + local bytes="${1:-0}" + awk -v b="$bytes" 'BEGIN { + if (b < 0) b = 0 + split("B KB MB GB TB PB", u, " ") + i = 1 + while (b >= 1024 && i < 6) { b /= 1024; i++ } + printf("%.2f %s", b, u[i]) + }' 2>/dev/null || printf "%s %s" "${bytes}" "B" +}
296-305: Make --no-prune vs --aggressive-prune precedence explicitIf both flags are set, current behavior is implicit. Prefer failing fast or documenting precedence.
Apply this diff right after the argument parsing loop:
done +# Resolve conflicting options +if [[ "$AGGRESSIVE_PRUNE" == "true" && "$NO_PRUNE" == "true" ]]; then + log "Both --no-prune and --aggressive-prune specified; ignoring --no-prune." + NO_PRUNE="false" +fiscripts/smoke.sh (1)
162-166: Let CLI --services override environment SERVICESCurrently,
${SERVICES:-$services}causes the environment to override the parsed CLI option, which is surprising. Make CLI take precedence.Apply this diff:
-cmd="${1:-up}" -no_build="false" -services="${SERVICES:-web,admin,space,live}" -shift $(( $# > 0 ? 1 : 0 )) +cmd="${1:-up}" +no_build="false" +# Default list; CLI --services should take precedence over environment. +services="web,admin,space,live" +if [[ -n "${SERVICES:-}" ]]; then services="$SERVICES"; fi +shift $(( $# > 0 ? 1 : 0 ))-IFS=',' read -r -a SELECTED <<< "${SERVICES:-$services}" +IFS=',' read -r -a SELECTED <<< "$services"Also applies to: 194-204
🧹 Nitpick comments (4)
scripts/bench-builds.sh (1)
172-181: Minor: remove unused local “unit” (ShellCheck SC2034)
unitis declared but never used.Apply this diff:
- local line size unit + local line sizescripts/smoke.sh (1)
189-191: Also verify Docker daemon, not just CLIIf the daemon isn’t running, later steps fail noisily. Add a lightweight check.
Apply this diff below the existing check:
have docker || die "Docker is required. Please install Docker and ensure the daemon is running." +docker info >/dev/null 2>&1 || die "Docker daemon is not running or not reachable. Start Docker and retry.".github/workflows/docker-smoke.yml (2)
118-139: Broaden success criteria to any 2xx (and optionally 3xx) HTTP statusSome endpoints may legitimately return 204 or redirect before settling. You already use -L to follow redirects, but limiting to 200 is brittle.
Apply this diff:
- for i in {1..60}; do - STATUS="$(curl -sS -o /dev/null -w "%{http_code}" -L "${URL}" || true)" - if [ "${STATUS}" = "200" ]; then - echo "Success: HTTP ${STATUS} from ${URL}" - exit 0 - fi + for i in {1..60}; do + STATUS="$(curl -sS -o /dev/null -w "%{http_code}" -L "${URL}" || true)" + case "${STATUS}" in + 2??|3??) + echo "Success: HTTP ${STATUS} from ${URL}" + exit 0 + ;; + esac echo "Attempt ${i}: HTTP ${STATUS} (waiting 2s)" sleep 2 done - echo "Failed to get HTTP 200 from ${URL}" + echo "Failed to get a 2xx/3xx response from ${URL}"
47-61: Nit: normalize path globs (drop leading ./) to match triggersThe filters use
./apps/...while triggers useapps/.... Both often work, but unifying reduces cognitive overhead.Apply this diff:
- web: - - './apps/web/**' + web: + - 'apps/web/**' space: - - './apps/space/**' + - 'apps/space/**' admin: - - './apps/admin/**' + - 'apps/admin/**' live: - - './apps/live/**' + - 'apps/live/**' common: - 'turbo.json' - 'pnpm-lock.yaml' - 'pnpm-workspace.yaml' - '.github/workflows/docker-smoke.yml' - - './packages/**' + - 'packages/**'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/docker-smoke.yml(1 hunks)scripts/bench-builds.sh(1 hunks)scripts/smoke.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/bench-builds.sh
[warning] 174-174: unit appears unused. Verify use (or export if used externally).
(SC2034)
�� Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and smoke test web
- GitHub Check: Build and smoke test space
- GitHub Check: Build and smoke test admin
- GitHub Check: Build and smoke test live
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
scripts/bench-builds.sh (1)
78-124: Robust time source detection looks solidThe
now_ns()implementation is portable and validates outputs to avoid BSD%Npitfalls. Good use of 10# to avoid octal issues.scripts/smoke.sh (1)
56-67: HTTP probe logic is correct and resilientAccepting both 2xx and 3xx via a case pattern is clear and avoids regex precedence pitfalls. Nice.
.github/workflows/docker-smoke.yml (1)
76-85: Align live app port with local smoke script for parity (3005 vs 3004)Matrix uses host port 3005 for live, while scripts/smoke.sh defaults LIVE_PORT=3004. Consider aligning to reduce confusion.
Would you like me to change the matrix port to 3004 for live?
- if (buildAll || changed.live) add('live', 'apps/live/Dockerfile.live', 'plane-live:ci-smoke', 'plane-live-ci', 3005, '/live/health', '-e NODE_ENV=production -e LIVE_BASE_PATH=/live'); + if (buildAll || changed.live) add('live', 'apps/live/Dockerfile.live', 'plane-live:ci-smoke', 'plane-live-ci', 3004, '/live/health', '-e NODE_ENV=production -e LIVE_BASE_PATH=/live');
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
from
85ac151 to
c596ad3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/docker-smoke.yml (1)
46-61: Paths filter looks good and is consistent with prior feedbackFilters are clear and consistently quoted; including packages/** under common is appropriate for rebuilding all. Nothing to change.
scripts/bench-builds.sh (2)
142-159: Simplify human_bytes() and avoid log(0)/awk portability pitfallsCurrent logic mixes integer loop with an awk recomputation using log(); this can misbehave for 0 bytes and differs across awk variants. A single awk block (or pure shell) is clearer and safer.
Apply this diff:
human_bytes() { - # Convert integer bytes to human readable - local bytes="$1" - local units=("B" "KB" "MB" "GB" "TB" "PB") - local idx=0 - local value="$bytes" - while (( value >= 1024 && idx < ${#units[@]}-1 )); do - value=$(( value/1024 )) - idx=$(( idx+1 )) - done - # value is integer; but to preserve more precision, recompute with awk - awk -v b="$bytes" -v i="$idx" 'BEGIN { - split("B KB MB GB TB PB", u, " "); - val=b; - while (val>=1024 && i>0){ val/=1024; i-- } - printf("%.2f %s", val, u[(int((log(b)/log(1024)))+1)]); - }' 2>/dev/null || printf "%d %s" "$value" "${units[$idx]}" + # Convert integer bytes to human-readable (two decimals) + local bytes="${1:-0}" + awk -v b="$bytes" 'BEGIN { + if (b < 0) b = 0 + split("B KB MB GB TB PB", u, " ") + i = 1 + while (b >= 1024 && i < 6) { b /= 1024; i++ } + printf("%.2f %s", b, u[i]) + }' 2>/dev/null || printf "%s %s" "${bytes}" "B" }
272-295: Resolve conflicting flags: --no-prune vs --aggressive-pruneIf both are provided, current behavior is ambiguous. Make precedence explicit or error out.
Apply this diff right after the parsing loop:
done +# Resolve conflicting options +if [[ "$AGGRESSIVE_PRUNE" == "true" && "$NO_PRUNE" == "true" ]]; then + log "Both --no-prune and --aggressive-prune specified; ignoring --no-prune." + NO_PRUNE="false" +fiIf you prefer to hard-fail instead of choosing precedence, replace the body with: die "Choose either --no-prune or --aggressive-prune, not both."
🧹 Nitpick comments (5)
.github/workflows/docker-smoke.yml (3)
1-29: Add minimal permissions and a concurrency group for safer, more efficient CITighten token scope and avoid redundant runs on the same ref.
Apply this diff:
name: Docker build and smoke test for apps on: workflow_dispatch: pull_request: branches: ["preview"] @@ paths: - ".github/workflows/docker-smoke.yml" +permissions: + contents: read + +concurrency: + group: docker-smoke-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: trueNote: Consider pinning all third-party actions (checkout, paths-filter, github-script) to immutable commit SHAs for supply-chain hardening.
118-139: Accept any 2xx status to reduce false negatives; keep logs on failureSome endpoints may return 204/201 or similar. Since you already follow redirects (-L), treating any 2xx as success makes the probe more robust.
Apply this diff:
- # Try up to ~120 seconds (60 attempts * 2s) + # Try up to ~120 seconds (60 attempts * 2s) for i in {1..60}; do STATUS="$(curl -sS -o /dev/null -w "%{http_code}" -L "${URL}" || true)" - if [ "${STATUS}" = "200" ]; then - echo "Success: HTTP ${STATUS} from ${URL}" + if [[ "${STATUS}" =~ ^2[0-9][0-9]$ ]]; then + echo "Success: HTTP ${STATUS} from ${URL}" exit 0 fi echo "Attempt ${i}: HTTP ${STATUS} (waiting 2s)" sleep 2 doneIf some services intentionally return a 3xx terminal status, expand the regex to include ^3..$.
76-83: Safer env passing than free-form flags stringPassing env via a free-form string can break on spaces/quotes. Consider a structured matrix field (list of KEY=VAL) and expanding to -e args in the step.
Example change (conceptual, adjust as needed):
- In the matrix, use env: ['NODE_ENV=production','LIVE_BASE_PATH=/live'] instead of env_flags.
- In the run step:
env_args=() for kv in ${{ join(matrix.env, ' ') }}; do env_args+=(-e "$kv") done docker run -d --name "${{ matrix.container }}" -p "${{ matrix.host_port }}:3000" "${env_args[@]}" "${{ matrix.image }}"Also applies to: 113-117
scripts/bench-builds.sh (2)
166-181: Minor cleanup: remove unused local variable flagged by ShellCheckThe unit variable is declared but never used.
Apply this diff:
- if have docker && docker buildx version >/dev/null 2>&1; then + if have docker && docker buildx version >/dev/null 2>&1; then # Try buildx du summary (not standardized across versions; best effort parsing) - local du + local du du="$(docker buildx du 2>/dev/null || true)" if [[ -n "$du" ]]; then # Look for a line with total size summary; fallback to whole output - local line size unit + local line size line="$(echo "$du" | tail -n 1)"
235-245: Optional: expose a verbose mode to show docker build logs during benchesHiding build output keeps logs clean but can hinder debugging. Consider a VERBOSE switch to surface docker build logs when needed.
Example tweak:
- Gate >/dev/null redirection on VERBOSE:
- DOCKER_BUILDKIT=1 docker build --no-cache --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR" >/dev/null + if [[ "${VERBOSE:-}" == "1" ]]; then + DOCKER_BUILDKIT=1 docker build --no-cache --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR" + else + DOCKER_BUILDKIT=1 docker build --no-cache --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR" >/dev/null + fiRepeat similarly for the warm build path.
Also applies to: 336-347
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/docker-smoke.yml(1 hunks)scripts/bench-builds.sh(1 hunks)scripts/smoke.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/smoke.sh
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/bench-builds.sh (1)
scripts/smoke.sh (3)
have(50-50)log(52-52)die(54-54)
🪛 Shellcheck (0.10.0)
scripts/bench-builds.sh
[warning] 174-174: unit appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and smoke test space
- GitHub Check: Build and smoke test web
- GitHub Check: Build and smoke test live
- GitHub Check: Build and smoke test admin
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
scripts/bench-builds.sh (3)
28-41: Solid Bash version gate for associative arraysClear, user-friendly guidance for macOS users. Approved.
78-124: Robust high-resolution time source selectionGood validation to avoid BSD date “%N” trap, and sane fallbacks. No change needed.
299-304: Good preflight checks for docker CLI and daemon availabilityEarly failure with actionable message. Approved.
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
from
c596ad3 to
066f6ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
.github/workflows/docker-smoke.yml (2)
62-86: Replace fragile empty-matrix string check with a boolean output.Comparing JSON to '{"include":[]}' is brittle. Emit and consume a has_targets boolean for clarity and safety.
- name: Build matrix id: build-matrix uses: actions/github-script@v7 with: script: | const include = []; const eventName = context.eventName; const anyCommon = '${{ steps.changes.outputs.common }}' === 'true'; const changed = { web: '${{ steps.changes.outputs.web }}' === 'true', space: '${{ steps.changes.outputs.space }}' === 'true', admin: '${{ steps.changes.outputs.admin }}' === 'true', live: '${{ steps.changes.outputs.live }}' === 'true', }; const add = (name, dockerfile, image, container, port, path, env_flags = "") => include.push({ name, dockerfile, image, container, host_port: port, path, env_flags }); const buildAll = anyCommon || eventName === 'push' || eventName === 'workflow_dispatch'; if (buildAll || changed.web) add('web', 'apps/web/Dockerfile.web', 'plane-web:ci-smoke', 'plane-web-ci', 3001, '/'); if (buildAll || changed.space) add('space', 'apps/space/Dockerfile.space', 'plane-space:ci-smoke', 'plane-space-ci', 3002, '/spaces'); if (buildAll || changed.admin) add('admin', 'apps/admin/Dockerfile.admin', 'plane-admin:ci-smoke', 'plane-admin-ci', 3003, '/god-mode'); if (buildAll || changed.live) add('live', 'apps/live/Dockerfile.live', 'plane-live:ci-smoke', 'plane-live-ci', 3005, '/live/health', '-e NODE_ENV=production -e LIVE_BASE_PATH=/live'); - if (include.length === 0) { - core.warning('No changes detected for any app. Matrix is empty. Skipping smoke tests.'); - } - core.setOutput('matrix', JSON.stringify({ include })); + const hasTargets = include.length > 0; + if (!hasTargets) core.warning('No changes detected for any app. Matrix is empty. Skipping smoke tests.'); + core.setOutput('matrix', JSON.stringify({ include })); + core.setOutput('has_targets', String(hasTargets)); @@ smoke: name: Build and smoke test ${{ matrix.name }} runs-on: ubuntu-latest needs: determine-matrix - if: ${{ needs.determine-matrix.outputs.matrix != '{"include":[]}' }} + if: ${{ needs.determine-matrix.outputs.has_targets == 'true' }}Also applies to: 87-96
7-16: Standardize quoting style for path globs and filters.Use a consistent quoting style across triggers and filters for readability.
Example:
- Prefer single quotes everywhere, or double quotes everywhere; avoid mixing.
Also applies to: 19-28, 47-61
scripts/bench-builds.sh (2)
142-159: Simplify human_bytes() and avoid log(0)/awk portability pitfalls.Current awk path uses log() and can misbehave for 0 bytes. A simpler approach is safer.
human_bytes() { - # Convert integer bytes to human readable - local bytes="$1" - local units=("B" "KB" "MB" "GB" "TB" "PB") - local idx=0 - local value="$bytes" - while (( value >= 1024 && idx < ${#units[@]}-1 )); do - value=$(( value/1024 )) - idx=$(( idx+1 )) - done - # value is integer; but to preserve more precision, recompute with awk - awk -v b="$bytes" -v i="$idx" 'BEGIN { - split("B KB MB GB TB PB", u, " "); - val=b; - while (val>=1024 && i>0){ val/=1024; i-- } - printf("%.2f %s", val, u[(int((log(b)/log(1024)))+1)]); - }' 2>/dev/null || printf "%d %s" "$value" "${units[$idx]}" + local bytes="${1:-0}" + awk -v b="$bytes" 'BEGIN { + if (b < 0) b = 0 + split("B KB MB GB TB PB", u, " ") + i = 1 + while (b >= 1024 && i < 6) { b /= 1024; i++ } + printf("%.2f %s", b, u[i]) + }' 2>/dev/null || printf "%s %s" "${bytes}" "B" }
272-295: Resolve conflicting flags: --no-prune vs --aggressive-prune.Make precedence explicit to avoid surprising behavior when both are set.
done +# Resolve conflicting options +if [[ "$AGGRESSIVE_PRUNE" == "true" && "$NO_PRUNE" == "true" ]]; then + log "Both --no-prune and --aggressive-prune specified; ignoring --no-prune." + NO_PRUNE="false" +fi
🧹 Nitpick comments (5)
.github/workflows/docker-smoke.yml (3)
108-116: Consider pulling latest base layers during CI builds.Adding --pull=always reduces cache-stale surprises on runners.
- docker build -f ${{ matrix.dockerfile }} -t ${{ matrix.image }} . + docker build --pull=always -f ${{ matrix.dockerfile }} -t ${{ matrix.image }} .
118-134: Harden curl probe with timeouts to avoid long hangs.Add connect and total timeouts; current loop interval alone won’t cap curl blocking time.
- STATUS="$(curl -sS -o /dev/null -w "%{http_code}" -L "${URL}" || true)" + STATUS="$(curl --connect-timeout 2 --max-time 5 -sS -o /dev/null -w "%{http_code}" -L "${URL}" || true)"
1-2: Optional: add workflow-level concurrency to auto-cancel superseded runs.Saves minutes on stacked pushes to the same ref.
name: Docker build and smoke test for apps on: +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: truescripts/bench-builds.sh (2)
171-178: Remove unused local variable to satisfy ShellCheck SC2034.unit is declared but never used.
- local line size unit + local line size
235-244: Optional: make build logs toggleable for debugging.Silencing docker build can hinder diagnosis on failures. Consider a VERBOSE flag to tee logs.
- if [[ "$cold" == "true" ]]; then - DOCKER_BUILDKIT=1 docker build --no-cache --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR" >/dev/null - else - DOCKER_BUILDKIT=1 docker build --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR" >/dev/null - fi + local quiet_redir=">/dev/null" + [[ "${VERBOSE:-0}" == "1" ]] && quiet_redir="" + if [[ "$cold" == "true" ]]; then + eval DOCKER_BUILDKIT=1 docker build --no-cache --progress=plain -f "\"$dockerfile\"" -t "\"$tag\"" "\"$ROOT_DIR\"" $quiet_redir + else + eval DOCKER_BUILDKIT=1 docker build --progress=plain -f "\"$dockerfile\"" -t "\"$tag\"" "\"$ROOT_DIR\"" $quiet_redir + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/docker-smoke.yml(1 hunks)scripts/bench-builds.sh(1 hunks)scripts/smoke.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/smoke.sh
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/bench-builds.sh
[warning] 174-174: unit appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and smoke test admin
- GitHub Check: Build and smoke test web
- GitHub Check: Build and smoke test space
- GitHub Check: Build and smoke test live
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
scripts/bench-builds.sh (2)
28-41: Good Bash version guard.Clear error and actionable guidance for macOS users.
296-304: Good Docker daemon reachability check.Early fail with clear guidance improves UX.
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
3 times, most recently
from
9b0ec86 to
f4cf0e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/smoke.sh (1)
62-69: Add network timeouts to avoid hangs in http probes.Without timeouts, curl/wget can block indefinitely on bad endpoints, stalling the loop.
- local code - code="$(curl -fsS -o /dev/null -w "%{http_code}" "$url" || true)" + local code + code="$(curl --connect-timeout 3 --max-time 10 -fsS -o /dev/null -w "%{http_code}" "$url" || true)" @@ - elif have wget; then - wget -q --spider "$url" >/dev/null 2>&1 + elif have wget; then + wget -q --timeout=10 --tries=1 --spider "$url" >/dev/null 2>&1
🧹 Nitpick comments (7)
scripts/smoke.sh (7)
63-66: Tighten 2xx/3xx match to exactly three digits.Use 2??|3?? for clarity and precision.
- case "$code" in - 2*|3*) return 0 ;; - *) return 1 ;; - esac + case "$code" in + 2??|3??) return 0 ;; + *) return 1 ;; + esac
189-191: Fail fast if Docker daemon isn’t reachable.Checking only for the CLI misses the “daemon not running” case.
-have docker || die "Docker is required. Please install Docker and ensure the daemon is running." +have docker || die "Docker is required. Please install Docker and ensure the daemon is running." +# Verify daemon availability early +docker ps -q >/dev/null 2>&1 || die "Docker daemon not reachable. Start Docker and retry."
194-194: Trim whitespace when parsing --services.Avoid accidental entries like " web" or "space ".
-IFS=',' read -r -a SELECTED <<< "$services" +IFS=',' read -r -a SELECTED <<< "${services//[[:space:]]/}"
209-213: Confirm live’s base path is runtime-driven; if not, mirror build-arg approach.If live resolves basePath at build time (like Next.js), passing LIVE_BASE_PATH only at
docker runwon’t take effect.Option (if build-time): bake it during build and drop the runtime env.
- live) run_container "$LIVE_CONT" "$LIVE_IMAGE" "$LIVE_PORT" -e LIVE_BASE_PATH="${LIVE_BASE_PATH}" ;; + live) run_container "$LIVE_CONT" "$LIVE_IMAGE" "$LIVE_PORT" ;;And in the build case above:
- live) build_image "live" "$LIVE_IMAGE" "apps/live/Dockerfile.live" ;; + live) build_image "live" "$LIVE_IMAGE" "apps/live/Dockerfile.live" --build-arg LIVE_BASE_PATH="${LIVE_BASE_PATH}" ;;Want me to scan the repo to verify how live reads this setting?
102-107: Consider pulling base images fresh for more deterministic smoke builds.Prevents stale cache from hiding broken base layers; toggleable if desired.
- DOCKER_BUILDKIT=1 docker build \ + DOCKER_BUILDKIT=1 docker build --pull=always \
233-241: Optional: show logs on failures to speed up triage.Auto-print the last lines from failed services.
if (( failures == 0 )); then log "Smoke test succeeded. Containers are running:" else log "Smoke test completed with ${failures} failure(s). Containers may still be running for inspection:" + # Dump brief logs to help triage + for svc in "${SELECTED[@]}"; do + case "$svc" in + web) name="$WEB_CONT" ;; + admin) name="$ADMIN_CONT" ;; + space) name="$SPACE_CONT" ;; + live) name="$LIVE_CONT" ;; + esac + docker ps -a --format '{{.Names}}' | grep -q "^${name}\$" && { + log "---- logs: ${name} (last 80 lines) ----" + docker logs --tail=80 "$name" || true + } + done fi
243-249: Nit: print URLs only for selected services.Avoids confusion when a subset is chosen.
-URLs: - web: ${web_url} - admin: ${admin_url} - space: ${space_url} - live: ${live_url} +URLs: +$(printf ' %s\n' "${SELECTED[@]}" | awk ' + /web/ {print " web: '"${web_url}"'"} + /admin/ {print " admin: '"${admin_url}"'"} + /space/ {print " space: '"${space_url}"'"} + /live/ {print " live: '"${live_url}"'"}')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/docker-smoke.yml(1 hunks)scripts/bench-builds.sh(1 hunks)scripts/smoke.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/docker-smoke.yml
- scripts/bench-builds.sh
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/smoke.sh (1)
scripts/bench-builds.sh (3)
have(73-73)die(77-77)log(75-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
scripts/smoke.sh (1)
197-201: Good: admin/space basePath baked at build time.Using build-args for NEXT_PUBLIC_ADMIN_BASE_PATH and NEXT_PUBLIC_SPACE_BASE_PATH aligns with Next.js build-time config.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
scripts/smoke.sh (1)
62-69: Harden HTTP check: add timeouts and precise status match (2xx/3xx)Prevent hangs and avoid overly broad globbing by matching 3-digit codes and adding curl/wget timeouts.
- local code - code="$(curl -fsS -o /dev/null -w "%{http_code}" "$url" || true)" - case "$code" in - 2*|3*) return 0 ;; - *) return 1 ;; - esac + local code + code="$(curl --connect-timeout 3 --max-time 10 -fsS -o /dev/null -w "%{http_code}" "$url" || true)" + case "$code" in + 2??|3??) return 0 ;; + *) return 1 ;; + esac elif have wget; then - wget -q --spider "$url" >/dev/null 2>&1 + wget -q --timeout=10 --spider "$url" >/dev/null 2>&1 return $?.github/workflows/docker-smoke.yml (2)
66-95: Expose has_targets boolean and use it instead of brittle string-compare on the matrixAvoid comparing JSON strings; surface a boolean and gate the job on it.
outputs: - matrix: ${{ steps.build-matrix.outputs.matrix }} + matrix: ${{ steps.build-matrix.outputs.matrix }} + has_targets: ${{ steps.build-matrix.outputs.has_targets }} @@ - name: Build matrix id: build-matrix uses: actions/github-script@v7 with: script: | const include = []; @@ if (include.length === 0) { core.warning('No changes detected for any app. Matrix is empty. Skipping smoke tests.'); } core.setOutput('matrix', JSON.stringify({ include })); + core.setOutput('has_targets', String(include.length > 0)); @@ smoke: @@ - if: ${{ needs.determine-matrix.outputs.matrix != '{"include":[]}' }} + if: ${{ needs.determine-matrix.outputs.has_targets == 'true' }}
117-121: Validate env_flags before injecting into docker run to avoid command injectionConstrain env_flags to safe “-e VAR=VALUE” patterns and use a sanitized variable.
+ - name: Validate env_flags (${{ matrix.name }}) + id: validate_env_flags + shell: bash + run: | + set -euo pipefail + ENV_FLAGS='${{ matrix.env_flags }}' + if [[ -z "$ENV_FLAGS" ]]; then + echo "SAFE_ENV_FLAGS=" >> $GITHUB_ENV + elif [[ "$ENV_FLAGS" =~ ^(-e[[:space:]][A-Za-z_][A-Za-z0-9_]*=[^[:space:]]+[[:space:]]?)*$ ]]; then + echo "SAFE_ENV_FLAGS=$ENV_FLAGS" >> $GITHUB_ENV + else + echo "Unsafe env_flags detected: $ENV_FLAGS" >&2 + exit 1 + fi + - name: Run container (${{ matrix.name }}) run: | - docker run -d --name ${{ matrix.container }} -p ${{ matrix.host_port }}:3000 ${{ matrix.env_flags }} ${{ matrix.image }} + docker run -d --name ${{ matrix.container }} -p ${{ matrix.host_port }}:3000 $SAFE_ENV_FLAGS ${{ matrix.image }} docker ps -a
🧹 Nitpick comments (1)
.github/workflows/docker-smoke.yml (1)
128-137: Clarify timeout commentNote request time adds overhead to the nominal 120s loop timing.
- # Try up to ~120 seconds (60 attempts * 2s) + # Try up to at least ~120 seconds (60 attempts * 2s), plus per-request time
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/docker-smoke.yml(1 hunks)scripts/bench-builds.sh(1 hunks)scripts/smoke.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/smoke.sh (1)
scripts/bench-builds.sh (3)
have(73-73)die(77-77)log(75-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and smoke test web
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
from
f4cf0e3 to
c620c12
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
scripts/smoke.sh (3)
60-66: HTTP status matcher fixed — LGTMThe 2xx/3xx case pattern is correct and robust.
165-173: CLI precedence over env for --services — LGTMParsing honors the CLI flag and later splits only the resolved
servicesvalue.Also applies to: 199-199
201-209: Unknown service validation in both build and run loops — LGTMFast-fail behavior is consistent now.
Also applies to: 211-219
🧹 Nitpick comments (2)
scripts/smoke.sh (2)
24-27: Validate integer timing vars or switch to time-based deadline
SLEEP_INTERVALandMAX_WAITare used in integer arithmetic; non-integer env values (e.g., “0.5”) will break the loop.Minimal fix (validate integers and die early):
MAX_WAIT="${MAX_WAIT:-120}" SLEEP_INTERVAL="${SLEEP_INTERVAL:-2}" +[[ "$MAX_WAIT" =~ ^[0-9]+$ ]] || die "MAX_WAIT must be an integer" +[[ "$SLEEP_INTERVAL" =~ ^[0-9]+$ ]] || die "SLEEP_INTERVAL must be an integer"Alternatively, switch
wait_for_urlto a timestamp deadline (supports fractional sleeps) if you want to allow non-integers.Also applies to: 75-87
192-196: Daemon reachability check added — goodYou now fail fast if Docker isn’t reachable. Consider
docker info >/dev/null(slightly clearer intent) instead ofdocker ps -q.Optional tweak:
-docker ps -q >/dev/null 2>&1 || die "Docker daemon not reachable. Start Docker and retry." +docker info >/dev/null 2>&1 || die "Docker daemon not reachable. Start Docker and retry."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/docker-smoke.yml(1 hunks)scripts/bench-builds.sh(1 hunks)scripts/smoke.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/docker-smoke.yml
- scripts/bench-builds.sh
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/smoke.sh
[warning] 262-266: Remove surrounding $() to avoid executing output (or use eval if intentional).
(SC2091)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and smoke test admin
- GitHub Check: Build and smoke test space
- GitHub Check: Build and smoke test live
- GitHub Check: Build and smoke test web
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
scripts/smoke.sh (1)
203-206: Request: verify Dockerfiles consume the passed build argsYou’re passing
--build-argfor admin/space/live; ensure the corresponding Dockerfiles declareARGand wire to the build (and Next.js sees them at build time).Would you like me to scan the repo for:
- apps/admin|space|live Dockerfiles containing
ARG NEXT_PUBLIC_*_BASE_PATH/ARG LIVE_BASE_PATH- next.config.js reading these envs for basePath?
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
from
6540940 to
f1d6fa2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
from
f1d6fa2 to
01dd81c
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive Docker smoke testing infrastructure to automate building and testing app containers. The changes include CI workflow automation, local testing capabilities, and Docker configuration improvements for security and health monitoring.
- Adds a CI workflow that builds app images conditionally based on changed files and runs readiness tests
- Implements local smoke testing scripts for developer verification with build timing benchmarks
- Enhances Dockerfiles with proper user security, health checks, and production environment setup
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/smoke.sh | Local smoke test CLI for building, running, and probing multiple apps |
| scripts/bench-builds.sh | Benchmarking script comparing cold vs warm Docker builds with timing/size metrics |
| apps/web/Dockerfile.web | Security improvements with proper user setup and health check addition |
| apps/space/Dockerfile.space | User security and health check enhancements matching web app pattern |
| apps/live/Dockerfile.live | Complete security setup with user creation and health monitoring |
| apps/admin/Dockerfile.admin | Consistent security and health check implementation |
| .github/workflows/docker-smoke.yml | CI workflow for automated Docker builds and smoke testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
apps/space/Dockerfile.space (1)
104-105: Healthcheck: avoid downloading bodies.Consider using a HEAD/spider probe to save bandwidth and speed up checks. If BusyBox wget supports it in your base, switch to
--spider; otherwise, current approach is acceptable.Example:
-HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=5 \ - CMD wget -qO- http://localhost:3000/spaces > /dev/null || exit 1 +HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=5 \ + CMD wget -q --spider http://localhost:3000/spaces || exit 1apps/web/Dockerfile.web (1)
121-122: Healthcheck micro-optimization.Prefer a metadata-only probe to avoid fetching the HTML body; retain current form if BusyBox wget lacks spider mode.
-HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=5 \ - CMD wget -qO- http://localhost:3000/ > /dev/null || exit 1 +HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=5 \ + CMD wget -q --spider http://localhost:3000/ || exit 1apps/live/Dockerfile.live (2)
71-72: Healthcheck path looks correct; consider spider mode.
/live/healthis a good target. Optionally avoid downloading response bodies with spider/HEAD if available.-HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=5 \ - CMD wget -qO- http://localhost:3000/live/health > /dev/null || exit 1 +HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=5 \ + CMD wget -q --spider http://localhost:3000/live/health || exit 1
28-29: Avoid unnecessaryapk update.
apk add --no-cache ...already pulls the latest index;apk updateadds an extra layer and is typically unnecessary.-RUN apk update RUN apk add --no-cache libc6-compat(Repeat for the installer stage.)
apps/admin/Dockerfile.admin (1)
104-105: Healthcheck: prefer metadata-only probe.Optionally switch to spider to avoid body downloads; keep current if not supported in your wget.
-HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=5 \ - CMD wget -qO- http://localhost:3000/god-mode > /dev/null || exit 1 +HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=5 \ + CMD wget -q --spider http://localhost:3000/god-mode || exit 1scripts/bench-builds.sh (2)
228-245: Prefer array-built docker commands; avoid duplication.Current branching duplicates the build invocation. Building the command in an array reduces repetition and quoting pitfalls.
- if [[ "${VERBOSE:-0}" == "1" ]]; then - if [[ "$cold" == "true" ]]; then - DOCKER_BUILDKIT=1 docker build --no-cache --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR" - else - DOCKER_BUILDKIT=1 docker build --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR" - fi - else - if [[ "$cold" == "true" ]]; then - DOCKER_BUILDKIT=1 docker build --no-cache --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR" >/dev/null - else - DOCKER_BUILDKIT=1 docker build --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR" >/dev/null - fi - fi + local -a cmd=(docker build --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR") + [[ "$cold" == "true" ]] && cmd=(docker build --no-cache --progress=plain -f "$dockerfile" -t "$tag" "$ROOT_DIR") + if [[ "${VERBOSE:-0}" == "1" ]]; then + DOCKER_BUILDKIT=1 "${cmd[@]}" + else + DOCKER_BUILDKIT=1 "${cmd[@]}" >/dev/null + fi
360-383: Nice summary output; minor: align headers for long service names.If new services have longer names, consider dynamic column widths; current is fine for the four services.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.github/workflows/docker-smoke.yml(1 hunks)apps/admin/Dockerfile.admin(2 hunks)apps/live/Dockerfile.live(2 hunks)apps/space/Dockerfile.space(2 hunks)apps/web/Dockerfile.web(2 hunks)scripts/bench-builds.sh(1 hunks)scripts/smoke.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/docker-smoke.yml
- scripts/smoke.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build and smoke test web
- GitHub Check: Build and smoke test admin
- GitHub Check: Build and smoke test live
- GitHub Check: Build and smoke test space
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (8)
apps/space/Dockerfile.space (1)
100-100: Good hardening: production runtime.Setting
NODE_ENV=productionin the runner stage is correct and prevents dev-mode behavior.apps/web/Dockerfile.web (1)
117-117: Good hardening: production runtime.apps/live/Dockerfile.live (2)
47-47: Nice: prune dev deps in runtime image.
pnpm prune --prodafter build reduces attack surface and image size.
67-67: Good hardening: production runtime.apps/admin/Dockerfile.admin (1)
100-100: Good hardening: production runtime.scripts/bench-builds.sh (3)
28-41: Bash 4+ guard: good fail-fast UX.Clear messaging for macOS users; early exit prevents confusing runtime errors with associative arrays.
79-125: now_ns(): robust cross-platform detection — nice.Validates
%s%Noutput, falls back through EPOCHREALTIME and perl to seconds. Solid and portable.
310-315: Preflight docker checks: great.Verifying both CLI presence and daemon reachability avoids late-stage failures.
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
from
01dd81c to
c9571a4
Compare
lifeiscontent
force-pushed
the
feat/docker-frozen-lock
branch
from
c9571a4 to
8b1d431
Compare
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Tests
Chores