Fix prod docker image #2618
Fix prod docker image #2618
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Dockerfile and the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Docker
participant Django
User->>Docker: Build Docker Image
Docker->>Django: Set up user/group (runtime)
Docker->>Django: Install packages
Docker->>Django: Set ownership of $APP_HOME
Docker->>Django: Run HEALTHCHECK
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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
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: 0
🧹 Outside diff range and nitpick comments (3)
docker/prod.Dockerfile (3)
59-61: Oh, look who finally decided to add proper user management! 🎉While it's wonderful to see proper user isolation being implemented, we could make it ever so slightly more efficient.
Here's a teensy suggestion to combine the commands:
-RUN addgroup --system django \ - && adduser --system --ingroup django django +RUN addgroup --system django && adduser --system --ingroup django django --no-create-home
67-67: Directory ownership set after package installation, interesting choice...While the ownership change is correct, it might be more logical to set it before installing packages to ensure no root-owned files sneak in.
Consider moving the
chowncommand before the package installation.
Line range hint
71-74: Nice to see proper file permissions, for a change!The ownership and permissions are correctly set, but let's make sure we're being crystal clear about our intentions.
Consider adding a comment explaining the permission choices:
+# Set executable permissions for scripts and ensure proper ownership COPY --chmod=0755 --chown=django:django ./scripts/*.sh $APP_HOME
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
docker/prod.Dockerfile(2 hunks)scripts/celery_beat.sh(0 hunks)
💤 Files with no reviewable changes (1)
- scripts/celery_beat.sh
🔇 Additional comments (2)
docker/prod.Dockerfile (2)
Line range hint 76-76: Verify complete non-root execution
Let's ensure we're not running any sensitive operations as root.
✅ Verification successful
Non-root execution appears to be properly configured... if you must know
The Dockerfile actually demonstrates quite good security practices (not that I'm impressed or anything):
- Creates a non-root user 'django' with minimal system privileges
- Switches to this user with
USER djangobefore running the application - All application files are properly owned by django:django
- Even the shell scripts are given appropriate permissions (0755) and ownership
- The only root operation is during the initial setup, which is, unfortunately, necessary
The only root-related path is the pip cache directory, but this is only used during the builder stage and doesn't persist to the runtime image.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential root operations in scripts
rg -l 'sudo|su root' scripts/
Length of output: 31
Script:
#!/bin/bash
# Let's check the Dockerfile itself for root-related operations and user switching
rg -i 'USER|sudo|root' docker/prod.Dockerfile
# Also check for any chmod/chown operations that might indicate root operations
rg -i 'chmod|chown' docker/prod.Dockerfile
# And let's see the actual content of the Dockerfile to analyze the execution context
cat docker/prod.Dockerfile
Length of output: 2985
82-83: Interesting choice of healthcheck timing parameters...
The reduced start period and new start interval might be a bit aggressive for some deployment scenarios.
Let's verify if these timings align with your orchestration setup:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2618 +/- ##
========================================
Coverage 69.58% 69.58%
========================================
Files 211 211
Lines 11875 11875
Branches 1201 1201
========================================
Hits 8263 8263
Misses 3246 3246
Partials 366 366 ☔ View full report in Codecov by Sentry. |
Proposed Changes
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
djangouser.celery_beat.shscript by removing the command that loads event types, maintaining existing functionality.