fix: docker files for db and app#1127
Conversation
WalkthroughThe changes transition the Docker build and runtime environments from Bun to Node.js with pnpm as the package manager. Dockerfiles for application and database services are updated accordingly, and the docker-compose production configuration for migrations now uses pnpm instead of Bun for running database migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Docker as Docker Build
participant Node as Node.js + pnpm
participant App as Application
participant DB as Database
Dev->>Docker: Build app/db images
Docker->>Node: Use node:22-alpine base image
Docker->>Node: Install pnpm globally
Docker->>Node: Install dependencies with pnpm
Docker->>App: Build application with pnpm run build
Docker->>DB: Install and prepare server/db with pnpm
Dev->>Docker: Run migrations service
Docker->>Node: Use pnpm run db:migrate
Node->>DB: Execute database migrations
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
docker/db/Dockerfile (4)
4-7: Pinpnpmfor reproducible builds
Switching tonode:22-alpineis correct, but installing the latestpnpmcan introduce breaking changes over time. Pin to the version matching your lockfile.- RUN npm install -g pnpm + RUN npm install -g pnpm@<your-locked-version>
16-16: Good use of frozen lockfile
Usingpnpm install --frozen-lockfileguarantees deterministic installs. If you later hit peer-dependency conflicts, consider adding--shamefully-hoist.
21-24: Eliminate redundant global install in runner
You installpnpmagain in the runner stage, but since you copy the fullpnpmbinary fromdeps, this step can be removed to slim down the image:- FROM node:22-alpine AS runner - RUN npm install -g pnpm + FROM node:22-alpine AS runner
26-30: Maintain cache-friendly layer order
Copying lockfiles,node_modules, and workspace configs before other files is ideal for cache utilization. Consider adding a brief comment to document this pattern so future contributors understand the rationale.docker/app/Dockerfile (2)
31-40: Optimize dependency install caching
You currently copy source (COPY . .) beforeRUN pnpm install, invalidating the cache on any code change. To speed rebuilds, invert the steps:COPY package.json pnpm-lock.yaml pnpm-workspace.yaml turbo.json ./ RUN pnpm install --frozen-lockfile COPY . .
56-59: Review runtime vs. build-time dependencies
You installwranglerin the final image. Ifwrangleris only needed for the build, remove it here to reduce image size. If it’s required at runtime, pin its version for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docker-compose.prod.yaml(1 hunks)docker/app/Dockerfile(3 hunks)docker/db/Dockerfile(1 hunks)
🔇 Additional comments (7)
docker-compose.prod.yaml (1)
47-47: Verify migration script existence in workspace
You’ve updated themigrationsservice to use['pnpm', 'run', 'db:migrate']. Ensure that thedb:migratescript is defined in the correctpackage.json(root orapps/serverworkspace) so the command resolves at/app/apps/server.docker/db/Dockerfile (3)
10-13: Confirm workspace packages coverage
You now copy all workspace configs and packages (apps/server,packages/,patches/). Double-check thatpnpm-workspace.yamlincludes these paths and no directories are inadvertently omitted.
32-34: Ensure correct working directory for migrations
You copy bothapps/server/and workspace packages before settingWORKDIR /app/apps/server. Confirm that all migration scripts and config files live under/app/apps/server.
36-36: Working directory correctly set for migration commands
SettingWORKDIR /app/apps/serveraligns with yourpnpm run db:migratecontext. This looks good.docker/app/Dockerfile (3)
4-4: Migrate base image from Bun to Node.js
Switching fromoven/bun:alpinetonode:22-alpineis the core change. Verify that no Bun-specific assumptions (e.g., environment variables or runtime flags) remain in your app.
47-48: Verify build script path
The build commandcd apps/mail && pnpm run buildmust match your workspace layout. Confirmapps/mail/package.jsondefines abuildscript and that this path is correct.
71-72: Proper ownership of build artifacts
Using--chown=nextjs:pnpmensures the non-rootnextjsuser owns the application files—excellent for security.
BlankParticle
force-pushed
the
staging
branch
6 times, most recently
from
e47318e to
a6ec63a
Compare
|
@MrgSub Any reviews here? |
|
Any updates on this? Your docker files not working on the main branch |
Merge activity
|
|
@MrgSub, Why do you need AI to merge a PR? Just curious. |
We use Graphite to auto-rebase and avoid conflicts, and streamline merging |
3 participants
Description
Migrated from Bun to Node.js with pnpm across Docker configurations. This change replaces Bun with pnpm for dependency management, build processes, and runtime execution in both the application and database Dockerfiles. The migration includes updating base images from
oven/bun:alpinetonode:22-alpine, modifying installation commands, and adjusting file copying processes to work with the pnpm workspace structure.Type of Change
Areas Affected
Testing Done
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.