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

Improve Docker development workflow #719

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jake-low
Copy link
Contributor

@jake-low jake-low commented Oct 1, 2024

This PR makes it easier to run the complete OSMCha stack in both development and production configurations using Docker Compose. My intent is for this to become the preferred way to run the app during development. The advantage of this is that it provides a standard environment for contributors to use, and it creates a development configuration that's more similar to what we actually end up deploying in prod (via EKS).

Notable changes:

  • The Django app no longer serves static files, even in development mode. Instead, python manage.py collectstatic is run at build time to collect static assets and place them in a Docker Volume, which can then be mounted into the frontend container at runtime which will serve them. This mirrors how static assets are served in production.
  • The Django app now runs under gunicorn by default, even in dev mode.
  • Database connection parameters have been simplified: they are now read from the environment variables $PGHOST, $PGPORT, $PGUSER, etc. None of the Django configuration modules override these or set fallbacks if they aren't defined, ensuring that connecting to the database works the same no matter what config is being used. As a bonus, these variables match what psql and other tools use, so you can now open a shell in your dev container (or, hypothetically, in the production pod) and run psql with no arguments to connect to the database.

This PR goes along with OSMCha/osmcha-frontend#749 - the changes to the Docker Compose workflow here assume that the osmcha-frontend container is a full-fledged, production ready Nginx server, as implemented in that PR.

NOTE: This PR will require some minor changes to the Helm charts in the osmcha-deploy repo to deploy to prod safely. It changes the names of some environment variables used to configure the application, and requires the deployment to set some additional environment variables (like BACKEND_URL and REDIS_URL) which were implicit before. I will help with making these changes 🙂

This brings the local dev environment closer to production by using
the same frontend container. It also changes the backend to serve with
gunicorn by default, even in dev mode. Also, static files are now always
served by the frontend and never by Django, even in dev mode.
@jake-low jake-low changed the base branch from jlow/remove-static-files to master October 1, 2024 01:45
@willemarcel
Copy link
Collaborator

@jake-low I'm having this error:

❯ docker-compose build
yaml: unmarshal errors:
  line 41: mapping key "volumes" already defined at line 3

@jake-low
Copy link
Contributor Author

jake-low commented Oct 24, 2024

Thanks for testing! I just pushed a commit that should fix this. Interestingly I didn't encounter this error, but maybe we're using different Docker versions.

You should be able to run docker compose up now and all four containers should start up. It seems like on a cold start the django container goes into an infinite loop, logging FATAL: database "osmcha" does not exist every 1s. By running docker compose exec django createdb and then docker compose exec django python manage.py migrate in a separate terminal I was able to resolve this, after which the app finished starting up and became available on http://localhost:8000/.

I'll see if I can find a way to run createdb and migrate automatically if necessary so that this step isn't needed.

@willemarcel
Copy link
Collaborator

I tried to test it, but I wasn't able to finish. My docker abilities are very poor. I don't know if @sunu has time to test it. If not, you can merge it.

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