Skip to content

Conversation

@zeylos
Copy link
Contributor

@zeylos zeylos commented Dec 10, 2025

  • Adds a runtime-prod target to frontend Dockerfile
  • Minor fixes on backend Dockerfile
  • Adds django migrations in the backend entrypoint (TODO look for improvements before going out of draft)
  • Moves Scalingo nginx ERB file into a reusable nginx container template; adds a sed command to the Scalingo post-frontend script that transforms the generic template into a Scalingo compatible configuration

Summary by CodeRabbit

  • New Features

    • Optional automatic database migrations on application startup
  • Chores

    • Enhanced frontend deployment infrastructure with improved nginx configuration
    • Added configurable backend server address and admin URL paths
    • Improved environment variable handling for flexible deployment customization

✏️ Tip: You can customize this high-level summary in your review settings.

@zeylos zeylos self-assigned this Dec 10, 2025
@zeylos zeylos marked this pull request as draft December 10, 2025 18:42
@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The 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

Cohort / File(s) Summary
GitHub Actions Workflows
.github/workflows/messages.yml
Added two new reusable workflow jobs: docker-publish-frontend and docker-publish-backend, both inheriting from docker-publish.yml with distinct image names, context paths, and targets.
Build Tooling
bin/scalingo_postfrontend
Replaced direct nginx file movement with dynamic configuration generation using sed to transform src/frontend/nginx/nginx.conf.template into servers.conf.erb, applying environment variable substitutions.
Backend Runtime
src/backend/Dockerfile, src/backend/entrypoint
Consolidated Dockerfile commands into heredoc blocks with non-interactive apt behavior, updated MIN\_UPDATE\_DATE to 2025-12-01, and added optional database migration step in entrypoint controlled by ENABLE\_DB\_MIGRATIONS environment variable.
Frontend Runtime
src/frontend/Dockerfile, src/frontend/nginx/nginx.conf.template
Switched frontend runtime from multi-stage build to nginx-unprivileged container, updated nginx configuration to use environment variables (MESSAGES\_FRONTEND\_PORT, MESSAGES\_FRONTEND\_BACKEND\_SERVER, DJANGO\_ADMIN\_URL), and modified routing to support proxying of admin and static asset paths.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Build script sed commands (bin/scalingo_postfrontend): Verify replacement patterns correctly handle all environment variable substitutions and ERB syntax generation
  • Frontend nginx configuration logic (src/frontend/nginx/nginx.conf.template): Review proxy routing changes, redirect logic for admin panel, and static asset handling
  • Backend Dockerfile consolidation (src/backend/Dockerfile): Confirm heredoc command grouping maintains intended execution flow and noninteractive behavior doesn't cause issues
  • Migration conditional implementation (src/backend/entrypoint): Verify ENABLE\_DB\_MIGRATIONS logic and noinput flag behavior

Poem

🐰 With Docker we build, through templates we flow,
Environment vars make the frontend aglow,
Nginx unprivileged, migrations on call,
Sed spins the configs—we publish them all!
From backend to frontend, the pipes now align,
These containers of code? Absolutely divine.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding production Docker image configurations for both frontend and backend services with deployment-focused modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@zeylos zeylos force-pushed the feat/docker_deployment branch 5 times, most recently from bed6cf1 to e0b54b4 Compare December 15, 2025 20:57
@zeylos
Copy link
Contributor Author

zeylos commented Dec 15, 2025

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Dec 15, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a 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
 fi
src/frontend/Dockerfile (1)

27-34: Consider adding EXPOSE for documentation and verify default backend server.

The runtime stage looks good. Two suggestions:

  1. Add EXPOSE ${MESSAGES_FRONTEND_PORT} for container port documentation.
  2. The default localhost:8000 for MESSAGES_FRONTEND_BACKEND_SERVER will 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 8080
src/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

📥 Commits

Reviewing files that changed from the base of the PR and between 36d9352 and f9b801c.

📒 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 if libmagic1 is needed in the poetry stage.

libmagic1 is a runtime library. If it's needed for compiling python-magic, you might need libmagic-dev instead. If it's only required at runtime, it could potentially be removed from this stage since it's already installed in runtime-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 the try_files directive syntax.

The current syntax try_files $uri index.html $uri/ =404 may 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.html as a literal fallback for SPA routing:

try_files $uri $uri/ /index.html;

The current form tries $uri, then literally index.html in 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.

@zeylos
Copy link
Contributor Author

zeylos commented Dec 15, 2025

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.
This works pretty well, it's very simple to maintain and it's relatively common in non-Kubernetes deployments, ("Jobs" don't exist in the compose context), but this may lead to some issues if someone runs 2 instances with ENABLE_DB_MIGRATIONS to true and we don't really have any way to prevent it.

Other solutions include :

  • running the migration on a dedicated container during the deployment process in the ansible collection, but that would require everyone to use the collection or to implement the migration task in their deployment process. This could be an issue for smaller teams that don't use ansible - they'd also probably end up modifying the entrypoint on their end.
  • using https://github.com/ryanhiebert/django-safemigrate, but it does not really look maintained
  • Implement some locks in redis before running the migration, either directly in the entrypoint with redis-cli or with python locks somewhere, but that'd add a hard dependency on redis for deployments. this could cause issues for smaller setups
  • Implement some locks in PG before running the migration, in the entrypoint with pg_advisory_lock. Way harder to maintain IMO and could cause issues on different postgresql versions. Also not really standard

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 {
Copy link
Member

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__ ?

Copy link
Contributor Author

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zeylos zeylos force-pushed the feat/docker_deployment branch from 80b9d99 to 77a3317 Compare December 19, 2025 17:08
@zeylos zeylos force-pushed the feat/docker_deployment branch from f6061c8 to d927f0b Compare December 20, 2025 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants