-
Notifications
You must be signed in to change notification settings - Fork 2
ETT-44: Use env vars for database config #146
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
Conversation
|
@Ronster2018 I'll have a corresponding ht_tanka pull request as well soon. |
Also: ETT-36: remove version.rb
a3710ca to
ea8e16a
Compare
|
Tests succeeded; I think the minimal coverage decrease isn't a problem. |
docker-compose.yml
Outdated
| environment: | ||
| REDIS_URL: redis://redis/ | ||
| PREVIEW_EMAIL: true | ||
| MARIADB_HT_RO_HOST: mariadb-test | ||
| MARIADB_HT_RO_USERNAME: datasets | ||
| MARIADB_HT_RO_PASSWORD: datasets | ||
| MARIADB_HT_RO_DATABASE: ht |
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.
Here, we don't need to duplicate the work being brought in from the fragment template above. It looks like the only difference here is the MARIADB_HT_RO_HOST section on line 39 which can be overwritten. I made the following changes locally and here is what I would suggest:
line 19: add &environment anchor
line 37: add <<: *environment alias
line 39: Keep MARIADB_HT_RO_HOST: mariadb-test
to test and check the output in the terminal to make sure things look right, run docker compose -f docker-compose.yml config
environment: &environment
REDIS_URL: redis://redis/
PREVIEW_EMAIL: true
MARIADB_HT_RO_HOST: mariadb-dev
MARIADB_HT_RO_USERNAME: datasets
MARIADB_HT_RO_PASSWORD: datasets
MARIADB_HT_RO_DATABASE: ht
services:
test:
<<: *common-service
restart: never
command: bundle exec rspec
depends_on:
redis: *healthy
mariadb-test: *healthy
environment:
<<: *environment
MARIADB_HT_RO_HOST: mariadb-testThere 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.
Thanks, I'll give that a try!
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 like it should work. Once tests pass I'll go ahead and merge.
In accordance with feedback on the PR
@Ronster2018 Mainly just want to check that this is looking the way you'd expect both in docker-compose.yml and that it matches what you'd expect given https://hathitrust.atlassian.net/wiki/spaces/ETT/pages/3489759233/Database+Credentials+in+Kubernetes