-
Notifications
You must be signed in to change notification settings - Fork 3
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
Refactor: move the application directory up a level #463
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! 👍 I tested by rebuilding the eligibility-server
images locally (the dev container ran without a problem) and then using the eligibility_server
test image for my local benefits app. I didn't test the terraform change but I'm sure it works since it looks just like the terraform change in benefits (PR 2156).
The only thing I saw is that in the compose
file of benefits we use ./:/calitp/app
for the volume mount and here we use .:/calitp/app/
. They are both equivalent so it doesn't matter but if we wanted to make them the same we could.
removed explicit WORKDIR in appcontainer as the base image already sets the default we want
862103d
to
a1f5b78
Compare
Thanks for that feedback, I fixed the mounts to be the same as in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Related to:
This PR should be merged after the one in docker-python-web.
This PR also removes the explicit
WORKDIR
setting in the appcontainer Dockerfile, as the base image already sets the default we want.