-
Notifications
You must be signed in to change notification settings - Fork 946
Created all neccasery files #1012
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: master
Are you sure you want to change the base?
Changes from all commits
b890b3e
246974a
9cdb0e4
34b7044
310d1b9
bcd1a9b
c8822a9
854ba0e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Dockerfile | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docker-compose.yaml maps db volume There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. settings.py (DATABASES) still uses sqlite3. To meet the requirement to switch to PostgreSQL via official image, settings must be updated to read DB_HOST, DB_NAME, DB_USER, DB_PASSWORD, DB_PORT from environment and use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docker-compose maps app ports as "8001:8000"; combined with the requirement that API be at 127.0.0.1:8000, this is inconsistent. Change to "8000:8000" to meet the check instructions. |
||
| docker-compose.yml | ||
|
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CRITICAL - Checklist item #12: Service must be named |
||
| venv/ | ||
|
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. .dockerignore currently lists only Dockerfile, docker-compose.yml, venv/. Consider adding pycache, *.pyc, .pytest_cache, .env, .git, media, and other unnecessary files to keep images thin as required.
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The .dockerignore is minimal and misses recommended entries. Expand to ignore Python/Django artifacts and media per checklist (e.g. pycache/, *.pyc, .git/, .env, .pytest_cache/, media/). This was suggested in previous review.
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMPORTANT - Checklist item #6: The |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| FROM python:3.11.6-alpine3.18 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Dockerfile doesn't install system dependencies required by Pillow (build tools, libjpeg, zlib). Without these, image upload/tests will fail inside the Alpine image. Add apk packages such as build-base, libjpeg-turbo-dev, zlib-dev, jpeg-dev, musl-dev (and any others needed) before pip install. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docker-compose service is named There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The repo is missing the Django management command There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. .dockerignore is very small. Expand to exclude |
||
| LABEL maintainer="shafran.andriy@gmail.com" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| ENV PYTHONUNBUFFERED=1 | ||
| WORKDIR /app | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ports are mapped to |
||
| COPY requirements.txt requirements.txt | ||
| RUN pip install --no-cache-dir -r requirements.txt | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Dockerfile does not install system dependencies required by pillow (used in tests for image creation). On Alpine you typically need build-base, jpeg-dev, zlib-dev, freetype-dev, lcms2-dev, openjpeg-dev, tiff-dev, libjpeg-turbo-dev, and musl-dev before pip install. Without them pip install pillow will likely fail. Add an apk add step before pip install.
Comment on lines
+1
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Dockerfile is missing Alpine system packages required for Pillow (build-base, libjpeg-turbo-dev, zlib-dev, jpeg-dev, musl-dev, musl-utils, libffi-dev, openssl-dev, cargo, rust). Without these, image uploads and related tests will fail during build or runtime. |
||
| COPY . . | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. COPY . . copies the entire repository including files that should be ignored by the image. Ensure .dockerignore is correct (see .dockerignore file includes only 3 lines, missing common entries like pycache, *.pyc, .git, .venv, env files, migrations if desired). Otherwise image will be larger and may leak sensitive files. |
||
| RUN mkdir -p /files/media | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You create directory /files/media and chown/chmod it, which is good, but MEDIA_ROOT in settings.py points to BASE_DIR / "media". The Dockerfile uses /files/media and docker-compose maps volume my_media to /files/media. This mismatch means Django will save uploaded files to a different path inside container than the volume; update MEDIA_ROOT to /files/media or change Dockerfile/volumes to match settings. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MEDIA_ROOT must match the volume mount used in docker-compose (/files/media). I see settings.py already sets MEDIA_ROOT = "/files/media" — good. Ensure docker-compose mounts my_media:/files/media (compose file currently mounts /files/media) and Dockerfile creates/chowns that path — Dockerfile does create /files/media but ensure permissions set while root before switching user. Move package install and media dir creation earlier (before USER my_user) and ensure chown/chmod executed as root (they are currently root). |
||
|
|
||
| RUN adduser \ | ||
| --disabled-password \ | ||
| --no-create-home \ | ||
|
Comment on lines
+11
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Dockerfile uses Alpine Python image which is fine, but there's no explicit creation or copying of a non-root user home; using --no-create-home prevents creating home and may cause permission issues for package caches or migrations. Consider creating a home or using a dedicated app user with a home directory. Also ensure packages requiring build tools on Alpine (Pillow) are installed in requirements or add build dependencies (musl headers, gcc, libjpeg-dev) before pip install to avoid install failures in Docker. |
||
| my_user | ||
|
|
||
| RUN chown -R my_user:my_user /files/media | ||
| RUN chmod -R 755 /files/media | ||
|
|
||
| USER my_user | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Final USER my_user will run commands; however many operations during image build (collectstatic/migrations) may require root or correct permissions. Ensure that entrypoint commands run inside container with appropriate permissions. Also consider switching to non-root after any operations that require root are complete. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import time | ||
| from django.db import connections | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Service name must be |
||
| from django.db.utils import OperationalError | ||
|
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Expand |
||
| from django.core.management import BaseCommand | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Port mapping must be |
||
|
|
||
| class Command(BaseCommand): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Dockerfile must install Alpine system packages for Pillow before pip install. Add this RUN command before line 7: |
||
| """Django command to pause execution until db is available""" | ||
|
|
||
| def handle(self, *args, **options): | ||
| self.stdout.write("Waiting for database...") | ||
| db_conn = None | ||
| while not db_conn: | ||
| try: | ||
| db_conn = connections["default"] | ||
| except OperationalError: | ||
| self.stdout.write("Database unavailable, waiting 1 second...") | ||
| time.sleep(1) | ||
|
|
||
| self.stdout.write(self.style.SUCCESS("Database available!")) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| # Generated by Django 6.0.4 on 2026-04-22 14:03 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't find implementation of |
||
|
|
||
| import django.db.models.deletion | ||
|
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. .dockerignore is minimal and excludes only Dockerfile, docker-compose.yml and venv/. Consider adding pycache/, *.pyc, .git/, .env, .pytest_cache/, media/ and other large directories to make images thinner as required by checklist.
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docker-compose.yaml uses service name
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dockerfile creates /files/media and sets ownership to |
||
| from django.conf import settings | ||
| from django.db import migrations, models | ||
|
|
||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Dockerfile does not install system packages required to build Pillow on Alpine (build-base, libjpeg-turbo-dev, zlib-dev, etc.). This will likely cause image upload/tests to fail. Add an APK install step before pip install, e.g.
Comment on lines
+1
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Dockerfile uses
Comment on lines
+1
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Alpine system packages required for Pillow (used for image handling). Add |
||
| class Migration(migrations.Migration): | ||
|
Comment on lines
+1
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docker-compose command chains
Comment on lines
+1
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dockerfile uses python:3.11.6-alpine which is good for thin image, but it copies entire project (
Comment on lines
+1
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests expect the API to be available at 127.0.0.1:8000/api/ and use image uploads; ensure runserver is bound to 0.0.0.0:8000 (done) and host port mapping is 8000:8000. Also ensure MEDIA_ROOT and media volume mapping align so tests that check os.path.exists(movie.image.path) succeed. |
||
|
|
||
|
Comment on lines
+1
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The docker-compose YAML uses service names |
||
| dependencies = [ | ||
| ("cinema", "0001_initial"), | ||
| migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
|
Comment on lines
+8
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The compose |
||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AlterField( | ||
| model_name="movie", | ||
|
Comment on lines
+10
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docker-compose defines |
||
| name="actors", | ||
| field=models.ManyToManyField( | ||
|
Comment on lines
+9
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dockerfile creates |
||
| blank=True, related_name="movies", to="cinema.actor" | ||
|
Comment on lines
+1
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In settings.py DATABASES is still configured for SQLite. The task requires switching to PostgreSQL via environment variables. You must modify DATABASES to read host, port, name, user, password from environment (e.g. via os.environ) and use 'django.db.backends.postgresql'. This is a critical missing requirement. |
||
| ), | ||
| ), | ||
| migrations.AlterField( | ||
| 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, | ||
| ), | ||
| ), | ||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,14 +72,10 @@ def __str__(self): | |
| class MovieSession(models.Model): | ||
| show_time = models.DateTimeField() | ||
| movie = models.ForeignKey( | ||
| Movie, | ||
| on_delete=models.CASCADE, | ||
| related_name="movie_sessions" | ||
| Movie, on_delete=models.CASCADE, related_name="movie_sessions" | ||
| ) | ||
| cinema_hall = models.ForeignKey( | ||
| CinemaHall, | ||
| on_delete=models.CASCADE, | ||
| related_name="movie_sessions" | ||
| CinemaHall, on_delete=models.CASCADE, related_name="movie_sessions" | ||
| ) | ||
|
|
||
| class Meta: | ||
|
|
@@ -106,15 +102,11 @@ class Meta: | |
|
|
||
| class Ticket(models.Model): | ||
| movie_session = models.ForeignKey( | ||
| MovieSession, | ||
| on_delete=models.CASCADE, | ||
| related_name="tickets" | ||
| ) | ||
| order = models.ForeignKey( | ||
| Order, | ||
| on_delete=models.CASCADE, | ||
| related_name="tickets" | ||
| MovieSession, on_delete=models.CASCADE, related_name="tickets" | ||
| ) | ||
| order = models.ForeignKey(Order, | ||
| on_delete=models.CASCADE, | ||
| related_name="tickets") | ||
| row = models.IntegerField() | ||
| seat = models.IntegerField() | ||
|
|
||
|
|
@@ -157,9 +149,8 @@ def save( | |
| ) | ||
|
|
||
| def __str__(self): | ||
| return ( | ||
| f"{str(self.movie_session)} (row: {self.row}, seat: {self.seat})" | ||
| ) | ||
| return f"{str(self.movie_session)}, " | ||
| f"(row: {self.row}, seat: {self.seat})" | ||
|
Comment on lines
151
to
+153
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
|
||
| class Meta: | ||
| unique_together = ("movie_session", "row", "seat") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| https://docs.djangoproject.com/en/4.0/ref/settings/ | ||
| """ | ||
| from datetime import timedelta | ||
| import os | ||
| from pathlib import Path | ||
|
|
||
| # Build paths inside the project like this: BASE_DIR / 'subdir'. | ||
|
|
@@ -84,10 +85,22 @@ | |
| # Database | ||
| # https://docs.djangoproject.com/en/4.0/ref/settings/#databases | ||
|
|
||
| # DATABASES = { | ||
| # "default": { | ||
| # "ENGINE": "django.db.backends.sqlite3", | ||
| # "NAME": BASE_DIR / "db.sqlite3", | ||
| # } | ||
| # } | ||
|
|
||
|
|
||
| DATABASES = { | ||
| "default": { | ||
| "ENGINE": "django.db.backends.sqlite3", | ||
| "NAME": BASE_DIR / "db.sqlite3", | ||
| "ENGINE": "django.db.backends.postgresql", | ||
| "NAME": os.environ.get("POSTGRES_DB"), | ||
| "USER": os.environ.get("POSTGRES_USER"), | ||
| "PASSWORD": os.environ.get("POSTGRES_PASSWORD"), | ||
| "HOST": os.environ.get("POSTGRES_HOST"), | ||
| "PORT": os.environ.get("POSTGRES_PORT"), | ||
|
Comment on lines
96
to
+103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Settings: DATABASES is configured to use postgresql and reads connection parameters from environment variables — this satisfies the requirement to switch from SQLite to Postgres (good). Ensure your .env defines POSTGRES_DB/USER/PASSWORD/HOST/PORT and that docker-compose passes the env_file to both services. |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -134,7 +147,7 @@ | |
| STATIC_URL = "static/" | ||
|
|
||
| MEDIA_URL = "/media/" | ||
| MEDIA_ROOT = BASE_DIR / "media" | ||
| MEDIA_ROOT = "/files/media" | ||
|
|
||
| # Default primary key field type | ||
| # https://docs.djangoproject.com/en/4.0/ref/settings/#default-auto-field | ||
|
Comment on lines
151
to
153
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In cinema/models.py Ticket.str is split across multiple return expressions (two separate f-strings on adjacent lines with commas). That will not produce the intended single string. Combine into one formatted string, e.g. return f"{self.movie_session}, (row: {self.row}, seat: {self.seat})". |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| services: | ||
| cinema: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Service name is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Service name is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| build: | ||
| context: . | ||
| env_file: | ||
| - .env | ||
| ports: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CRITICAL - Checklist item #13: Port mapping must be |
||
| - "8001:8000" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ports mapping is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Host port mapping is |
||
| command: > | ||
| sh -c "python manage.py wait_for_db && | ||
| python manage.py makemigrations && | ||
| python manage.py migrate && | ||
| python manage.py runserver 0.0.0.0:8000" | ||
|
Comment on lines
+9
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The container command runs |
||
| volumes: | ||
| - ./:/app | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Volume mapping for the app mounts the project directory |
||
| - my_media:/files/media | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Media volume is mounted at
Comment on lines
+15
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Volume mapping |
||
| depends_on: | ||
| - db | ||
|
|
||
| db: | ||
| image: postgres:16-alpine3.17 | ||
| restart: always | ||
| env_file: | ||
| - .env | ||
| ports: | ||
| - "5432:5432" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Service port mapping exposes Postgres on host (
Comment on lines
+25
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Postgres service exposes
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. db service uses env variables from .env |
||
| volumes: | ||
| - my_db:$PGDATA | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DB service volume uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. db volume uses
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PGDATA is set in .env, and it is defined |
||
|
|
||
| volumes: | ||
| my_db: | ||
| my_media: | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,3 +8,5 @@ djangorestframework | |
| djangorestframework-simplejwt | ||
| drf-spectacular | ||
| Pillow | ||
| psycopg==3.1.12 | ||
| psycopg-binary==3.1.12 | ||
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.
docker-compose.yaml uses service names
cinemaanddbbut task requires services namedappanddb, and thatappalways waits fordbusing await_for_dbmanagement command. The compose file runspython manage.py wait_for_dbbut service iscinema, notapp. Rename service toappto match checklist.