Add support for local secrets #227
Conversation
.gitignore
Outdated
| # This file is used in local development to pass an /app/.env | ||
| # file to the container, for secrets. It should not be committed | ||
| # to the repo because tests and CI/CD will not have an .env file. | ||
| docker-compose.override.yml |
There was a problem hiding this comment.
So, I wonder if the real issue here is where we keep the docker-compose file. Would there be any issue moving it to the app folder itself?
That's what we did on the simpler grants gov repo (this api folder is the equivalent of app): https://github.com/navapbc/simpler-grants-gov/tree/main/api
We do have a top-level docker-compose but just for the purposes of spinning up both the frontend and API together.
There was a problem hiding this comment.
I agree that inside the app folder seems more appropriate to me.
| # if you are running the app directly, or | ||
| # in your `app/.env` if you are running the | ||
| # app in a Docker container |
There was a problem hiding this comment.
Threading arbitrarily, - we might also need a change to the load_local_env_vars method which handles getting the local.env file into your env vars when you run outside of docker. Right now that automatically gets called for unit tests in the conftest file:
https://github.com/navapbc/template-application-flask/blob/main/app/tests/conftest.py#L21
I think if we just adjusted load_local_env_vars to do "if .env exists, load it before the local.env file" bit - then it would work.
There was a problem hiding this comment.
Alternatively - we could get rid of load_local_env_vars entirely and more accurately document how to do that properly when running outside of docker. Maybe I'll take that on next week.
docker-compose.override.yml.example
Outdated
| main-app: | ||
| env_file: | ||
| - ./app/local.env | ||
| - ./app/.env |
There was a problem hiding this comment.
I'm curious if we want to name the override env file something like override.env to make it clear on the behavior.
There was a problem hiding this comment.
apologies in advance for not providing more actionable or constructive feedback, but this approach feels kinda hacky and non-standard to me. i don't think this is a typical structure you see for projects using docker and docker compose. i prefer to come up with a "boring" solution that isn't something someone needs to learn if they come from another place.
| 1. In your terminal, `cd` to the `app` directory of this repo. | ||
| 2. Make sure you have [Docker Desktop](https://www.docker.com/products/docker-desktop/) installed & running. | ||
| 3. Run `make setup-local` to install dependencies |
There was a problem hiding this comment.
Removed these lines:
- 1 is redundant with the note about running from within the app folder (which I also moved to be directly above these steps)
- 2 is redundant with the prerequisites above. If someone does miss that note, the error message is very clear ("Is Docker running?")
- I believe 3 is not necessary since these steps assume running everything in the Docker container. For folks who do want to run things like formatting, etc, directly on their machine, the linked application docs under Next steps at the end of this do a great job explaining how to do so (including doing this step)
docs/app/getting-started.md
Outdated
| 1. Run `make init start` to build the image and start the container. | ||
| 2. Navigate to `localhost:8080/docs` to access the Swagger UI. | ||
| 3. Run `make run-logs` to see the logs of the running API container | ||
| 4. Run `make stop` when you are done to delete the container. |
There was a problem hiding this comment.
This line is a bit misleading. Deleting might make someone think that the contents of their container will be removed when they run stop, but that isn't true as that's cached and/or stored in the volume.
Maybe we should adjust this to something like .. when you are done to stop the container.
There was a problem hiding this comment.
👍 will make this change!
## Ticket n/a ## Changes - #227 moved the docker-compose.yml file out of the root and into /app, but missed updating links to it in the documentation. This updates those links. ## Context for reviewers n/a ## Testing n/a
Ticket
n/a
Changes
Context for reviewers
Testing
Follow the instructions in the documentation and verify the contents of your .env match what's available to the Docker container.