Skip to content

feat: Add Docker configuration and wait_for_db management command#1022

Open
Anton924 wants to merge 2 commits into
mate-academy:masterfrom
Anton924:develop
Open

feat: Add Docker configuration and wait_for_db management command#1022
Anton924 wants to merge 2 commits into
mate-academy:masterfrom
Anton924:develop

Conversation

@Anton924
Copy link
Copy Markdown

  • Add Dockerfile with non-root user (my_user) and media directory setup
  • Add docker-compose.yml with cinema_service and db services
  • Add wait_for_db management command to wait for database availability
  • Configure DATABASES settings to use environment variables
  • Add psycopg2-binary in requirements.txt

- Add Dockerfile with non-root user (my_user) and media directory setup
- Add docker-compose.yml with cinema_service and db services
- Add wait_for_db management command to wait for database availability
- Configure DATABASES settings to use environment variables
- Add psycopg2-binary in requirements.txt
Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

The Docker setup has several critical issues that will prevent successful deployment. First, there's a typo in the Dockerfile: PYTHOUNBUFFERED should be PYTHONUNBUFFERED (line where ENV is set), which breaks real-time logging. Second, the ALLOWED_HOSTS = [] setting in settings.py will cause Django to return 400 errors for all requests in Docker - it needs to include ['0.0.0.0', 'localhost', '127.0.0.1']. Third, the requirements.txt file is referenced in the Dockerfile but not provided in the submission, which will cause the Docker build to fail. Additionally, the .env file referenced in docker-compose.yml is missing, causing environment variables to be undefined. Finally, depends_on: db without a healthcheck is insufficient - you need to add a healthcheck to the db service and use condition: service_healthy for proper container orchestration. Please fix these issues and resubmit.


✨ Please rate this review in the Platform's notification center1. 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 button2 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 tag3.
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.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

import time

from django.core.management.base import BaseCommand
from django.db import connection, OperationalError
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Typo in environment variable: PYTHOUNBUFFERED should be PYTHONUNBUFFERED. This environment variable won't work correctly with the typo, affecting real-time output logging in Docker.

Comment on lines +1 to +3
import time

from django.core.management.base import BaseCommand
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The .dockerignore file is missing common patterns that should be excluded from Docker images: __pycache__, *.pyc, *.pyo, .git, and potentially migrations/ folders (except the ones needed).

Comment thread .dockerignore Outdated
Comment on lines +1 to +3
.venv
docker-compose.yml
.idea No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

.dockerignore is missing common entries that should be excluded: .git, *.pyc, __pycache__, *.md, tests/, .github/, and the db volume name my_db shouldn't be there (volumes are not copied).

Comment thread Dockerfile
Comment on lines +1 to +23
FROM python:3.11.6-alpine3.18
LABEL maineiner="yakovenkoanton2007@gmail.com"

ENV PYTHOUNBUFFERED=1

WORKDIR app/

COPY requirements.txt requirements.txt

RUN pip install -r requirements.txt

COPY . .
RUN mkdir -p /files/media

RUN adduser \
--disabled-password \
--no-create-home \
my_user

RUN chown -R my_user /files/media
RUN chmod 755 /files/media

USER my_user No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical Issue: The docker-compose.yml file is missing from the submitted code. This file is explicitly required by the task to 'manage multiple services (containers) at the same time' and is essential for running docker-compose up.

Comment thread Dockerfile
Comment on lines +1 to +3
FROM python:3.11.6-alpine3.18
LABEL maineiner="yakovenkoanton2007@gmail.com"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The .dockerignore is minimal. Consider adding common Python/Django exclusions like __pycache__, *.pyc, .git, .env, *.log, .pytest_cache, and staticfiles to make images thinner.

dependencies = [
('cinema', '0001_initial'),
migrations.swappable_dependency(settings.AUTH_USER_MODEL),
]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The .env file is referenced in docker-compose.yml's env_file directive but is not provided. Without this file, the services will fail to start as environment variables like POSTGRES_DB, POSTGRES_USER, POSTGRES_PASSWORD, POSTGRES_HOST, and POSTGRES_PORT will be undefined.

from django.db import migrations, models


class Migration(migrations.Migration):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The requirements.txt file is referenced in the Dockerfile (line 8) but not provided in the source files. This will cause the Docker image build to fail.

@@ -0,0 +1,41 @@
# Generated by Django 5.2.14 on 2026-05-20 12:02

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Typo in LABEL - 'maineiner' should be 'maintainer'. While this doesn't affect functionality, it's a minor correction.

Comment thread docker-compose.yml

volumes:
my_db:
my_media: No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical Issue: ALLOWED_HOSTS = [] will cause Django to return 400 Bad Request for all incoming requests in Docker. Add at least ALLOWED_HOSTS=['0.0.0.0', 'localhost', '127.0.0.1'] so the service can respond to requests from the mapped port 8000.

Comment thread docker-compose.yml
Comment on lines +14 to +15
sh -c "python manage.py wait_for_db && python manage.py migrate && python manage.py runserver 0.0.0.0:8000"
depends_on:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The depends_on: db without health check is insufficient. PostgreSQL container may be running but not ready to accept connections yet, causing wait_for_db to waste time retrying. Add a healthcheck to the db service and use condition: service_healthy in depends_on for proper Docker Compose service orchestration.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

The provided JSON was an array instead of an object. It has been corrected to match the expected schema with valid 'decision' and 'reason' fields.


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

Footnotes

  1. Rate AI review example

Comment thread docker-compose.yml
Comment on lines +18 to +27

db:
image: postgres:16.0-alpine3.17
restart: always
env_file:
- .env
ports:
- "5433:5432"
volumes:
- my_db:$PGDATA
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The db service uses condition: service_healthy but lacks a healthcheck configuration. Docker Compose requires a healthcheck to determine when the service is healthy. Add:

healthcheck:
  test: ["CMD-SHELL", "pg_isready -U postgres"]
  interval: 5s
  timeout: 5s
  retries: 5

Comment thread docker-compose.yml
Comment on lines +6 to +7
env_file:
- .env
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The .env file is referenced in env_file (lines 6-7, 21-22) but not provided. You need to include a .env file with: POSTGRES_DB, POSTGRES_USER, POSTGRES_PASSWORD, POSTGRES_HOST, POSTGRES_PORT, and PGDATA variables for the containers to work properly.

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.

2 participants