-
Notifications
You must be signed in to change notification settings - Fork 11
(deployment) implement production docker images #451
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
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce Docker-based publishing workflows for frontend and backend services, refactor the build process to dynamically generate nginx configuration from templates using sed replacements, update the backend Dockerfile with non-interactive apt operations and conditional database migrations, and redesign the frontend runtime using an nginx container with environment-based configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
bed6cf1 to
e0b54b4
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
🧹 Nitpick comments (3)
src/backend/entrypoint (1)
34-38: Consider adding error handling for migration failures.If migrations fail, the script continues to execute the final command. For production, a failed migration should typically halt startup to prevent running the application against an inconsistent database schema.
# This _HAS_ to be "true" on a single backend instance if [ "${ENABLE_DB_MIGRATIONS:-false}" = "true" ]; then echo "🐳(entrypoint) running django migrations..." - python manage.py migrate --noinput + python manage.py migrate --noinput || exit 1 fisrc/frontend/Dockerfile (1)
27-34: Consider addingEXPOSEfor documentation and verify default backend server.The runtime stage looks good. Two suggestions:
- Add
EXPOSE ${MESSAGES_FRONTEND_PORT}for container port documentation.- The default
localhost:8000forMESSAGES_FRONTEND_BACKEND_SERVERwill only work in host network mode or when backend runs in the same container—ensure this is documented or consider a more obvious placeholder.FROM docker.io/nginxinc/nginx-unprivileged:1-alpine AS runtime-prod COPY --from=frontend-build /home/frontend/out /app COPY nginx/nginx.conf.template /etc/nginx/templates/default.conf.template ENV MESSAGES_FRONTEND_PORT=8080 ENV MESSAGES_FRONTEND_BACKEND_SERVER=localhost:8000 ENV DJANGO_ADMIN_URL=admin + +EXPOSE 8080src/frontend/nginx/nginx.conf.template (1)
18-19: Minor: Inconsistent indentation in closing braces.Lines 19 and 41 have single-space indentation before the closing brace, while other blocks use consistent 4-space indentation.
proxy_redirect off; proxy_pass http://backend_server; - } + }Also applies to: 40-41
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/messages.yml(1 hunks)bin/scalingo_postfrontend(1 hunks)src/backend/Dockerfile(4 hunks)src/backend/entrypoint(1 hunks)src/frontend/Dockerfile(2 hunks)src/frontend/nginx/nginx.conf.template(2 hunks)
🔇 Additional comments (8)
bin/scalingo_postfrontend (1)
17-21: LGTM!The sed transformation order is correct—specific replacements are applied before the generic pattern, ensuring proper conversion to Scalingo's ERB format.
.github/workflows/messages.yml (1)
152-176: LGTM!The new publish jobs follow the established pattern and have appropriate permissions for GHCR publishing with attestations.
src/backend/Dockerfile (4)
7-11: LGTM!The heredoc syntax improves readability and
DEBIAN_FRONTEND="noninteractive"correctly prevents apt prompts during build.
20-27: Verify iflibmagic1is needed in the poetry stage.
libmagic1is a runtime library. If it's needed for compiling python-magic, you might needlibmagic-devinstead. If it's only required at runtime, it could potentially be removed from this stage since it's already installed inruntime-base.
65-69: LGTM!The heredoc consolidation for collectstatic and rdfind commands improves readability.
80-86: LGTM!Runtime dependencies are correctly installed with noninteractive apt and cleanup.
src/frontend/nginx/nginx.conf.template (2)
71-73: Verify thetry_filesdirective syntax.The current syntax
try_files $uri index.html $uri/ =404may not behave as expected. Typically for serving a static site with an index fallback, it should be:try_files $uri $uri/ /index.html =404;Or if you want to serve
index.htmlas a literal fallback for SPA routing:try_files $uri $uri/ /index.html;The current form tries
$uri, then literallyindex.htmlin the current directory, then$uri/, then 404.
22-24: Admin redirect assumes HTTPS termination.The redirect to
https://$host/${DJANGO_ADMIN_URL}/hardcodes HTTPS. This is appropriate if a load balancer or reverse proxy handles TLS termination, but will cause issues in local development or HTTP-only environments.Confirm this is the intended behavior for your deployment setup.
|
About the migrations, for now we use the ENABLE_DB_MIGRATIONS env var in the entrypoint, if this var is true the instance starts the migrations before starting gunicorn. Other solutions include :
I'd go for either the ENABLE_DB_MIGRATIONS or the dedicated container during deployment, the others look too complicated. @sylvinus wdyt ? |
| try_files /domain/[maildomainId]/signatures.html =404; | ||
| } | ||
|
|
||
| location /health { |
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.
isn't there a more obscure (and standard?) route for this? __lbheartbeat__ ?
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.
underscores are not rfc compliant https://www.rfc-editor.org/rfc/rfc3986#page-17 and there is no standard from what I know. backend has /healthz we can put the same here if that works for you ?
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.
80b9d99 to
77a3317
Compare
f6061c8 to
d927f0b
Compare
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.