Fix Docker setup - #98
Conversation
This just ensures BuildKit uses the latest version of Dockerfile syntax
This lets us drop the options in the command and also guarantees these options are set when running bundler commands inside the container. Using an absolute path for BUNDLE_GEMFILE also guarantees bundler commands work properly in subdirectories inside the container
This prevents cache busting the entrypoint copy on changes to the application code.
service_started doesn't prevent race conditions since the web service may have started and db operations might still be working. Rails provides an `/up` endpoint which guarantees the _app_ is up. Only after that is it safe to run the entrypoint script
Attempting to reuse the web image presumes it already exists locally. Furthermore, it's better to have them build separately since their dependencies can be different
Provides a build context and pass in the needed build args
arielj
left a comment
arielj
left a comment
There was a problem hiding this comment.
I left some questions, I'm not sure I understand some of the code
| if ENV["CI"] | ||
| # run migrations as root to avoid exec permission issues, then restore ownership | ||
| system! "#{DOCKER_ROOT_PREFIX} rails db:create db:migrate" | ||
| system! "#{DOCKER_ROOT_PREFIX} sh -c 'chown -R 3434:3434 /code || true'" | ||
| else |
There was a problem hiding this comment.
what is this solving? I don't think we use docker in CI, do we even need this? it also looks weird to have the setup change ownership of /code, it's the code mounted as a volume from outside the container, what even is "3434"?
what are the exec permissions issues this is fixing?
| CONTAINER = ENV["BUNDLE_GEMFILE"] == "Gemfile.next" ? "web_next" : "web" | ||
| DOCKER_PREFIX = if ENV["CI"] || ENV["RAILS_ENV"] == "test" | ||
| # run as the non-root CI user so files created by commands are owned correctly | ||
| "docker compose run --user 3434:3434 #{CONTAINER}" |
There was a problem hiding this comment.
similar to the other comment, what is this fixing? sounds weird to have to set the user, I've never seen this before, sounds like there's a different issue to solve (like adding this is a workaround, not a real solution)
| end | ||
|
|
||
| # explicit root-run prefix (used only when we must perform privileged actions) | ||
| DOCKER_ROOT_PREFIX = "docker compose run --user root #{CONTAINER}" |
There was a problem hiding this comment.
I'm not sure which actions we are performing that need to be privileged
We don't run these images in CI and the setup script was using root permissions without a clear reason. Removed the tests on the CI environment to fix both issues
|
@arielj I've removed the branches that were in there to run in CI. I also fixed the permissions on the bon/docker/run script |
2 participants
What is this PR:
Description:
This updates the docker setup for the audit app. Changes docker-compose.yml to build web_next and adds several improvements that help avoid issues with changing dependencies, undesired cache busting when changing application code and also implements caching when running
bundle installThe changes that are actually needed are only the ones configuring web_next to get built from the Dockerfile and removing the version pinning on the
bundle installcommand. The rest is just improvements that I'm suggesting hereHow has this been tested?
What manual tests have been run?
Delete all your local images of for the audit app and attempt to setup the app as in the README.
The rest are standard docker features that require no testing, as long as images build successfully and
docker compose upspins up both containers without issues.