Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.venv
Dockerfile
.idea
docker-compose.yaml
23 changes: 23 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
FROM python:3.11.6-alpine3.18

LABEL maintainer="rarturus"

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 very minimal. Add common ignores such as .env, __pycache__/, *.pyc, .pytest_cache/ and optionally media if you don't want to copy uploaded files into the image. Also you currently ignore Dockerfile (line 2) which is unnecessary; while not always fatal it is unusual and may hide files needed in the build context. Finally, you list docker-compose.yaml but the task expects docker-compose.yml — consider including both names to be safe.

ENV PYTHONUNBUFFERED 1

WORKDIR /app

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 management command has a typo: use the attribute name help, not help1. While the command will still run, the help text won't be available in manage.py help and this deviates from the typical implementation required by the task. Consider also adding a small timeout/retry limit or logging if you need it.

COPY requirements.txt .
RUN pip install --no-cache-dir -r requirements.txt
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Installing Python packages on Alpine often requires system build dependencies (gcc, musl-dev, postgresql-dev, libffi-dev, openssl-dev, build-base, etc.) if your requirements include packages like psycopg2. If you rely on such packages, add the necessary apk add --no-cache ... before pip install, or use prebuilt wheels (for example psycopg2-binary) to avoid build failures on Alpine.

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: you must install OS build dependencies before running pip install so native DB drivers (like psycopg2) can be built. Add a RUN step such as: apt-get update && apt-get install -y build-essential libpq-dev && rm -rf /var/lib/apt/lists/* before the pip install line to avoid build failures on python:3.11-slim.


COPY . .
RUN mkdir -p /files/media

RUN adduser \
--disabled-password \
--no-create-home \
my_user
Comment on lines +17 to +20
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 adduser options used here (--disabled-password, --no-create-home) are GNU-style and may not be supported by Alpine's adduser. On Alpine you typically use adduser -D -H my_user or create user/group with addgroup/adduser flags appropriate for the base image. Please adapt this to the Alpine toolset so the user is created reliably.


RUN chown -R my_user /files/media
RUN chmod -R 755 /files/media
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Permissions: you create /files/media and later run USER my_user, but you never chown the media directory to the non-root user. Add chown -R my_user:my_user /files/media (or include it with the existing chown) before switching to USER my_user so the app can write uploaded files at runtime.


USER my_user
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Setting USER my_user makes the container run as non-root, which is good for security but will often cause permission problems when mounting the host project into /app (the mounted files may be owned by root or a different UID). Either ensure /app is owned by my_user (chown) at runtime, create the user with the same UID as the host developer, or run as root in development to avoid permission errors with migrations, collectstatic, or file writes.

Empty file added cinema/management/__init__.py
Empty file.
Empty file.
19 changes: 19 additions & 0 deletions cinema/management/commands/wait_for_db.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import time
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor/informational: the migration header says it was generated by Django 6.0.4 while project settings indicate Django 4.x. This version mismatch is unusual and may indicate the migration was generated with a different Django version. Verify migrations and Django version compatibility.

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.

Critical: You are ignoring the Dockerfile in .dockerignore. Docker will not send the Dockerfile into the build context if it's listed here, which prevents building the app image. Remove this line so the Dockerfile is included in the build context.

from django.db import connections
from django.db.utils import OperationalError


class Command(BaseCommand):
help1 = "Wait for database to be available"
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 management command sets help1 instead of the expected help attribute. Django expects help on BaseCommand to provide the description. Rename help1 to help so the command conforms to Django conventions.


def handle(self, *args, **options):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If your requirements include packages like psycopg2 (common for PostgreSQL), this pip install step will fail because the image currently does not install system build dependencies (libpq headers, gcc, etc.). On Debian-slim you should run apt-get update && apt-get install -y build-essential libpq-dev (and clean apt caches) before pip install (or switch to psycopg2-binary).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Before running pip install -r requirements.txt you must install OS build dependencies required for DB drivers on python:3.11-slim. Add a RUN step before line 10 such as:

RUN apt-get update && apt-get install -y build-essential libpq-dev && rm -rf /var/lib/apt/lists/*

This ensures packages like psycopg2 can compile and keeps image layers clean as required by the checklist.

self.stdout.write("Waiting for database...")
while True:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

COPY . . is fine for image build, but if you later mount the project directory as a volume in docker-compose (bind mount), it will overwrite the image contents at runtime and make the image less independent/thin. Reconsider the bind mount in compose if your goal is to make the service independent of the local machine, or document that mounting is for development only.

try:
connections["default"].cursor()
break
except OperationalError:
self.stdout.write("Database unavailable, waiting 1 second...")
time.sleep(1)
Comment on lines +13 to +16
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 adduser command used here appears to use Debian-style flags (e.g. --disabled-password, --no-create-home). On Alpine-based images the adduser implementation and flags are different and this RUN may fail during image build. Either use an Alpine-compatible user creation (e.g. adduser -D my_user) or use a different base image. Also consider creating a home directory if needed.

Comment on lines +13 to +16
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You create a user with adduser ... my_user. Make sure the flags used are supported by this base image (python:3.11-slim is Debian-based so these flags are likely fine). Still: it is better to set ownership of the application dir (/app) and switch to the non-root user with USER my_user so the container doesn't run as root. Also consider creating the user non-interactively and ensuring home/uid if needed.

self.stdout.write(self.style.SUCCESS("Database available!"))
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Generated by Django 6.0.4 on 2026-04-16 22:41
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If requirements.txt includes packages that need compilation (for example psycopg2), you must install system build dependencies before running pip install. Since the base image is Debian-based (python:3.11-slim), add apt-get steps to install packages like build-essential, libpq-dev (and clean up after). Without these, pip install may fail during image build.


import django.db.models.deletion
from django.conf import settings
from django.db import migrations, models

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You currently ignore docker-compose.yml in .dockerignore. It's not harmful but usually unnecessary to ignore docker-compose.yml — consider removing that line so the compose file is present in the build context (or keep it intentionally if you have a reason). This is low severity but worth checking.


class Migration(migrations.Migration):

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

operations = [
migrations.AlterField(
model_name='movie',
name='actors',
field=models.ManyToManyField(blank=True, related_name='movies', to='cinema.actor'),
),
migrations.AlterField(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You create the my_user account but never switch to it or ensure the application files are owned by that user. For best practice add chown -R my_user /app (or make /app owned earlier) and USER my_user so the container does not run as root. Also ensure the adduser flags you use are compatible with the base image (Debian vs Alpine).

model_name='movie',
name='genres',
field=models.ManyToManyField(blank=True, related_name='movies', to='cinema.genre'),
),
migrations.AlterField(
model_name='moviesession',
name='cinema_hall',
field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='movie_sessions', to='cinema.cinemahall'),
),
migrations.AlterField(
model_name='moviesession',
name='movie',
field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='movie_sessions', to='cinema.movie'),
),
migrations.AlterField(
model_name='order',
name='user',
field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='orders', to=settings.AUTH_USER_MODEL),
),
]
15 changes: 10 additions & 5 deletions cinema_service/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
For the full list of settings and their values, see
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 app service runs Django on 0.0.0.0:8000 but there is no ports mapping for the service. Add ports: - "8000:8000" so the API is accessible on 127.0.0.1:8000 as required by the task.

https://docs.djangoproject.com/en/4.0/ref/settings/
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: the Dockerfile runs pip install -r requirements.txt without first installing OS build dependencies needed by DB drivers (e.g. psycopg2). Per the checklist you must add a step before pip install such as: apt-get update && apt-get install -y build-essential libpq-dev && rm -rf /var/lib/apt/lists/* so the image builds successfully on python:3.11-slim.

"""
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You bind-mount the project directory into the container (./:/app). This makes the container dependent on the host filesystem and contradicts the requirement that the service be fully independent of the local machine. For a production-like Docker setup remove the bind mount (use the image contents) or make it optional (development-only).

import os
from datetime import timedelta
from pathlib import Path

Expand Down Expand Up @@ -86,12 +87,15 @@

DATABASES = {
"default": {
"ENGINE": "django.db.backends.sqlite3",
"NAME": BASE_DIR / "db.sqlite3",
"ENGINE": os.environ.get("SQL_ENGINE", "django.db.backends.sqlite3"),
"NAME": os.environ.get("SQL_DATABASE", BASE_DIR / "db.sqlite3"),
Comment on lines +90 to +91
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 database ENGINE and NAME default to SQLite if environment variables are not provided. Ensure your .env (used by docker-compose) sets SQL_ENGINE=django.db.backends.postgresql and SQL_DATABASE to the PostgreSQL DB name so Django uses Postgres in Docker. Without these env vars Django will try to use SQLite instead of the required PostgreSQL.

"USER": os.environ.get("SQL_USER", "user"),
"PASSWORD": os.environ.get("SQL_PASSWORD", "password"),
"HOST": os.environ.get("SQL_HOST", "localhost"),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Default HOST is localhost. Inside the app container Django should connect to the Postgres service by host name (the service name in docker-compose, e.g. db). Make sure your .env sets SQL_HOST=db, otherwise Django will attempt to connect to a DB on the same container and fail.

"PORT": os.environ.get("SQL_PORT", "5432"),
}
}


# Password validation
# https://docs.djangoproject.com/en/4.0/ref/settings/#auth-password-validators

Expand Down Expand Up @@ -134,7 +138,8 @@
STATIC_URL = "static/"

MEDIA_URL = "/media/"
MEDIA_ROOT = BASE_DIR / "media"
MEDIA_ROOT = "files/media"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MEDIA_ROOT is set to a relative path files/media. Your Dockerfile and docker-compose mount use /files/media (an absolute path). Change MEDIA_ROOT to the absolute path (e.g. /files/media) so Django reads/writes media files inside the container volume as intended.



# Default primary key field type
# https://docs.djangoproject.com/en/4.0/ref/settings/#default-auto-field
Expand Down Expand Up @@ -167,7 +172,7 @@
}

SIMPLE_JWT = {
"ACCESS_TOKEN_LIFETIME": timedelta(minutes=5),
"ACCESS_TOKEN_LIFETIME": timedelta(days=1),
"REFRESH_TOKEN_LIFETIME": timedelta(days=1),
"ROTATE_REFRESH_TOKENS": False,
}
33 changes: 33 additions & 0 deletions docker-compose.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
services:
cinema:
build:
context: .
env_file:
- .env
command: >
sh -c "python manage.py wait_for_db && python manage.py migrate &&
python manage.py runserver 0.0.0.0:8000"
volumes:
- ./:/app
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 cinema service does not expose the application port to the host. Add a ports mapping such as ports: - "8000:8000" so the app is reachable at 127.0.0.1:8000 as required by the task.

- media_data:/files/media
depends_on:
- db

db:
image: postgres:16.0-alpine3.17
environment:
POSTGRES_USER: "rarturus"
POSTGRES_PASSWORD: "password"
POSTGRES_DB: "db"
restart: always
ports:
- "5432:5432"
env_file:
- .env
volumes:
- db_data:$PG_DATA
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 volume mapping uses db_data:$PG_DATA, which references an undefined environment variable and is incorrect. Map the named volume to PostgreSQL's data directory inside the container, e.g. db_data:/var/lib/postgresql/data.



volumes:
db_data:
media_data:
2 changes: 2 additions & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@ djangorestframework
djangorestframework-simplejwt
drf-spectacular
Pillow
psycopg==3.3.3
psycopg-binary==3.3.3
Loading