Modernize docker compose #71
Conversation
jsma
force-pushed
the
modernize-docker-compose
branch
from
fec9316
to
db8914c
Compare
|
Thank you for taking the time to help @jsma ! At the time |
|
I'll do my best to test this out today or tomorrow. Another request might be to update to Node 20 in this PR also. See #64 |
When this PR is rebased to main, it will get the node 20 upgrade :) |
* Move database connection parameters to an environment file to avoid duplication * Update to modern command `docker compose` vs `docker-compose` * Remove the version number from the file since this is ignored by docker and causes IDEs to attempt to validate * Removed mount of `node_modules` volume in `web` container since the `web` container does not have `node` installed * Removed `:delegated,rw` since this was the incorrect option (it meant that the container had the authoritative view of the files, leading to long delays in synchronizing with the host which caused issues when trying to detect changes for migrations, etc.), and is no longer necessary in modern docker * Added health checks for containers to avoid warnings/errors * Updated base image to python:3.9-bullseye to at least match the version used in `bakerydemo`'s `docker-compose.yml` * Updated base image to postgres:14.1 to match `bakerydemo`'s `docker-compose.yml`
jsma
force-pushed
the
modernize-docker-compose
branch
from
db8914c
to
7b87fc5
Compare
|
I've cleaned up the branch and rebased from main. |
Renames docker-compose.yml => compose.yaml which is the new preferred name going forward Moves Dockerfiles to subdirectory, ensuring the filenames end with Dockerfile (for syntax highlighting in IDEs) Renamed the `web` service to `app`, since this is typical for Docker examples and is also what bakerydemo uses Various tricks and tips to optimize the build cache to speed up subsequent builds The new Dockerfile was partially based on the output of `docker init`, which includes some best practices: * create non-root users to run the applications * use bind-mounts for pip & npm installs (this means that when adding a new requirement, it will only download the new package and install everything else from the cache) NOTE: the wagtail and libs directories are no longer COPY'd into the container for doing a `pip install`, but installing wagtail and django-modelcluster creates .egg-info directories, so the bind mounts need write access. Added custom ignore files for each Dockerfile to restrict the build context to the absolute minimum Added a custom requirements file to avoid relying on bakerydemo's requirements which results in duplicate installs. The previous setup was installing bakerydemo requirements, which installed wagtail, django-modelcluster, and Willow, and then a few steps later, reinstalling these packages from the local sources. Now they are only installed once. The CMDs for both `app` and `frontend` are now wrapped with `dumb-init` (node is not designed to run as PID 1) which handles killing the processes. Previously attempting the Ctrl-C the running containers would hang waiting for `npm` to give up the ghost, now it exits immediately.
|
I've pushed a rather significant update. I spent a bit more time with the Docker docs and took this "modernization" project more seriously than just borrowing from what I was doing a couple years ago ;) Let me know if you have questions about some of the changes and I'll do my best to explain while it's fresh in my mind. There may be room for some additions related to At any rate, I've got this tuned as well as I can. It builds, starts, and stops much more quickly now. I've only lightly tested the front-end, changing the background color of the 
That said, it's very slow to rebuild. I'm not super familiar with webpack (I'm still old school using django-compressor just for scss, no Javascript build step), so I'm not sure what can be done to optimize. Although any optimizations would likely need to happen upstream in Wagtail. What I've observed is that changing a single scss file results in three rounds of Seems like it should not be doing this ;) I realize this is out of scope a bit, but any objection to including |
|
I can fix the path to the correct Dockerfile for the |
|
@jsma It is as you say, a smoke test to verify that the app container builds so we can at least verify that a contribution is not breaking the setup and build process. So if you could update your PR with the correct path then that would be great. |
|
@jsma I could not get the frontend container to run correctly. Here are the steps I took:
I get an error that I did not get with the previous version of this PR: |
|
If you are working in an existing checkout of this project, have you done the equivalent of |
|
I had the v5.2 tag checked out in wagtail to test something else but now I'm on I redid all the steps, took care to destroy all containers, images and volumes, used Still, I get the same error and somehow the database still persists, there are no migrations to perform 🤔 Do you have any idea about why the postgres volume would persist even if I delete it from Docker Desktop? It seems like stale data is persisting both in postgres and the node volume. |
|
Hey, sorry, I am also having trouble getting this branch to work. |
|
I'm sorry I haven't had a chance to update this branch to wrap things up for now. There are currently issues with editing frontend files and having these reflected in Wagtail. Leaving for a holiday but hope to wrap this up next weekend. As far as the errors, there are new environment variables that only get copied over if you don't already have a That said, I'll let you know when I've pushed my "final" version of this branch. It's not worth your time at the moment with the current issues with the |
|
Thank you @jsma - your help here is really appreciated. Enjoy the break. |
This work in progress attempts to modernize and cleanup the docker configuration to address several issues.
I've based this off two other open PRs I've created (#69 & #70) but can either officially bundle those small fixes into this branch or break this PR into tinier pieces if necessary.
I have not made any significant changes to the
frontendcontainer as the existing approach may need a rethink (active discussion in #64).bakerydemodocker composevsdocker-composenode_modulesvolume inwebcontainer since thewebcontainer does not havenodeinstalled:delegated,rwsince this was the incorrect option (it meant that the container had the authoritative view of the files, leading to long delays in synchronizing with the host which caused issues when trying to detect changes for migrations, etc.), and is no longer necessary in modern dockerbakerydemo'sdocker-compose.ymlbakerydemo'sdocker-compose.yml