Avoid duplicate backend image builds in Docker Compose#2934
Avoid duplicate backend image builds in Docker Compose#2934arkid15r merged 4 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughCompose files updated to reuse a prebuilt backend image for worker services (instead of building twice), add startup ordering so workers wait for backend Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (2)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docker-compose/local.yaml (2)
25-26: Critical: Fix the Redis password configuration bug in the backend service.Line 26 uses
${DJANGO_REDIS_HOST:-nest-cache-password}to setDJANGO_REDIS_PASSWORD, which is incorrect—it should reference theDJANGO_REDIS_PASSWORDenvironment variable, notDJANGO_REDIS_HOST. This bug causes the Redis password to be misconfigured.The worker service correctly fixes this at line 133, but the backend service still has the bug.
Apply this diff to fix the Redis password configuration:
DJANGO_REDIS_HOST: ${DJANGO_REDIS_HOST:-nest-cache} - DJANGO_REDIS_PASSWORD: ${DJANGO_REDIS_HOST:-nest-cache-password} + DJANGO_REDIS_PASSWORD: ${DJANGO_REDIS_PASSWORD:-nest-cache-password}
25-25: Inconsistent Redis host defaults between backend and worker.The backend service defaults
DJANGO_REDIS_HOSTtonest-cache(line 25), but the worker service defaults it tocache(line 132). Since the Redis service in local.yaml is namedcache(line 35), the worker's default is correct and the backend's is wrong.Both services should use the same default to avoid configuration inconsistencies.
Apply this diff to align the backend's Redis host default:
- DJANGO_REDIS_HOST: ${DJANGO_REDIS_HOST:-nest-cache} + DJANGO_REDIS_HOST: ${DJANGO_REDIS_HOST:-cache} DJANGO_REDIS_PASSWORD: ${DJANGO_REDIS_PASSWORD:-nest-cache-password}Also applies to: 132-132
♻️ Duplicate comments (1)
docker-compose/staging.yaml (1)
70-71: Same concern as production: clarify the startup dependency rationale.Like production, the staging environment already shares the same prebuilt image (
owasp/nest:backend-staging) between services—no duplicate builds occur. The added startup dependency raises the same questions about necessity and implementation as noted in the production.yaml review.
🧹 Nitpick comments (1)
docker-compose/local.yaml (1)
119-120: The startup dependency doesn't guarantee image build completion.While using
service_startedensures the backend container starts before the worker, it doesn't explicitly guarantee the image build has completed. In practice, Docker Compose builds all images before starting services, so this should work. However, the dependency seems unnecessary—the worker doesn't need the backend service to be running; it only needs the shared image to exist, which Docker Compose handles automatically.Consider removing this dependency unless there's a specific runtime reason the worker needs the backend service to be started first.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docker-compose/local.yaml(2 hunks)docker-compose/production.yaml(1 hunks)docker-compose/staging.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T10:40:27.924Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2211
File: backend/apps/nest/schedulers/calendar_events/base.py:27-31
Timestamp: 2025-09-17T10:40:27.924Z
Learning: In the OWASP/Nest project, the user ahmedxgouda has confirmed that enqueuing bound model methods (like self.reminder_schedule.reminder.delete) works in their RQ setup in backend/apps/nest/schedulers/calendar_events/base.py, despite potential serialization concerns.
Applied to files:
docker-compose/staging.yamldocker-compose/production.yaml
🔇 Additional comments (4)
docker-compose/local.yaml (3)
9-12: LGTM! Backend image tagging enables reuse.Adding the
imagedirective alongsidebuildcorrectly tags the built image asowasp/nest:backend-local, which allows the worker service to reference and reuse this same image instead of building separately.
117-117: LGTM! Worker now reuses the backend image.Replacing the build directive with the image reference achieves the PR objective of preventing duplicate builds—the worker will use the same image built for the backend service.
133-133: LGTM! Redis password configuration bug fixed in worker.The worker service now correctly references
${DJANGO_REDIS_PASSWORD:-nest-cache-password}instead of the erroneous${DJANGO_REDIS_HOST:-nest-cache-password}. However, the same bug still exists in the backend service at line 26.docker-compose/production.yaml (1)
71-72: Thedepends_onconstraint for backend lacks clear justification and appears unnecessary.The worker service runs independently: it uses
python manage.py rqworker ai --with-schedulerto process background jobs from the RQ queue. Both the worker and backend share the same image and connect independently to the cache and database with their own health checks.No runtime dependency exists—the worker doesn't make HTTP calls to the backend service, and there's no shared initialization logic requiring the backend to start first. If a legitimate startup ordering dependency exists, it should be documented; otherwise, removing this constraint would allow the services to start in parallel and improve startup performance. If a dependency is necessary, using
service_healthyinstead ofservice_startedwould be more appropriate.⛔ Skipped due to learnings
Learnt from: ahmedxgouda Repo: OWASP/Nest PR: 2211 File: docker-compose/production.yaml:21-0 Timestamp: 2025-09-10T16:04:28.502Z Learning: The RQ worker service (backend-worker) in production requires access to various secrets including the GitHub PEM file for potential GitHub API operations, even though primary tasks like Slack messaging use different credentials (SLACK_BOT_TOKEN, SLACK_SIGNING_SECRET). The worker might handle diverse background tasks beyond calendar/Slack operations that could require GitHub authentication.
|
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Looks good, let's try this 👍
3 participants
Resolves #2927
Proposed change
Build the backend image once and used the same image for the worker as both services use the same code so building it twice is unnecessary.
Checklist
make check-testlocally and all tests passed