docker: add multi-stage Dockerfile for session-api service#5
docker: add multi-stage Dockerfile for session-api service#5Shubham-Aswekar wants to merge 2 commits intohemanth5544:mainfrom
Conversation
📝 WalkthroughWalkthroughA new multi-stage Dockerfile has been introduced for the API service. The builder stage installs dependencies and builds the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Pre-merge checks✅ Passed checks (3 passed)
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: 5
🧹 Nitpick comments (2)
apps/api/Dockerfile (2)
9-9: Custom PATH modification is likely unnecessary.The custom PATH with workspace-specific
node_modules/.bindirectories is unnecessary when using npm workspaces. Thenpm runcommands automatically resolve binaries from the appropriate workspace context.🔎 Simplification
-ENV PATH="/app/node_modules/.bin:/app/packages/logger/node_modules/.bin:/app/apps/api/node_modules/.bin:$PATH" - RUN npm install
28-28: Prefer running Node.js directly over npm.Using
npm run startadds unnecessary overhead and prevents proper signal handling (SIGTERM/SIGINT). Node.js should be run directly in production containers for better performance and graceful shutdown behavior.🔎 Run node directly
Assuming your start script runs something like
node dist/index.js, replace npm with:-CMD ["npm","run","start"] +CMD ["node", "dist/index.js"]If you need environment-specific configuration, you can also use:
-CMD ["npm","run","start"] +CMD ["node", "--enable-source-maps", "dist/index.js"]This provides better signal handling for graceful shutdown when the container stops.
|
|
||
| ENV PATH="/app/node_modules/.bin:/app/packages/logger/node_modules/.bin:/app/apps/api/node_modules/.bin:$PATH" | ||
|
|
||
| RUN npm install |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use npm ci for reproducible builds.
npm install can install different dependency versions if package-lock.json is out of sync, leading to inconsistent builds. Use npm ci for reproducible, deterministic builds in CI/CD and Docker environments.
-RUN npm install
+RUN npm ci📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN npm install | |
| RUN npm ci |
🤖 Prompt for AI Agents
In apps/api/Dockerfile around line 11, the Dockerfile uses "RUN npm install"
which can yield non-deterministic installs; replace that command with "RUN npm
ci" to ensure reproducible, lockfile-driven installs (for production images
consider "RUN npm ci --only=production" or equivalent flags to skip dev deps).
| FROM node:20-alpine | ||
|
|
||
| WORKDIR /app | ||
|
|
||
| COPY --from=builder /app/apps/api/package.json . | ||
| COPY --from=builder /app/apps/api/dist/ /app/dist/ | ||
| COPY --from=builder /app/node_modules /app/node_modules | ||
| COPY --from=builder /app/packages/logger /app/packages/logger |
There was a problem hiding this comment.
Critical: Container runs as root user.
The runtime image doesn't specify a USER directive, causing the container to run as root. This violates security best practices and increases the attack surface if the container is compromised.
🔎 Add non-root user
FROM node:20-alpine
WORKDIR /app
+# Create non-root user
+RUN addgroup -g 1001 -S nodejs && \
+ adduser -S nodejs -u 1001
+
COPY --from=builder /app/apps/api/package.json .
COPY --from=builder /app/apps/api/dist/ /app/dist/
COPY --from=builder /app/node_modules /app/node_modules
COPY --from=builder /app/packages/logger /app/packages/logger
+RUN chown -R nodejs:nodejs /app
+
+USER nodejs
+
EXPOSE 2000
CMD ["npm","run","start"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FROM node:20-alpine | |
| WORKDIR /app | |
| COPY --from=builder /app/apps/api/package.json . | |
| COPY --from=builder /app/apps/api/dist/ /app/dist/ | |
| COPY --from=builder /app/node_modules /app/node_modules | |
| COPY --from=builder /app/packages/logger /app/packages/logger | |
| FROM node:20-alpine | |
| WORKDIR /app | |
| # Create non-root user | |
| RUN addgroup -g 1001 -S nodejs && \ | |
| adduser -S nodejs -u 1001 | |
| COPY --from=builder /app/apps/api/package.json . | |
| COPY --from=builder /app/apps/api/dist/ /app/dist/ | |
| COPY --from=builder /app/node_modules /app/node_modules | |
| COPY --from=builder /app/packages/logger /app/packages/logger | |
| RUN chown -R nodejs:nodejs /app | |
| USER nodejs | |
| EXPOSE 2000 | |
| CMD ["npm","run","start"] |
🤖 Prompt for AI Agents
In apps/api/Dockerfile around lines 17-24 the container never switches from root
to a non-root user; create a non-root user and switch to it, ensure
files/directories under /app are owned by that user and writable where needed,
and set the USER at the end of the Dockerfile. Specifically: add steps to create
a user/group (or use the existing node user in the base image), chown /app and
any runtime-writeable dirs to that UID/GID, avoid binding to privileged ports,
and add a USER directive so the container does not run as root at runtime.
| COPY --from=builder /app/apps/api/package.json . | ||
| COPY --from=builder /app/apps/api/dist/ /app/dist/ | ||
| COPY --from=builder /app/node_modules /app/node_modules | ||
| COPY --from=builder /app/packages/logger /app/packages/logger |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check API dependencies and workspace configuration
echo "=== API package.json dependencies ==="
cat apps/api/package.json | jq '.dependencies, .devDependencies' 2>/dev/null || cat apps/api/package.json
echo -e "\n=== Logger package name ==="
cat packages/logger/package.json | jq '.name' 2>/dev/null || cat packages/logger/package.json
echo -e "\n=== Root package.json workspaces ==="
cat package.json | jq '.workspaces' 2>/dev/null || cat package.json
echo -e "\n=== Dockerfile content (apps/api/Dockerfile) ==="
cat apps/api/DockerfileRepository: hemanth5544/quicksync
Length of output: 1250
🏁 Script executed:
#!/bin/bash
# Check if logger package is meant to be published or workspace-only
echo "=== Logger package.json (complete) ==="
cat packages/logger/package.json
echo -e "\n=== Checking for .npmrc or registry config ==="
cat .npmrc 2>/dev/null || echo "No .npmrc file"
echo -e "\n=== Checking for npm publish scripts in root package.json ==="
cat package.json | jq '.scripts | keys[] | select(contains("publish"))'
echo -e "\n=== Checking root package.json private field ==="
cat package.json | jq '.private'Repository: hemanth5544/quicksync
Length of output: 828
Change API's dependency on @quick-sync/logger to use workspace protocol.
The API references @quick-sync/logger with version "*", which causes npm to attempt resolution from the npm registry. Since the logger package is marked as "private": true and is not published, this reference will fail to resolve at runtime. During the build stage in the monorepo context, this resolves correctly, but the flattened runtime structure copies node_modules without a proper mechanism to resolve the workspace dependency.
Change the dependency in apps/api/package.json to use the workspace protocol:
"@quick-sync/logger": "workspace:*"This ensures the dependency resolves correctly within the monorepo structure during both build and runtime.
🤖 Prompt for AI Agents
In apps/api/Dockerfile around lines 21 to 24, the runtime image copies
node_modules and a private workspace package but the API's package.json
currently depends on @quick-sync/logger as "*" which causes npm to try the
registry and fail; update apps/api/package.json to set the logger dependency to
the workspace protocol ("@quick-sync/logger": "workspace:*"), then rebuild the
image so the workspace dependency is resolved locally and included correctly in
the runtime build.
|
|
||
| COPY --from=builder /app/apps/api/package.json . | ||
| COPY --from=builder /app/apps/api/dist/ /app/dist/ | ||
| COPY --from=builder /app/node_modules /app/node_modules |
There was a problem hiding this comment.
Production image includes development dependencies.
Copying the entire node_modules directory from the builder includes all development dependencies (testing frameworks, build tools, etc.), significantly increasing image size and exposing unnecessary packages that could contain security vulnerabilities.
🔎 Install production dependencies only
Instead of copying node_modules from builder, install production dependencies in the runtime stage:
FROM node:20-alpine
WORKDIR /app
COPY --from=builder /app/apps/api/package.json .
COPY --from=builder /app/apps/api/dist/ /app/dist/
-COPY --from=builder /app/node_modules /app/node_modules
COPY --from=builder /app/packages/logger /app/packages/logger
+
+# Copy only root package files needed for workspace resolution
+COPY --from=builder /app/package*.json ./
+
+# Install production dependencies only
+RUN npm ci --only=production --workspace=@quick-sync/session-api
EXPOSE 2000Note: This assumes workspace dependencies are properly configured. You may need to adjust the approach based on your workspace setup.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/Dockerfile around line 23, the Dockerfile copies the entire
node_modules from the builder which brings dev dependencies into the production
image; instead, remove the COPY --from=builder /app/node_modules step and in the
final/runtime stage copy only package.json and package-lock.json (or appropriate
lockfiles) from the builder and run a production-only install (e.g. npm ci
--omit=dev or npm ci --production depending on your npm version) so only
production dependencies are installed into the runtime image.
1 participant
What this PR does
Notes
wsandwebservices will be added in follow-up PRsHow to run