-
Notifications
You must be signed in to change notification settings - Fork 945
Solution #1007
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?
Solution #1007
Changes from 1 commit
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,6 @@ | ||
| .github/ | ||
|
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. Using 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. Base image is |
||
| .venv/ | ||
| .gitignore | ||
| README.md | ||
| 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. Do not ignore the Dockerfile in .dockerignore — the Dockerfile must be available in the build context when building the image. Remove this line so the Docker daemon receives the Dockerfile. |
||
| docker-compose.yaml | ||
|
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. Do not ignore docker-compose.yaml in .dockerignore — docker-compose files are needed for local orchestration and should not be excluded from the image/context. Remove this line. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| FROM python:3.11-alpine | ||
|
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. Using the 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. (Medium) Using Alpine Python image can cause build issues for packages like psycopg2 and typically requires installing build dependencies (gcc, musl-dev, postgresql-dev). Consider using |
||
| LABEL maintainer="ys33ys33ys55@gmail.com" | ||
|
|
||
| ENV PYTHONUNBUFFERED=1 | ||
|
|
||
| WORKDIR /app/ | ||
|
Comment on lines
+1
to
+6
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. Add sensitive and unnecessary files to .dockerignore (for example |
||
|
|
||
|
|
||
| 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. COPYing the entire project before installing dependencies prevents Docker from caching the installed dependencies layer. 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. (Medium) Copying the entire project context before installing Python dependencies prevents proper layer caching and makes rebuilds slower. Change order to: |
||
|
|
||
| RUN pip install -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. Use |
||
|
|
||
| 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. (Informational) Creating |
||
|
|
||
| RUN adduser -D -H my_user | ||
|
|
||
|
Comment on lines
+9
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. (Low) The retry loop can become a tight busy-loop. Add |
||
| RUN chown -R 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. (Informational / caution) You add a non-root user and switch to it (good). Remember package installation must run as root before |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # Generated by Django 5.2.13 on 2026-04-16 13:35 | ||
|
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. Using alpine can reduce image size but often requires additional build dependencies for packages like psycopg2. Consider either installing required build libs (gcc, musl-dev, libpq) or using a slim Debian image for easier compatibility. 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 base image is |
||
|
|
||
| import django.db.models.deletion | ||
| 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. Don't ignore the Dockerfile in .dockerignore — excluding the Dockerfile from the build context can break building the image. Remove this line so the Dockerfile is sent to the daemon during docker build. |
||
|
|
||
|
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. Ignoring docker-compose.yaml may hide important compose configuration from the build context and tools; typically you don't need to ignore the compose file. Remove this entry to avoid surprises. 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. WORKDIR is set to /app/ but you never change ownership of this directory. When switching to a non-root user later the process may lack write permissions. Consider chown'ing /app to |
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
|
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 . . before installing requirements prevents Docker build cache reuse. For faster and thinner builds, first COPY only requirements.txt, run pip install, then COPY the rest of the project. 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 currently |
||
| dependencies = [ | ||
| ('cinema', '0001_initial'), | ||
|
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. 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. This |
||
| migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
|
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 project is missing the required |
||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AlterField( | ||
| model_name='movie', | ||
|
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 chown |
||
| name='actors', | ||
| field=models.ManyToManyField(blank=True, related_name='movies', to='cinema.actor'), | ||
| ), | ||
|
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 add a non-root user and switch to it, but you don't install bash. The task explicitly suggests entering container with 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 switch to a non-root user here. Make sure that after reordering COPY steps any files the app needs to write (e.g., static collection target, media dirs) are owned or writable by |
||
| 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 |
|---|---|---|
|
|
@@ -9,6 +9,8 @@ | |
| For the full list of settings and their values, see | ||
|
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. COPYing the entire project before installing dependencies prevents Docker layer caching for requirements. Better pattern: COPY only 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 |
||
| https://docs.djangoproject.com/en/4.0/ref/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. 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. This |
||
| import os | ||
|
|
||
|
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. Good: the |
||
| from datetime import timedelta | ||
| from pathlib import Path | ||
|
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 bind-mount the project directory with |
||
|
|
||
|
|
@@ -27,7 +29,9 @@ | |
| # SECURITY WARNING: don't run with debug turned on in production! | ||
| DEBUG = True | ||
|
|
||
| ALLOWED_HOSTS = [] | ||
| ALLOWED_HOSTS = [ | ||
| "0.0.0.0", | ||
| ] | ||
|
Comment on lines
+32
to
+34
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. ALLOWED_HOSTS includes |
||
|
|
||
| INTERNAL_IPS = [ | ||
| "127.0.0.1", | ||
|
|
@@ -86,8 +90,12 @@ | |
|
|
||
| DATABASES = { | ||
| "default": { | ||
| "ENGINE": "django.db.backends.sqlite3", | ||
| "NAME": BASE_DIR / "db.sqlite3", | ||
| "ENGINE": "django.db.backends.postgresql", | ||
| "NAME": os.environ["POSTGRES_DB"], | ||
| "USER": os.environ["POSTGRES_USER"], | ||
| "PASSWORD": os.environ["POSTGRES_PASSWORD"], | ||
| "HOST": os.environ["POSTGRES_HOST"], | ||
| "PORT": os.environ["POSTGRES_PORT"], | ||
|
Comment on lines
+93
to
+98
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 Django DATABASES settings use |
||
| } | ||
|
Comment on lines
+93
to
99
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. Your settings read DB credentials with |
||
| } | ||
|
|
||
|
|
@@ -134,7 +142,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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| services: | ||
|
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. Using |
||
| 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. The 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 compose service is named |
||
| build: | ||
| context: . | ||
| env_file: | ||
| - .env | ||
| ports: | ||
| - "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. Host port is mapped as |
||
| 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. COPYing the entire project before installing dependencies prevents Docker layer caching and makes rebuilds slower. Reorder to: |
||
| sh -c "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 startup command runs migrations and then |
||
| volumes: | ||
|
Comment on lines
+9
to
+14
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 |
||
| - ./:/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.
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 a host bind-mount |
||
| - my_media:/files/media | ||
|
Comment on lines
+14
to
+15
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. Mounting the project host directory into the container with |
||
| depends_on: | ||
| - db | ||
|
Comment on lines
+16
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.
|
||
|
|
||
| db: | ||
|
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 switch to a non-root |
||
| image: postgres:16.0-alpine3.17 | ||
| restart: always | ||
| env_file: | ||
| - .env | ||
|
Comment on lines
+22
to
+23
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 reference an |
||
| ports: | ||
| - "5432:5432" | ||
| 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. The db volume entry 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 volume is declared as |
||
|
|
||
| volumes: | ||
| my_db: | ||
| my_media: | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| django | ||
| psycopg | ||
| psycopg-binary | ||
| flake8 | ||
| flake8-quotes | ||
| flake8-variables-names | ||
|
|
||
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.
Using the Alpine base is fine, but many Python packages that interface with PostgreSQL (e.g. psycopg2) need system build dependencies on Alpine. Add apk installation of required packages (e.g. build-base, postgresql-dev, musl-dev) or switch to a Debian-slim base or use
psycopg2-binaryto avoid compilation failures atpip installtime.