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

Add a Docker Compose setup for development. #128

Merged
merged 14 commits into from
Jul 1, 2021

Conversation

toolness
Copy link
Contributor

@toolness toolness commented Jun 11, 2021

This attempts to add support for contributing to the project using Docker Compose, as mentioned in #120 (comment). My hope is that this will make it easier for contributors (or at least, ones familiar with Docker Compose) to get up and running.

Specifically, it does the following:

  • Runs test_project interactively on port 8000
  • Runs the Sphinx auto-reloading server on port 8001
  • Runs a Postgres database that test_project uses

Instructions

Note: These instructions are now out of date; see this PR's contributing.md for up-to-date instructions.

Setting up the Docker Compose environment can be accomplished with:

docker-compose build

# Wait a few seconds after running this, to give Postgres time
# to create the initial account and database.
docker-compose up -d db

docker-compose run app python manage.py migrate
docker-compose run app python manage.py createsuperuser

Once you've done that, you can run:

docker-compose up

This will start up both the test project and the Sphinx documentation server. If you only want to start one of them, you can use docker-compose up app to start up only the test project, or docker-compose up docs to start up only the documentation server.

You will probably want to visit http://localhost:8000/admin/ to log in as your newly-created superuser, and then visit http://localhost:8000/dashboard/ to tinker with the dashboard UI.

If you want to run the test suite, you can run:

docker-compose run app pytest

To do

  • Figure out if this is actually something @simonw wants merged into the codebase (it's fine if not; since this PR solely adds files to the repo and doesn't change existing files, I can always use this stuff separately for my personal use)
  • Add documentation about all this to contributing.md.
  • See if we can automate the initial manage.py migrate.
  • See if we can dynamically use the UID of the current user rather than hard-coding it to 1000--or at least, provide a way to specify a custom value for UID/GID.

@toolness
Copy link
Contributor Author

Ok here's a rough attempt @simonw, let me know if this is something that you think would be useful to add to the repo. No worries if not--as I mentioned in the PR description, since this only adds files to the repo and doesn't change anything, I (and anyone else who prefers docker-based development) can always just use this stuff separately.

@simonw
Copy link
Owner

simonw commented Jun 11, 2021

I like this a lot. I'm not at all opposed to optional Docker stuff - it's especially good for helping people get a working dev environment with as little friction as possible.

Do you know if it's possible to use Docker Compose in this way but allow users to easily override which ports are forwarded to on the host system? My port 8000 and 8001 are frequently running something else, would be neat if there was a pattern that meant I didn't have to free up those ports before running this.

This has made me think that maybe a cool ability would be if you could run a pre-compiled Django SQL Dashboard in its own Docker container and point it at an existing PostgreSQL database via an environment variable...

@simonw
Copy link
Owner

simonw commented Jun 11, 2021

I particularly like that this runs the Sphinx auto build container too - helps enforce a culture that documentation isn't an afterthought.

@simonw
Copy link
Owner

simonw commented Jun 11, 2021

Wait a few seconds after running this, to give Postgres time to create the initial account and database.

I think we can avoid that step entirely by adding the following to the Docker Compose file:

services:
  app:
    # ...
    depends_on:
    - db

Might need an extra trick to wait for the DB to fully start though, see https://docs.docker.com/compose/startup-order/

@simonw
Copy link
Owner

simonw commented Jun 11, 2021

docker-compose run app python manage.py migrate

It would be nice if we could automate this too - maybe have the development container run it on startup? Definitely not a blocker though, just a nice-to-have.

@simonw
Copy link
Owner

simonw commented Jun 11, 2021

Do you know if it's possible to use Docker Compose in this way but allow users to easily override which ports are forwarded to on the host system?

It looks like there's a pattern for making ports configurable with environment variables - see replies to this tweet: https://twitter.com/simonw/status/1403383417899675651

@simonw
Copy link
Owner

simonw commented Jun 11, 2021

https://docs.docker.com/compose/compose-file/compose-file-v3/#variable-substitution looks like the trick. Looks like we can substitute variables from the environment but provide defaults like this:

services:
  app:
    web:
      ports: "${WEB_PORT:-8000}:8000"

@toolness
Copy link
Contributor Author

Oh nice! Cool, I will make the changes you've suggested and will let you know when it's ready for review!

Dockerfile Outdated
# Set up a non-root user. Aside from being best practice,
# we also need to do this because the test suite refuses to
# run as the root user.
RUN groupadd -g 1000 appuser && useradd -r -u 1000 -g appuser appuser
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this assumes that the UID of the current user is 1000, which it usually is on individual OS X and Linux machines (it's the UID of the first non-root account). However, if it's different, the user could run into some really annoying permission issues.

There are some funky workarounds to set this to the UID of the user who is currently active, I think. I can try looking into it.

Comment on lines 16 to 17
- "${APP_PORT:-8000}:${APP_PORT:-8000}"
command: python manage.py runserver 0.0.0.0:${APP_PORT:-8000}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that it wasn't necessary for me to set the internal port to APP_PORT here, but I wanted to because the development server prints a message saying what port it's on, e.g.:

Starting development server at http://0.0.0.0:8000/

I wanted the port displayed in such messages to always be whatever the user's environment variable is set to, to avoid confusion, which is why I'm using APP_PORT inside the container too.

I used the same approach for the docs server below, too.

docs/contributing.md Outdated Show resolved Hide resolved
@toolness toolness marked this pull request as ready for review June 12, 2021 16:47
@toolness
Copy link
Contributor Author

Ok @simonw, I think this is good enough for a first attempt, let me know what you think!

For instance, if your UID and GID are 1001, you can build your container with the following arguments:

```
docker-compose build --build-arg UID=1001 --build-arg GID=1001
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I could have tried to auto-detect the user's UID/GID but I didn't have a lot of confidence that it'd work across different kinds of operating systems and such, so instead I just opted to make it default to 1000 with an option to override.

@toolness
Copy link
Contributor Author

docker-compose run app python manage.py migrate

It would be nice if we could automate this too - maybe have the development container run it on startup? Definitely not a blocker though, just a nice-to-have.

Just automated this in 3570550! This was easy since migrate is idempotent... the developer will still need to manually run createsuperuser though.

@toolness
Copy link
Contributor Author

Ok I think this is good to go @simonw!

@simonw simonw merged commit e82f8a6 into simonw:main Jul 1, 2021
simonw added a commit that referenced this pull request Jul 1, 2021
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.

2 participants