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.

Sign up for GitHub

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

Jump to bottom

Fix: use correct storage directory for client service #2621

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Fix: use correct storage directory for client service #2621

wants to merge 2 commits into from
+3 −0

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Jan 9, 2025 •
edited
Loading

Closes #2620

This PR adds volume mappings to bring in django.db and the uploads folder into /home/calitp/app/data inside the client service.

Explanation

It needs to be this specific path is because the client container needs DJANGO_STORAGE_DIR to be /home/calitp/app/data so that nginx can serve media files correctly. But then, the client container fails to start because there is no database file at /home/calitp/app/data.

This volume mapping gives the container access to the django.db file and the uploads/ directory from the host side of the mapping.

@angela-tran angela-tran self-assigned this Jan 9, 2025
@angela-tran angela-tran requested a review from a team as a code owner January 9, 2025 20:13
@github-actions github-actions bot added the docker Application container, devcontainer, Compose, etc. label Jan 9, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025

Coverage report

This PR does not seem to contain any modification to coverable code.

@angela-tran angela-tran force-pushed the fix/media-files-in-client-service branch from 4e7289f to 0f450bf Compare January 9, 2025 20:16
@angela-tran
fix: add volume mapping for client service
854c49d 
the client container needs DJANGO_STORAGE_DIR to be
`/home/calitp/app/data` because the nginx.conf location blocks use that
base path to serve media files.

the volume mapping is needed so that the container has access to the
django.db file and the uploads/ directory from the host side of the
mapping.
@angela-tran angela-tran force-pushed the fix/media-files-in-client-service branch from 0f450bf to 854c49d Compare January 9, 2025 22:03
thekaveman
thekaveman reviewed Jan 9, 2025
View reviewed changes
Copy link
Member

@thekaveman thekaveman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm confused why this is required? What problem are we trying to solve here? I'm able to bring up docker compose up -d client on the main branch on my machine.

I thought DJANGO_STORAGE_DIR was more of a deployment config (i.e. not really for devtime).

I don't have this setting in my local .env, so maybe that is why it works for me?

@angela-tran
Copy link
Member Author

angela-tran commented Jan 9, 2025 •
edited
Loading

Yeah, so you're right that when running client locally, it's not strictly necessary to set DJANGO_STORAGE_DIR. When that env variable isn't set, settings.py will use BASE_DIR, i.e. the working directory, as STORAGE_DIR.

If you run docker compose -d client in that state, the app will come up just fine and use django.db at your project root. Because django.db isn't copied into the app container, an empty database is created by the migration part of the init script. You can start a bash session inside the running container and run ./bin/reset_db.sh to load our sample fixtures into the database.

But then you'll notice that the media files return 404s. And that's the main problem that needed to be solved here.

It was kind of a chain of problems that led to this change, so I totally understand the need for more clarification.

@angela-tran
Copy link
Member Author

I just edited the title of #2620 to be more accurate / descriptive 😅

@thekaveman
Copy link
Member

OK thank you -- I did see a missing agency logo on the modal selector before this PR, and now I see the agency logo 👍

One comment, the effect of this is to have the entire source directory mounted under /home/user/calitp/app/data. Is there any way to reduce this? It takes us away from client which is supposed to really be a clean runtime. I.e. could we mount in just the ./uploads directory or some other subset?

image

@angela-tran
Copy link
Member Author

One comment, the effect of this is to have the entire source directory mounted under /home/user/calitp/app/data. Is there any way to reduce this? It takes us away from client which is supposed to really be a clean runtime. I.e. could we mount in just the ./uploads directory or some other subset?

Great suggestion. Turns out we can map files too, so I updated it to only bring in django.db and the uploads directory: ea3e18a

image

thekaveman
thekaveman requested changes Jan 10, 2025
View reviewed changes
Copy link
Member

@thekaveman thekaveman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I was worried this would break if django.db did not exist. But that doesn't appear to be the case, the client service still comes up OK.

However, if django.db does not exist, a local directory called django.db is created upon service startup:

image

So that isn't what we want. Recall our goal with client has been it should mostly work OOTB -- create an .env file and docker compose up client. Of course this doesn't work as well in the world where we have a stable database file in prod but no such thing locally.

Can we think about this one a little more?

@angela-tran angela-tran marked this pull request as draft January 10, 2025 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thekaveman thekaveman thekaveman requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@angela-tran angela-tran

Labels
docker Application container, devcontainer, Compose, etc.
Projects
None yet
Milestone
No milestone
2 participants
@angela-tran @thekaveman