Solution by Viktor395 · Pull Request #1009 · mate-academy/py-dockerize-cinema

Viktor395 · 2026-04-19T08:47:09Z

No description provided.

mateacademy-ai-mentor

Review Summary

Changes are needed before this can be accepted, mainly around the Docker/PostgreSQL integration and startup behavior.

Critical issues that must be fixed (blockers):

In cinema_service/settings.py, the DATABASES block (currently using ENGINE: 'django.db.backends.sqlite3') still points to SQLite; this prevents the app from using the PostgreSQL container. Replace it with a PostgreSQL configuration (ENGINE: 'django.db.backends.postgresql') that reads DB_HOST, DB_NAME, DB_USER, DB_PASS, and optional DB_PORT (default 5432) from environment variables so Django connects to the db service defined in docker-compose.yml.
In the same settings file, ALLOWED_HOSTS is an empty list and STATIC_ROOT is missing; this will cause host header issues once DEBUG is off and will break collectstatic/static serving in Docker. Add at least ['127.0.0.1', 'localhost'] (or read from an env var) to ALLOWED_HOSTS and define STATIC_ROOT = BASE_DIR / 'staticfiles', ensuring it matches any static volume you configure in Docker.
In core/management/commands/wait_for_db.py, the loop only accesses connections['default'] but never actually opens a connection, so it may report the DB as ready when PostgreSQL is still starting. Inside the try block, explicitly call something like connections['default'].cursor() or connections['default'].ensure_connection() so an OperationalError is raised until the database is truly available.

Important but non-blocking improvements:

In docker-compose.yml, avoid relying on the .:/app bind mount for a self-contained deployment; instead, bake the code into the image and keep the mount only for local development if needed.
Also in docker-compose.yml, add named volumes for media and static files (for example, media_data:/app/media and static_data:/app/staticfiles) and consider moving database credentials into an .env file or secrets mechanism rather than hardcoding them.
Once you address the PostgreSQL settings, wait_for_db connection attempt, and static/host configuration, your Dockerized setup will satisfy the functional requirements and the rest will be solid refinements of an otherwise good structure.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2026-04-19T08:49:21Z

    "debug_toolbar",
    "cinema",
    "user",
+    "core",


This file still uses SQLite (see DATABASES block) which prevents the app from using the PostgreSQL service defined in docker-compose. Update DATABASES to use the PostgreSQL engine (django.db.backends.postgresql) and read connection parameters from environment variables (DB_HOST, DB_NAME, DB_USER, DB_PASS, optional DB_PORT default 5432) so the app connects to the db container. Also set ALLOWED_HOSTS to include the host(s) you will access (e.g. ['127.0.0.1', 'localhost'] or ['*'] for dev) so runserver 0.0.0.0:8000 is accessible. Finally, add STATIC_ROOT (for example: STATIC_ROOT = BASE_DIR / 'staticfiles') so you can run collectstatic and serve static files from a volume in Docker; MEDIA_ROOT is present but confirm it matches your docker volumes. Optionally, consider sourcing SECRET_KEY and DEBUG from environment variables for better Docker practice.

mateacademy-ai-mentor · 2026-04-19T08:49:21Z

+            try:
+                db_conn = connections['default']


The command does not actually attempt a DB connection. Accessing connections['default'] alone does not necessarily raise an OperationalError if the DB is down. Use connections['default'].cursor() or connections['default'].ensure_connection() inside the try block to attempt a real connection so the loop correctly waits until PostgreSQL is ready.

mateacademy-ai-mentor · 2026-04-19T08:49:21Z

+        while not db_conn:
+            try:
+                db_conn = connections['default']
+            except OperationalError:
+                self.stdout.write('Database unavailable, waiting 1 second...')
+                time.sleep(1)


Consider limiting the retry behavior or adding a timeout/logging: currently this loops indefinitely with 1s sleeps. That's acceptable for many setups, but you may want to surface a clear error after N attempts for easier debugging.

mateacademy-ai-mentor · 2026-04-19T08:49:21Z

+
+class Command(BaseCommand):
+    """Django command to pause execution until database is available"""
+


docker-compose defines postgres but does not expose a DB port to the host (not necessary for container-to-container comms) — fine. However ensure your Django settings read the DB env vars defined below (DB_HOST, DB_NAME, DB_USER, DB_PASS) so the app will actually connect to this postgres service.

mateacademy-ai-mentor · 2026-04-19T08:49:21Z

+
+        self.stdout.write(self.style.SUCCESS('Database available!'))


The app service mounts the project directory into /app (- .:/app). Your Dockerfile switches to a non-root user (django-user), which can cause permission problems when the container user UID does not match the host file ownership. Either run as root during development, adjust ownership in the image (chown) before switching users, or document required permissions to avoid permission errors at runtime.

mateacademy-ai-mentor · 2026-04-19T08:49:22Z

+      sh -c "python manage.py wait_for_db &&
+             python manage.py migrate &&
+             python manage.py runserver 0.0.0.0:8000"


Good: the app command runs python manage.py wait_for_db before migrations and starting the server, which ensures the service will wait for the DB rather than fail on startup.

mateacademy-ai-mentor · 2026-04-19T08:49:22Z

+             python manage.py migrate &&
+             python manage.py runserver 0.0.0.0:8000"
+    volumes:
+      - .:/app


The app service bind-mounts the project directory from the host (- .:/app). That makes the container depend on local files and breaks the requirement that Docker alone should be sufficient. For a self-contained deployment, avoid bind mounts so the image includes the code (or make mounts optional for development only).

mateacademy-ai-mentor · 2026-04-19T08:49:22Z

+    volumes:
+      - .:/app
+    ports:
+      - "8000:8000"


There is no named volume for media/static files. Add volumes for media/ (and use STATIC_ROOT + collectstatic) so media and static can persist in Docker, for example a named volume like - media_data:/app/media and - static_data:/app/staticfiles.

mateacademy-ai-mentor · 2026-04-19T08:49:22Z

+    environment:
+      POSTGRES_DB: cinema_db
+      POSTGRES_USER: cinema_user
+      POSTGRES_PASSWORD: secretpassword


You expose DB credentials directly in the compose file (POSTGRES_PASSWORD for the db and DB_PASS for the app). Avoid hardcoding secrets in the repository; consider using an .env file (kept out of VCS), Docker secrets, or CI/CD injection instead.

mateacademy-ai-mentor · 2026-04-19T08:49:22Z

+      - DB_HOST=db
+      - DB_NAME=cinema_db
+      - DB_USER=cinema_user
+      - DB_PASS=secretpassword


The app receives DB environment variables (DB_HOST, DB_NAME, DB_USER, DB_PASS). Ensure your Django settings actually read these variables and configure the PostgreSQL backend accordingly; otherwise the app will not connect to the postgres service at runtime.

mateacademy-ai-mentor

Review Summary

Your solution is approved because it implements all core Dockerization requirements: the Dockerfile builds a working Django + PostgreSQL image with a non-root user, docker-compose.yml brings up app and db with a wait_for_db management command, and Django is correctly configured via environment variables for the Postgres database, STATIC_ROOT, and ALLOWED_HOSTS.

There are a few non-blocking improvements you could make next. In the Dockerfile, consider adding --no-install-recommends to the apt-get install line and either removing build-time packages (gcc, libpq-dev) after pip install or switching to psycopg2-binary to keep the final image smaller; also prefer COPY --chown=django-user:django-user . . so you can drop the expensive recursive chown -R layer. In docker-compose.yml, the app service currently uses a bind mount .:/app in volumes, which is great for local development but prevents the image from being fully self-contained; for a more production-like setup, move that mount to a separate dev override file or remove it in your main compose. Also in docker-compose.yml, the hardcoded POSTGRES_USER, POSTGRES_PASSWORD, and related DB_* variables in the environment sections should ideally be moved to an .env file or secrets for safer credential handling. As an enhancement, you can add restart: unless-stopped and simple healthcheck definitions for app and db so that containers are automatically restarted and their readiness is visible to orchestration tools.

Overall, you met the functional checklist items well, and these suggestions are about making your images thinner and your deployment more production-ready, which is a strong next step in your learning.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2026-04-19T09:23:50Z

+.gitignore
+__pycache__
+*.pyc
+db.sqlite3


Rather than copying the whole repo and then running chown -R, consider using COPY --chown=django-user:django-user . . so ownership is set at copy time and you can remove the expensive chown -R step (makes the image build faster and layer-smaller). Also ensure your .dockerignore excludes development artifacts (you have one, good).

mateacademy-ai-mentor · 2026-04-19T09:23:50Z

 media
+.venv
+.git
+.gitignore


If your requirements include psycopg2, consider using psycopg2-binary to avoid the need for gcc/libpq-dev at build time, or use a multi-stage build where build dependencies are removed in the final image. This helps keep the runtime image small.

mateacademy-ai-mentor · 2026-04-19T09:23:50Z

+RUN apt-get update && apt-get install -y \
+    gcc \
+    libpq-dev \
+    && rm -rf /var/lib/apt/lists/*


Add --no-install-recommends to the apt-get install command (and consider cleaning up build-time packages after pip install or using a multi-stage build). This reduces image size by avoiding unnecessary recommended packages and allows you to remove build dependencies (gcc/libpq-dev) after they are no longer needed.

mateacademy-ai-mentor · 2026-04-19T09:23:50Z

+RUN pip install --no-cache-dir -r requirements.txt
+
+
+COPY . .


COPY . . will copy all files and then you run a chown later. Prefer using the COPY --chown=django-user:django-user form to set ownership during copy (or create the user before COPY), which avoids an expensive recursive chown at build time.

mateacademy-ai-mentor · 2026-04-19T09:23:50Z

+RUN adduser --disabled-password --no-create-home django-user
+
+
+RUN chown -R django-user:django-user /app


The explicit recursive chown is expensive and can be avoided if you use COPY --chown or if the user is created before copying files. Consider removing this step after changing COPY to set ownership.

mateacademy-ai-mentor · 2026-04-19T09:23:51Z

+    depends_on:
+      - db


depends_on ensures service start order but does not wait for the DB to be ready; your command runs wait_for_db which addresses this. As an enhancement you can also add a short healthcheck for the db service so orchestration and tools can detect readiness more explicitly.

mateacademy-ai-mentor · 2026-04-19T09:23:51Z

+            except OperationalError:
+                self.stdout.write('Database unavailable, waiting 1 second...')
+                time.sleep(1)
+


For production deployments avoid bind-mounting the project source into the container (the .:/app mount). Baking code into the image makes the deployment self-contained; keep bind mounts only for local development to allow live code edits.

mateacademy-ai-mentor · 2026-04-19T09:23:51Z

+    """Django command to pause execution until database is available"""
+
+    def handle(self, *args, **options):
+        self.stdout.write('Waiting for database...')


The apt-get install line should use --no-install-recommends to avoid pulling unnecessary packages. Also consider removing build-time packages after pip install or use a multi-stage build (or psycopg[binary]) to keep the final image thin.

mateacademy-ai-mentor · 2026-04-19T09:23:51Z

+            try:
+                connection = connections['default']
+                connection.cursor() 
+                db_conn = True


You COPY the entire project and then run a recursive chown -R, which creates an extra expensive layer. Prefer creating the user before COPY and use COPY --chown=django-user:django-user . . to set ownership during copy and avoid the chown step.

mateacademy-ai-mentor · 2026-04-19T09:23:51Z

+ALLOWED_HOSTS = os.environ.get("ALLOWED_HOSTS", "127.0.0.1,localhost").split(",")



depends_on only controls start order and does not guarantee the DB is ready. You already run wait_for_db in the app command which is good; as an improvement consider adding a healthcheck for the db service and using depends_on: condition: service_healthy (Compose v3.4+) so Compose can reason about service health explicitly.

Viktor395 added 2 commits April 19, 2026 11:28

Solution

fe89c92

Solution

cd29f69

mateacademy-ai-mentor suggested changes Apr 19, 2026

View reviewed changes

Solution

ac39745

Viktor395 requested a review from mateacademy-ai-mentor April 19, 2026 09:21

mateacademy-ai-mentor approved these changes Apr 19, 2026

View reviewed changes


		class Command(BaseCommand):
		"""Django command to pause execution until database is available"""


		self.stdout.write(self.style.SUCCESS('Database available!')) No newline at end of file

		RUN adduser --disabled-password --no-create-home django-user


		RUN chown -R django-user:django-user /app

		ALLOWED_HOSTS = os.environ.get("ALLOWED_HOSTS", "127.0.0.1,localhost").split(",")

Conversation

Viktor395 commented Apr 19, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants