-
Notifications
You must be signed in to change notification settings - Fork 316
feat: Added support for Multi-Stage Docker build #827
Conversation
WalkthroughThe Dockerfile has been restructured to implement a multi-stage build process, optimizing the final image size and improving caching. The Changes
Assessment against linked issues
Poem
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- .dockerignore (1 hunks)
- Dockerfile (1 hunks)
Files skipped from review due to trivial changes (1)
- .dockerignore
Additional comments: 1
Dockerfile (1)
- 1-13: The Multi-Stage Docker build is correctly implemented. Each stage is clearly defined and serves a specific purpose, which is good for maintainability and readability. The use of
node:17-alpine
is appropriate for a lightweight image, and separating the dependency installation (deps
stage) from the build process (builder
stage) can indeed leverage Docker's caching mechanism to speed up builds when dependencies don't change.However, there are a few points to consider:
- Ensure that Node.js 17 is compatible with all project dependencies, as some might not yet support newer Node.js versions.
- Verify that
pm2
is the best choice for the project's needs. While it's a powerful process manager, it might introduce unnecessary complexity if the application doesn't require its advanced features.- The
--no-daemon
flag in theENTRYPOINT
is important for Docker containers as it keeps the process in the foreground, which is a Docker best practice.- The
--spa
flag is used to serve single-page applications, which is suitable if the project is indeed an SPA.- The
COPY --from=builder build ./build
command assumes that the build process outputs to abuild
directory. Ensure this matches the actual output directory of the build process.- The
EXPOSE 3000
instruction is good for documentation purposes, but remember that it doesn't actually publish the port. The port needs to be published when running the container using the-p
flag withdocker run
.Overall, the Dockerfile looks well-structured and optimized for the project's requirements. Just ensure that all the assumptions made (like Node.js version compatibility and the output directory) are indeed correct and that
pm2
is the right choice for the project.
@AnuragThePathak Should I also add docker-compose.yml file to automatically set the port required? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- Dockerfile (1 hunks)
Additional comments: 1
Dockerfile (1)
- 1-13: The Multi-Stage Docker build is correctly implemented with separate stages for dependencies, building, and running the application. This should indeed improve caching and build times, as well as ensure a smaller final image size because only the necessary artifacts are included in the
runner
stage.
- Using
node:17-alpine
is a good choice for keeping the image size small, but ensure that Node.js 17 is compatible with all project dependencies since it's not an LTS version and might introduce breaking changes or deprecations.- The
COPY --from=builder build ./build
command correctly copies only the build artifacts from thebuilder
stage to therunner
stage.- Installing
pm2
globally in therunner
stage is appropriate for process management.- The
EXPOSE 3000
instruction indicates that the application will be available on port 3000, which is standard for web applications.- Using
ENTRYPOINT
withpm2
is a good practice to ensure that the process manager starts with the container and the specified parameters are used. However, ensure that the--no-daemon
flag works as intended with the Docker container lifecycle.- The
--spa
flag is correctly used for serving single-page applications, which require all requests to be redirected to the index.html file.Overall, the Dockerfile changes seem well-structured and thought out. Just ensure compatibility with Node.js 17 and that the
pm2
flags are suitable for your deployment environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @AnuragThePathak for reviewing and merging this PR 😄! |
Fixes Issue
Fixes #814
Changes proposed
deps
,builder
andrunner
to install dependencies, build the source code and run (serve) the build directory respectively.pm2
to serve the build directory as serve did not work for all the other webpages and Nginx failed for the same reason.Check List (Check all the boxes which are applicable)
Screenshots
Summary by CodeRabbit
Chores
.dockerignore
to enhance build context by excluding unnecessary files and directories such as configuration files, documentation, and dependencies directory.Refactor
New Features
pm2
for efficient process management when serving the application.Style
CMD
instruction toENTRYPOINT
to better handle the serving of the application usingpm2
.