Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jsma
Copy link
Contributor

@jsma jsma commented Nov 12, 2023

  • Moved database connection parameters and other environment variables to an environment file
  • Added a persistent volume for the database (should solve Add docker-compose volumne #207)
  • Removed the version number from docker-compose.yml since this is ignored by docker and causes IDEs to attempt to validate
  • Added health checks for containers to avoid warnings/errors (and removed "try again" notes from the readme)
  • Updated docs to use exec instead of run since the containers should already be running
  • Updated base images to latest stable versions (Python 3.12, PostgreSQL 16, and Redis 7)
  • Added missing notes about update_index

This was backported from my work in wagtail/docker-wagtail-develop#71

* Move database connection parameters and other environment variables to an environment file
* Remove the version number from docker-compose.yml since this is ignored by docker and causes IDEs to attempt to validate
* Added health checks for containers to avoid warnings/errors (and removed "try again" notes from the readme)
* Updated docs to use `exec` instead of `run` since the containers should already be running
* Updated base images to latest stable versions (Python 3.12, PostgreSQL 16, and Redis 7)
* Added missing notes about `update_index`
@jsma
Copy link
Contributor Author

jsma commented Nov 12, 2023

I did not touch the existing uwsgi bits, is there any reason to still have this around? It appears it's a leftover of "production" style configuration but the docker-compose setup is specifically documented as not being suitable for production. Can we remove uwsgi and get back to a basic manage.py runserver?

REDIS_URL=redis://redis

# Database connection settings:
DATABASE_HOST=db
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@gasman
Copy link
Contributor

gasman commented Nov 13, 2023

+1 for dropping the uwsgi setup. It's not entirely clear to me what the original motivation for it was, but judging from 6652c42 it was done with some sort of Docker-based hosting platform in mind. I'm pretty sure we're not relying on that for any demo/nightly testing infrastructure - that's all on Heroku (right @RealOrangeOne?)

@jsma
Copy link
Contributor Author

jsma commented Nov 13, 2023

+1 for dropping the uwsgi setup. It's not entirely clear to me what the original motivation for it was, but judging from 6652c42 it was done with some sort of Docker-based hosting platform in mind. I'm pretty sure we're not relying on that for any demo/nightly testing infrastructure - that's all on Heroku (right @RealOrangeOne?)

It seems to me that the setup here should be strictly focused on spinning up the demo vs creating a development/debugging environment (that's what docker-wagtail-develop is for).

Looking at this a bit more closely, it looks like we could delete both production.txt and development.txt.

Vagrant only installs base.txt, while the virtualenv option instructs to install development.txt, and then of course the docker setup currently uses production.txt. (Confusingly, base.txt also includes django-debug-toolbar)

Only the docker setup needs psycopg3 and django-redis so I could see having a requirements/docker.txt to keep that separate from base.txt. So what I'm proposing is to only have base.txt (pruned of developer tools) and docker.txt in the requirements folder.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Question: Why are we removing the default settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved DJANGO_SETTINGS_MODULE to .env/.env.example (although it points to the dev settings in this PR). DJANGO_DEBUG was not used anywhere.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should still keep the production default in the Dockerfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

**Important:** This `docker-compose.yml` is configured for local testing only, and is _not_ intended for production use.

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:

  • Gitpod installs requirements.txt which is essentially just an alias for requirements/production.txt
  • Vagrant installs requirements/base.txt
  • Docker Compose installs requirements/production.txt
  • Virtualenv installs requirements/development.txt

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 base.txt (and perhaps an extra file for docker to install the redis/psycopg3 libs).

For anyone who wishes to submit a PR to bakerydemo, they'll need to install development.txt (removing the -r base.txt from that file) in a virtualenv on their host so that they can run ruff etc. on their changes before submitting a PR (the contributing docs needs a note about installing pre-commit).

expose:
- '6379'
healthcheck:
test: ["CMD-SHELL", "redis-cli ping || exit 1"]
Copy link
Member

Choose a reason for hiding this comment

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

Nit: || exit 1 should be a noop

@RealOrangeOne
Copy link
Member

We still need uWSGI, but correct we don't need the entrypoint. Heroku handles running migrations during deploy for us.

@jsma
Copy link
Contributor Author

jsma commented Nov 13, 2023

We still need uWSGI, but correct we don't need the entrypoint. Heroku handles running migrations during deploy for us.

Ah, so there is an undocumented heroku path that relies on production.txt and related settings then? I can leave that untouched, but how does that intersect with the docker install instructions? I thought the primary purpose of this project was for folks to easily spin up a demo so they can check out wagtail's features etc. And of course it is also useful for spinning up a vanilla wagtail project to create a simplified test scenario when debugging or contributing to wagtail. I was not aware of a scenario where folks would want to deploy this in a production environment.

Can we keep things simple for the "I just want to quickly demo wagtail on my machine" and have this more or less be the same whether they are doing this via the virtualenv, vagrant, or docker paths? Perhaps a "if you need to deploy this in a production-like environment to showcase to a distributed team over the web, follow these steps" addition to the documentation with a separate compose-production.yaml?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants