fix dev docker file and enable sql logging #2649
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several changes to the Django application configuration and Docker setup. It modifies the Django settings to conditionally include the Changes
Possibly related PRs
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
config/settings/local.py (1)
22-24: LGTM, but the SILKY_PYTHON_PROFILER setting needs attention.The conditional loading of django-silk is a nice touch for optimizing development resources. However, I couldn't help but notice that the
SILKY_PYTHON_PROFILER = Truesetting below remains unconditional, which might cause issues when silk is disabled.Consider wrapping the SILKY_PYTHON_PROFILER setting in the same conditional block:
if env("ENABLE_SILK", default=False): INSTALLED_APPS += ["silk"] MIDDLEWARE += ["silk.middleware.SilkyMiddleware"] + SILKY_PYTHON_PROFILER = True -SILKY_PYTHON_PROFILER = True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
config/settings/local.py(1 hunks)docker/.local.env(1 hunks)docker/dev.Dockerfile(1 hunks)scripts/start-dev.sh(1 hunks)
🔇 Additional comments (4)
config/settings/local.py (1)
Line range hint 1-100: Verify development-only settings.
Just want to make sure these development settings (CORS_ORIGIN_ALLOW_ALL, DISABLE_RATELIMIT) aren't somehow finding their way into production.
✅ Verification successful
Development settings are properly isolated, though you might want to double-check that environment variable
Well, well, well... looks like everything is actually quite properly set up (how refreshing). The production environment:
- Uses
CORS_ALLOWED_ORIGINSwith an explicit list instead of allowing all origins - Doesn't have
CORS_ORIGIN_ALLOW_ALLanywhere in sight - Has
DISABLE_RATELIMITdefaulting toFalsein base.py, and isn't overridden
Though, I suppose I should mention that DISABLE_RATELIMIT is controlled by an environment variable. You might want to ensure it's explicitly set to False in your production environment, just in case someone gets any funny ideas about environment variables.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these development settings exist in production settings
echo "Checking for potentially unsafe settings in production..."
rg -l "CORS_ORIGIN_ALLOW_ALL\s*=\s*True|DISABLE_RATELIMIT\s*=\s*True" config/settings/production.py
Length of output: 226
Script:
#!/bin/bash
# Let's check if production.py exists and see its content
echo "First, checking if production.py exists..."
fd -t f "production.py" config/settings/
echo -e "\nChecking production settings content..."
cat config/settings/production.py 2>/dev/null || echo "File not found"
echo -e "\nChecking for CORS settings in production..."
rg "CORS_" config/settings/production.py || echo "No CORS settings found"
echo -e "\nChecking for rate limit settings in production..."
rg "RATELIMIT" config/settings/production.py || echo "No rate limit settings found"
# Let's also check if there are any other settings files that might be used in production
echo -e "\nChecking for other potential settings files..."
fd -t f . config/settings/
Length of output: 1270
Script:
#!/bin/bash
# Let's check deployment.py since production.py imports from it
echo "Checking deployment.py content..."
cat config/settings/deployment.py 2>/dev/null || echo "File not found"
echo -e "\nChecking for CORS settings in deployment.py..."
rg "CORS_" config/settings/deployment.py || echo "No CORS settings found"
echo -e "\nChecking for rate limit settings in deployment.py..."
rg "RATELIMIT" config/settings/deployment.py || echo "No rate limit settings found"
# Let's also check base.py for default settings
echo -e "\nChecking relevant settings in base.py..."
rg "(CORS_|RATELIMIT)" config/settings/base.py || echo "No relevant settings found in base.py"
Length of output: 5861
scripts/start-dev.sh (1)
14-18: Oh, I see we're finally standardizing on runserver_plus...
The changes look reasonable, though it's interesting that we're now printing SQL queries regardless of the debug state. I suppose more information is... better?
Let's make sure this won't flood our logs too much:
✅ Verification successful
SQL logging seems... manageable, I suppose
Looking at the query patterns across the codebase, most of them are simple filters and joins that won't generate particularly verbose SQL. The queries are primarily:
- Basic filtering on foreign keys and boolean fields
- Simple text searches and annotations
- Standard Django ORM operations (select_related, prefetch_related)
While we'll see more SQL in the logs, it won't be overwhelming since there aren't any particularly complex or chatty queries that would flood the output.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if we have any particularly chatty SQL queries in our codebase
rg -l "select|SELECT" --type py | while read -r file; do
echo "=== $file ==="
ast-grep --pattern 'queryset.$$$' "$file"
done
Length of output: 18229
docker/.local.env (1)
10-11: Ah, enabling debug mode by default... how brave.
While this makes sense for a development environment, let's make absolutely sure this won't accidentally slip into production:
✅ Verification successful
Well, someone has their debug settings under control... mostly.
The debug flag appears to be properly configured:
- Only
.local.envhas DEBUG set to true - The Django settings use environment variables with a safe default (
DEBUG = env.bool("DJANGO_DEBUG", False)) - Production configurations in
.prebuilt.envdon't show any concerning debug settings - Docker Compose files correctly separate development and production environments
Though I suppose it wouldn't hurt to have a .env.example file to prevent any... unfortunate misconfigurations in the future.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any production environment files that might have DEBUG enabled
rg -l "DJANGO_DEBUG.*true" --glob '*.env' --glob '*.env.*' | grep -v "local.env"
# Also check for any Django settings files that might have hardcoded DEBUG=True
ast-grep --pattern 'DEBUG = True'
Length of output: 116
Script:
#!/bin/bash
# Let's cast a wider net for any debug-related settings in env files
rg -l "DEBUG.*=.*true" --glob '*.env*'
# Check for Django settings with DEBUG in a more flexible way
ast-grep --pattern 'DEBUG = $_'
# Look for any docker-compose files that might reference these env files
fd -e yml -e yaml docker-compose | xargs cat
# And check other potential env templates
fd ".env.template|.env.example|.env.sample"
Length of output: 2800
docker/dev.Dockerfile (1)
Line range hint 14-26: Proper architecture handling - how thoughtful.
The architecture check for Typst installation is well implemented, though it might be worth noting that we're making some assumptions about supported architectures.
Let's check if we handle architecture-specific cases consistently:
✅ Verification successful
Architecture handling is consistently implemented across both Dockerfiles
The architecture checks for Typst installation are identical in both dev and prod Dockerfiles, and I see no other architecture-dependent installations that might have been... overlooked. The implementation gracefully handles both amd64 and arm64 architectures while thoughtfully failing on unsupported ones - which is more than what some other projects do.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for other architecture-dependent installations
fd Dockerfile | while read -r file; do
echo "=== $file ==="
rg -A 5 "dpkg --print-architecture|uname -m" "$file"
done
Length of output: 801
Script:
#!/bin/bash
# Let's check if there are any other architecture-specific binaries or installations
rg -i "download|install|binary|bin" docker/ | grep -i "arch\|x86\|amd64\|arm"
Length of output: 698
| if env("ENABLE_SILK", default=False): | ||
| INSTALLED_APPS += ["silk"] | ||
| MIDDLEWARE += ["silk.middleware.SilkyMiddleware"] |
There was a problem hiding this comment.
💡 Codebase verification
Would be lovely if we could add documentation for the ENABLE_SILK environment variable
The local setup documentation in docs/local-setup/configuration.rst seems to be missing any mention of this rather important development feature. Since developers might want to use Django Silk for profiling, it would be quite helpful to document this configuration option alongside other environment variables.
- Add documentation about
ENABLE_SILKenvironment variable indocs/local-setup/configuration.rst, perhaps under the development environment setup section - Include a brief explanation of what Silk does and when developers might want to enable it
🔗 Analysis chain
Documentation update might be nice.
Since this introduces a new environment variable that affects development workflow, it would be absolutely wonderful if we could document this change in the development setup guide.
Let's check if there's a relevant documentation file:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for development setup documentation
fd -t f -e md -e rst "setup|development|configuration" docs/
Length of output: 421
Script:
#!/bin/bash
# Check content of local setup configuration docs
cat docs/local-setup/configuration.rst
# Also check if there's any existing mention of silk in docs
rg -i "silk" docs/
Length of output: 5087
| RUN --mount=type=cache,target=/root/.cache/pip pip install pipenv==2024.4.0 | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Looks like we found a lonely pip install without cache in prod.Dockerfile
The build cache optimization is only implemented in dev.Dockerfile but not in prod.Dockerfile. For consistency and build performance, the same cache mount should be added to the pip install command in prod.Dockerfile as well.
docker/prod.Dockerfile: Add--mount=type=cache,target=/root/.cache/pipto the pip install command
🔗 Analysis chain
Finally, someone remembered that build caches exist!
Nice optimization using --mount=type=cache. This should make our builds significantly faster, assuming everyone remembers to enable BuildKit.
Let's check if we're consistent with cache usage across our Dockerfiles:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check other Dockerfiles for potential cache optimization opportunities
fd Dockerfile | while read -r file; do
echo "=== $file ==="
rg "pip install" "$file"
done
Length of output: 392
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2649 +/- ##
========================================
Coverage 69.63% 69.63%
========================================
Files 211 211
Lines 11879 11879
Branches 1202 1202
========================================
Hits 8272 8272
Misses 3240 3240
Partials 367 367 ☔ 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
New Features
Bug Fixes
Refactor