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.

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

Allow database variable injection for config.json #389

Merged
merged 9 commits into from
Feb 28, 2023
Merged

Allow database variable injection for config.json #389

merged 9 commits into from
Feb 28, 2023

Conversation

spwoodcock
Copy link
Member

  • Currently start-odk.sh substitutes environment variables into config.json.template to create a final config file under /usr/odk/config/local.json.
  • This substitution does not allow for the database variables (host, credentials, db) to be injected (the user must keep the default).
  • This PR keeps the defaults, but also allows the user to specify 4 variables in their .env as an override, which will be injected into the config:
CENTRAL_DB_HOST=
CENTRAL_DB_USER=
CENTRAL_DB_PASSWORD=
CENTRAL_DB_NAME=

Note: currently I use a hacky ENTRYPOINT in my dockerfile to update the config file with jq:

RUN printf '#!/bin/bash\n\
set -eo pipefail\n\
tmp=$(mktemp)\n\
echo "Setting database credentials in /usr/share/odk/config.json.template"\n\
jq --arg host "${CENTRAL_DB_HOST:-central-db}" --arg user "${CENTRAL_DB_USER:-odk}" \
--arg pass "${CENTRAL_DB_PASSWORD:-odk}" --arg db "${CENTRAL_DB_NAME:-odk}" \
'"'"'.default.database.host = $host | .default.database.user = $user | .default.database.password = $pass | .default.database.database = $db'"'"' \
/usr/share/odk/config.json.template > \
"$tmp" && mv "$tmp" /usr/share/odk/config.json.template\n\
echo "Credentials set"\n\
exec ./start-odk.sh' \
>> ./sub-db-vars.sh \
&& chmod +x ./sub-db-vars.sh
ENTRYPOINT ["./wait-for-it.sh", "central-db:5432", "--", "./sub-db-vars.sh"]

@lognaturel
Copy link
Member

lognaturel commented Feb 22, 2023

Thanks, @spwoodcock, this is a nice improvement and fits in nicely with other database-related work we've been doing.

We're going to do a little more thinking through it and see whether or not it makes sense to get into the release that we're planning for next week. Some things that will need to be done:

  • Remove CENTRAL_ prefix for env vars. If we ever have other databases (cache or something), then we can prefix those.
  • Use vars in call to wait-for-it.sh (line 37 of the docker-compose file)
  • rebase onto next to combine with postgres upgrade changes
  • update custom database instructions
  • either update v2023.2 upgrade instructions or write standalone instructions for migrating from current custom db setup
  • from @matthew-white: consider what to do about possible special characters in e.g. passwords

@lognaturel lognaturel changed the base branch from master to next February 22, 2023 21:11
@lognaturel
Copy link
Member

I'm leaning towards doing this as part of v2023.2 because it does simplify the merge conflict resolution for folks who have custom database configs. @spwoodcock if you're able to do some of the things in the list above, great. I can do the documentation parts and am happy to also do the rest during my day tomorrow.

@alxndrsn you may also want to take a quick look at this and see if you have any other comments.

@spwoodcock
Copy link
Member Author

Hi @lognaturel, thanks for the feedback!

I completed the first 3 tasks as requested (rebased with master first, then next).

If it's ok I'll leave the docs with you, as you probably know best what to write.

Let me know if I can help with anything else!

@lognaturel
Copy link
Member

lognaturel commented Feb 25, 2023

Great, thanks! I made sure host env vars are passed on to the service container so they're available to the start-odk script and now I believe that this works. I've verified it on https://dev.getodk.cloud with an RDS instance.

@yanokwa could you please review?

I'm wondering whether we should move other custom config into .env at the same time so that users can do this kind of migration once. In particular, folks who have custom dbs almost certainly also have custom email configurations. If we move that out to .env then users wouldn't see unmerged changes when they have custom configs. One possible disadvantage is that the environment definition will become long. I think we should use env_file instead. Note: there should not be a merge conflict with the mail config section of the .json.template file at this time. It's just that if we do it later then we will need to document the migration process again.

.env.template Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ done
Development

Successfully merging this pull request may close these issues.

3 participants