Conversation
| environment: | ||
| # Set the MongoDB connection string to connect to the mongo service | ||
| MONGO_URI: "mongodb://admin:root@mongo:27017" |
There was a problem hiding this comment.
This was not being used. Rather than update it to make it reference the newly-introduced environment variables (but still not be used), I opted to delete it.
| # Note: The `--exit-code-from test` option applies the exit code of the `test` container | ||
| # to the `docker compose` process, so that the GHA step fails if tests fail. | ||
| # Reference: https://docs.docker.com/reference/cli/docker/compose/up/ | ||
| - name: Spin up `test` container | ||
| run: docker compose up test | ||
| run: docker compose up --exit-code-from test test |
|
There is some overlap between this PR and #77; for example, both introduce a test involving accessing the MongoDB database. |
| # back to default values for variables not defined on the host. | ||
| # Docs: https://docs.docker.com/compose/how-tos/environment-variables/set-environment-variables/#use-the-environment-attribute | ||
| environment: | ||
| MONGO_HOST: ${MONGO_HOST:-mongo} |
There was a problem hiding this comment.
I assume the -mongo here is a default value for the environment variable? If so, do you think we should support defaults, or put our efforts toward making it clear what environment variables everyone needs to have defined? Just asking the question so it's been asked. :-)
There was a problem hiding this comment.
Thanks for reviewing this! I can confirm the first part immediately:
I assume the
-mongohere is a default value for the environment variable?
Yes (reference). If—in the host environment—the MONGO_HOST environment variable is either not defined or its value is an empty string, then—within the container—that environment variable will be given the value mongo.
I'll think about the second part of your question now and reply after a break. I like the question. 👍
There was a problem hiding this comment.
If so, do you think we should support defaults, or put our efforts toward making it clear what environment variables everyone needs to have defined?
Thanks for asking.
Here are my thoughts:
- I do like the idea of there being only one layer at which the configuration parameters' values are defined. By "layer," I mean (a) the host environment, (b) an
.envfile, (c) thedocker-compose.ymlfile, or (d) the PydanticSettingsclass definition. I think limiting it to one layer would make it easier for us to trace down the source of misconfigurations. - I don't think I can declare a configuration parameter in the Pydantic
Settingsclass definition without either giving it a default value there, or passing its value into theSettings()constructor via a kwarg. This is based upon the red squiggly lines VS Code is showing and what I am/am not seeing in thepydantic-settingsdocumentation. I could set its type toOptional[str], for example, but—if I do—Pydantic won't warn me about its value not being astr. - I can update the
docker-compose.ymlfile to require that the host environment (or.envfile) have a given environment variable defined, instead ofdocker-compose.ymlcontaining default values. I think this would be a step in the right direction, and I plan to make this change right after I post this comment. Here's how the syntax used indocker-compose.ymlwould change:When an environment variable is missing and someone runs- MONGO_HOST: ${MONGO_HOST:-mongo} + MONGO_HOST: ${MONGO_HOST:?}
docker compose up, they'll see a message like this:$ docker compose up --detach error while interpolating services.app.environment.MONGO_HOST: required variable MONGO_HOST is missing a value
- The end result of me making the change described in bullet point 3 is that the configuration parameters will get their values from one of three layers, instead of one of all four layers listed in bullet point 1; i.e. the
docker-compose.ymlfile would be off the list.
jeff-cohere
left a comment
There was a problem hiding this comment.
Very nicely done and carefully documented, @eecavanna . I posed one question, but this looks good to me.

On this branch, I made changes related to Docker, environment variables, and testing.
Main changes
pydantic-settingspackage (documented here, and also used bymicrobiomedata/nmdc-server) to get the MongoDB connection parameters from environment variables. The precedence order (from highest to lowest) is: OS,.envfile, class attribute defaults. Fixes Update FastAPI application obtain MongoDB credentials from environment (not hard-coded) #53..envfile: I included an example.envfile named.env.exampleand instructions on creating one's own.envfile. Here's an example.envfile:.envfile. The test database is always namedbertron_test(we can make this, too, be read from an environment variable in a future PR). As a protective measure, tests will not run if the non-test database is also namedbertron_test.Tangential changes
testcontainer was not being propagated to thedocker composecommand, itself. With that bug in place, even if a test had failed, the GHA step would have succeeded. Now, GHA "sees" that status code, so the step fails when a test fails..dockerignorefile so Docker omits some unnecessary directories from the container image it builds. I did this while working on the GHA workflow (see next bullet point)..venv/collisions between host and container: I updated the GHA workflow to run the tests in thetestcontainer only, instead of also running them locally. That eliminated the need to handle collisions between the.venv/directory created by the containers and the one the localuvprogram tries to create. This can be un-done, in case team members want the GHA workflow to also run the tests locally. Note: Previously, the "run tests locally" step was only running tests defined in the top-leveltests/directory, which was only the "hello world" test.