-
Notifications
You must be signed in to change notification settings - Fork 947
Solution #1009
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 #1009
Changes from 2 commits
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,8 @@ | ||
| .git | ||
| .venv | ||
| __pycache__ | ||
| *.pyc | ||
| db.sqlite3 | ||
| media/ | ||
| staticfiles/ | ||
| .env | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,28 +1,11 @@ | ||
| name: Test | ||
| name: Docker Test | ||
|
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. Consider pinning the base image more precisely (for example |
||
|
|
||
| on: | ||
| pull_request: | ||
| branches: | ||
| - "master" | ||
| on: [push] | ||
|
|
||
| jobs: | ||
| test: | ||
| build: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 10 | ||
|
|
||
| steps: | ||
| - name: Checkout repo | ||
| uses: actions/checkout@v2 | ||
|
|
||
| - name: Set Up Python 3.10 | ||
| uses: actions/setup-python@v2 | ||
| with: | ||
| python-version: "3.10" | ||
|
|
||
| - name: Install requirements | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements.txt | ||
|
|
||
| - name: Run flake8 | ||
| run: flake8 | ||
| - uses: actions/checkout@v3 | ||
| - name: Build and Run | ||
|
Comment on lines
7
to
+10
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. When installing build-time packages use |
||
| run: docker compose build && docker compose up -d | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| FROM python:3.11-slim | ||
|
|
||
| ENV PYTHONUNBUFFERED=1 | ||
| WORKDIR /app | ||
|
|
||
|
|
||
| RUN apt-get update && apt-get install -y \ | ||
| gcc \ | ||
| libpq-dev \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
Comment on lines
+7
to
+10
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 --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. |
||
|
|
||
| COPY requirements.txt . | ||
| RUN pip install --no-cache-dir -r requirements.txt | ||
|
|
||
| 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 . . 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. |
||
|
|
||
| RUN adduser --disabled-password --no-create-home django-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. You create the non-root user after copying files. If you plan to use COPY --chown (recommended), create the user earlier so the chown can target the correct user during the COPY step. |
||
| USER django-user | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ | |
| "debug_toolbar", | ||
| "cinema", | ||
| "user", | ||
| "core", | ||
|
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 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 |
||
| ] | ||
|
|
||
| MIDDLEWARE = [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import time | ||
| from django.core.management.base import BaseCommand | ||
| from django.db import connections | ||
| from django.db.utils import OperationalError | ||
|
|
||
| class Command(BaseCommand): | ||
| """Django command to pause execution until database is available""" | ||
|
|
||
|
Comment on lines
+5
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 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. |
||
| def handle(self, *args, **options): | ||
| self.stdout.write('Waiting for database...') | ||
|
Comment on lines
+7
to
+10
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 apt-get install line should use |
||
| db_conn = None | ||
| while not db_conn: | ||
| try: | ||
| db_conn = connections['default'] | ||
|
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 command does not actually attempt a DB connection. Accessing |
||
| except OperationalError: | ||
| self.stdout.write('Database unavailable, waiting 1 second...') | ||
| time.sleep(1) | ||
|
Comment on lines
+12
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. 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. 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 |
||
|
|
||
|
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. For production deployments avoid bind-mounting the project source into the container (the |
||
| self.stdout.write(self.style.SUCCESS('Database available!')) | ||
|
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 |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| services: | ||
| db: | ||
| image: postgres:15-alpine | ||
| restart: always | ||
| environment: | ||
| POSTGRES_DB: cinema_db | ||
| POSTGRES_USER: cinema_user | ||
| POSTGRES_PASSWORD: secretpassword | ||
|
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 expose DB credentials directly in the compose file ( |
||
| volumes: | ||
| - postgres_data:/var/lib/postgresql/data | ||
|
|
||
| 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. Consider adding a restart policy (for example |
||
| 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. For a fully self-contained deployment (one of the task requirements) avoid relying on a host bind-mount for the application code. Keep |
||
| command: > | ||
| sh -c "python manage.py wait_for_db && | ||
| python manage.py migrate && | ||
| python manage.py runserver 0.0.0.0:8000" | ||
|
Comment on lines
+16
to
+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. Good: the |
||
| 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. The app service bind-mounts the project directory from the host ( 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 host bind mount |
||
| ports: | ||
| - "8000:8000" | ||
|
Comment on lines
+19
to
+24
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 named volume for media/static files. Add volumes for |
||
| environment: | ||
| - DB_HOST=db | ||
| - DB_NAME=cinema_db | ||
| - DB_USER=cinema_user | ||
| - DB_PASS=secretpassword | ||
|
Comment on lines
+26
to
+29
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 receives DB environment variables (
Comment on lines
+25
to
+29
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. Database credentials are hardcoded in the |
||
| depends_on: | ||
| - db | ||
|
Comment on lines
+31
to
+32
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.
|
||
|
|
||
| volumes: | ||
| postgres_data: | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,3 +8,4 @@ djangorestframework | |
| djangorestframework-simplejwt | ||
| drf-spectacular | ||
| Pillow | ||
| psycopg2-binary | ||
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.
The image can be made thinner: add --no-install-recommends to apt-get install to avoid extra packages, and consider removing build-time dependencies after pip install or use a multi-stage build so tools like gcc/libpq-dev aren't left in the final image.