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

obs-443: switch to docker compose watch #6873

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

Conversation

willkg
Copy link
Contributor

@willkg willkg commented Jan 23, 2025

Currently for the local dev environment, we volume mount the local directory as /app which causes anything built during docker image build to be shadowed by the user's local directory. We get around this by having docker image build any artifacts into directories in the image that are not under /app. This is a little wonky and there are hacks strewn about to alleviate the consequences.

Instead of doing that, this PR is an initial pass and switching to docker compose watch. In this new model, we no longer volume mount the local directory as /app and instead docker compose watches for changes on the local file system and syncs them with the container and will run any rebuilding scripts.

This should work with the local dev environment, CI, and server environments.

This requires v2.22.0 of docker compose plugin.

This doesn't update documentation and doesn't document edge cases to be aware of.

Current problems:

  1. If you make changes to the source when not running the container, you have to rebuild the image before running the container otherwise those changes won't be in the container.
    1. Maybe just run should always build first, then run? Is that too onerous?
    2. What should we do about just shell and other just rules?
  2. We have to add the develop -> watch thing to every service listed in docker-compose.yaml that needs it. It's not sufficient to do it for the app image. I think I got them all, but maybe I missed something.
  3. CI will need at least docker compose version 2.22.0 otherwise it'll balk at the develop key. Does CI have the right version?
    1. It's running v2.27.1, so I think we're fine.

To do:

  • probably should fix just run to always build first
  • update developer documentation
  • fix static asset building so it puts it in /app in a default place
    • pretty sure this means we don't have to run ./manage.py collectstatic after just testshell and before running tests anymore
  • fix node modules installation so it puts it in /app in a default place
  • figure out what happens if the image builds node modules and then the user updates them--do they sync with the container? are there cases where the user ends up in a weird state and has to just build again?

Currently for the local dev environment, we volume mount the local
directory as `/app` which causes anything built during docker image
build to be shadowed by the user's local directory. We get around this
by having docker image build any artifacts into directories in the image
that are not under `/app`. This is a little wonky and there are hacks
strewn about to alleviate the consequences.

Instead of doing that, this PR is an initial pass and switching to
docker compose watch. In this new model, we no longer volume mount the
local directory as `/app` and instead docker compose watches for changes
on the local file system and syncs them with the container and will run
any rebuilding scripts.

This should work with the local dev environment, CI, and server
environments.

This requires v2.22.0 of docker compose plugin.

This doesn't update documentation and doesn't document edge cases to be
aware of.
@willkg willkg requested a review from a team as a code owner January 23, 2025 15:53
@willkg
Copy link
Contributor Author

willkg commented Jan 23, 2025

@toufali This is the PR with the docker compose watch stuff in it. If we're going to go this route, it'll be easier to land this before landing your next set of changes.

Do you want to look at this and see if you can finish it up?

watch:
- action: sync
path: .
target: /app
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This includes the sync action which will copy files from the host to the container.

There's also a rebuild action which I think we want to use to run manage.py collectstatic and esbuild when js and css files change.

We only need this rebuild on the webapp service because that's the only one using js and css files.

Copy link
Contributor

@toufali toufali left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started @willkg ! A couple questions:

Should we ignore the node_modules folder and similar?

Are we now able to change this line so Node can find modules?

@toufali
Copy link
Contributor

toufali commented Jan 23, 2025

@toufali This is the PR with the docker compose watch stuff in it. If we're going to go this route, it'll be easier to land this before landing your next set of changes.

Do you want to look at this and see if you can finish it up?

Yes! Thanks for posting, I'll work on this. See also my questions above that I'm happy to add if you agree.

@toufali
Copy link
Contributor

toufali commented Jan 30, 2025

@willkg I've spent a bit of time testing this – using Compose Watch accomplishes the goal of not overwriting files in the app folder, as discussed. However, I am not seeing the "sync" behavior as expected. Would love to pair on this when you're able!

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