-
Notifications
You must be signed in to change notification settings - Fork 544
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
Modernize docker-compose.yml
#464
base: main
Are you sure you want to change the base?
Changes from all 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 | ||
---|---|---|---|---|
@@ -1,4 +1,4 @@ | ||||
FROM python:3.9-slim | ||||
FROM python:3.12-slim | ||||
|
||||
# Install packages needed to run your application (not build deps): | ||||
# We need to recreate the /usr/share/man/man{1..8} directories first because | ||||
|
@@ -33,7 +33,7 @@ RUN set -ex \ | |||
zlib1g-dev \ | ||||
" \ | ||||
&& apt-get update && apt-get install -y --no-install-recommends $BUILD_DEPS \ | ||||
&& python3.9 -m venv ${VIRTUAL_ENV} \ | ||||
&& python3 -m venv ${VIRTUAL_ENV} \ | ||||
&& pip install -U pip \ | ||||
&& pip install --no-cache-dir -r /requirements/production.txt \ | ||||
&& apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false $BUILD_DEPS \ | ||||
|
@@ -45,9 +45,6 @@ ADD . /code/ | |||
ENV PORT 8000 | ||||
EXPOSE 8000 | ||||
|
||||
# Add custom environment variables needed by Django or your settings file here: | ||||
ENV DJANGO_SETTINGS_MODULE=bakerydemo.settings.production DJANGO_DEBUG=off | ||||
|
||||
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. Question: Why are we removing the default 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. I moved 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 think we should still keep the production default in 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. If there are undocumented use cases that require production-like settings and packages, could that be addressed by someone else who is aware of these scenarios in a separate PR? I can only work from the documented use cases in the readme, which has this to say about the docker compose setup: Line 112 in cd09104
The readme.md makes it sound like all paths for starting the demo are equivalent and only a matter of preference, but per my previous comment:
Either the readme needs some reworking or the configurations should be consistent with each other and with the readme. The simplest starting point for demo purposes is to install For anyone who wishes to submit a PR to |
||||
# Call collectstatic with dummy environment variables: | ||||
RUN DATABASE_URL=postgres://none REDIS_URL=none python manage.py collectstatic --noinput | ||||
|
||||
|
@@ -57,8 +54,5 @@ RUN mkdir -p /code/bakerydemo/media/images && chown -R 1000:2000 /code/bakerydem | |||
# mark the destination for images as a volume | ||||
VOLUME ["/code/bakerydemo/media/images/"] | ||||
|
||||
# start uWSGI, using a wrapper script to allow us to easily add more commands to container startup: | ||||
ENTRYPOINT ["/code/docker-entrypoint.sh"] | ||||
|
||||
# Start uWSGI | ||||
CMD ["uwsgi", "/code/etc/uwsgi.ini"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,38 +1,40 @@ | ||
version: '2' | ||
volumes: | ||
postgres-data: | ||
|
||
services: | ||
db: | ||
environment: | ||
POSTGRES_DB: app_db | ||
POSTGRES_USER: app_user | ||
POSTGRES_PASSWORD: changeme | ||
restart: unless-stopped | ||
image: postgres:14.1 | ||
expose: | ||
- '5432' | ||
|
||
redis: | ||
restart: unless-stopped | ||
image: redis:6.2 | ||
expose: | ||
- '6379' | ||
|
||
app: | ||
environment: | ||
DJANGO_SECRET_KEY: changeme | ||
DATABASE_URL: postgres://app_user:changeme@db/app_db | ||
REDIS_URL: redis://redis | ||
DJANGO_SETTINGS_MODULE: bakerydemo.settings.dev | ||
build: | ||
context: . | ||
dockerfile: ./Dockerfile | ||
volumes: | ||
- ./bakerydemo:/code/bakerydemo | ||
links: | ||
- db:db | ||
- redis:redis | ||
ports: | ||
- '8000:8000' | ||
depends_on: | ||
- db | ||
- redis | ||
db: | ||
condition: service_healthy | ||
redis: | ||
condition: service_healthy | ||
|
||
db: | ||
environment: | ||
- "POSTGRES_DB=${DATABASE_NAME}" | ||
- "POSTGRES_USER=${DATABASE_USER}" | ||
- "POSTGRES_PASSWORD=${DATABASE_PASSWORD}" | ||
restart: unless-stopped | ||
image: postgres:16 | ||
volumes: | ||
- postgres-data:/var/lib/postgresql/data | ||
healthcheck: | ||
test: "pg_isready --quiet --dbname=${DATABASE_URL}" | ||
interval: 10s | ||
timeout: 5s | ||
retries: 5 | ||
|
||
redis: | ||
restart: unless-stopped | ||
image: redis:7 | ||
expose: | ||
- '6379' | ||
healthcheck: | ||
test: ["CMD-SHELL", "redis-cli ping || exit 1"] |
This file was deleted.
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.
.env
is not just used for the Docker setup - if we make this change, it's going to break the virtualenv setup instructions, since we've not told people to set up a Postgres database there (and, of course, we'd rather not make that a requirement). Maybe there should be a separate.env.docker.example
?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.
Ah good call, my brain is full of docker at the moment and lost site of the other install options. FYI, I'm wrapping up much more extensive
docker compose
"modernization" work over in wagtail/docker-wagtail-develop#71 which I'll backport here once complete.