-
Notifications
You must be signed in to change notification settings - Fork 176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(Dockerfile): use tini from package manager #2533
Conversation
WalkthroughOhayo, sensei! The Dockerfile has been updated to streamline the installation of the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Dockerfile (2)
19-19
: Ohayo again, sensei! Good move on using the absolute path.The update to use the absolute path
/usr/bin/tini
in the ENTRYPOINT is a solid improvement. It reduces ambiguity and aligns with the standard location for binaries installed via the package manager.As a tiny suggestion, consider adding a comment explaining why tini is used as the entrypoint. It could help future maintainers understand the purpose of this setup.
21-21
: Ohayo once more, sensei! Excellent addition of the version check.Adding the tini version check is a smart move. It provides a quick way to verify the correct installation and version of tini, which can be super helpful for debugging.
To make this even more useful, consider capturing the output of the version check. Here's a suggestion:
-RUN /usr/bin/tini --version +RUN echo "Tini version: $(/usr/bin/tini --version)"This way, the version information will be clearly visible in the build logs, making it easier to track which version of tini is included in each Docker image build.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- Dockerfile (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
Dockerfile (2)
3-3
: Ohayo, sensei! Nice simplification of the tini installation.The addition of
tini
to the package installation list streamlines the Dockerfile and aligns perfectly with the PR objective. This change eliminates the need for manual download and permission setting steps. Well done!
Line range hint
3-21
: Ohayo, sensei! Overall, these changes are a dojo-level improvement!The modifications to this Dockerfile achieve the goal of simplifying tini installation and usage. By leveraging the package manager, you've reduced potential points of failure and improved maintainability. The consistent use of absolute paths and the addition of a version check enhance the robustness of the setup.
These changes align well with Docker best practices and should make the image more reliable and easier to maintain. Great work on streamlining the process!
Simplified Dockerfile by installing tini directly via package manager. Removed manual download and permission setting steps.
1a0d2a0
to
2a8e44d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2533 +/- ##
=======================================
Coverage 69.30% 69.30%
=======================================
Files 388 388
Lines 50033 50033
=======================================
Hits 34673 34673
Misses 15360 15360 ☔ View full report in Codecov by Sentry. |
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.
LGTM
ENV TINI_VERSION v0.19.0 | ||
ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini /tini | ||
RUN chmod +x /tini | ||
COPY --from=builder /usr/bin/tini /tini |
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.
copied to /tini instead of /usr/bin/tini for backwards-compatibility
Description
Simplified Dockerfile by installing tini directly
via package manager. Removed manual download and
permission setting steps.
Related issue
Tests
Added to documentation?
Checklist
scripts/prettier.sh
,scripts/rust_fmt.sh
,scripts/cairo_fmt.sh
)scripts/clippy.sh
,scripts/docs.sh
)Summary by CodeRabbit
tini
package.ENTRYPOINT
to use an absolute path for improved reliability.tini
.