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

Extract configurable values from json config templates and docker-compose.yml to .env #401

Merged
merged 7 commits into from
Mar 9, 2023

Conversation

lognaturel
Copy link
Member

@lognaturel lognaturel commented Mar 7, 2023

Continuation of #389. The goals are for docker-compose.yml and the service config.json.template to very rarely be customized by users to minimize dirty files.

What has been done to verify that this works as intended?

I tried the mail commit on a server using the default mail container (caribou) and using an external mail service (dev). In particular, I confirmed that it's acceptable to have empty username and password values.

I verified the extraction of the Sentry vars and the logic around the base URL by looking at the generated conf files in the containers.

I did not specifically verify the Node configuration but I think that can be visually inspected.

Why is this the best possible solution? Were any other approaches considered?

It's extending a pattern we've already established. I think it's a good balance of providing configurability in a flexible, secure way and not making dramatic changes to how things are done now.

I briefly looked into using env_file but I can't come up with a way to do defaults without resulting in ongoing file conflicts. The idea here is that the .conf.template files are responsible for the shape of the configuration and some number of almost-always static values, the docker-compose file is responsible for defaults for configurable values, and the .env file is responsible for deployment-specific configuration. It is ok for users to modify the template files if they have really special needs but then they're on their own for managing conflicts, etc.

I tried to use a name in the default for the no-reply email (e.g. ODK Central <no-reply@${DOMAIN}>) but couldn't get that to work. Tried no quotes, backslash-escaped < and >, single quotes, double quotes. It's currently nameless so this maintains the existing behavior.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

  • There may be things that users customize but aren't pulled out into .env. That's annoying because they'll still have conflicts
  • If any of the values from .env are incorrectly passed through docker-compose.yml and the starting scripts, then those values won't make it to the containers they're supposed to go to.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Yes, this will be addressed in getodk/docs#1577

Before submitting this PR, please make sure you have:

  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@lognaturel lognaturel requested a review from yanokwa March 7, 2023 05:05
@lognaturel lognaturel force-pushed the email_vars branch 2 times, most recently from def7335 to 55359c3 Compare March 8, 2023 19:26
@lognaturel lognaturel changed the title Extract configurable values from service json template to .env Extract configurable values from json config templates and docker-compose.yml to .env Mar 8, 2023
.env.template Outdated Show resolved Hide resolved
.env.template Show resolved Hide resolved
files/enketo/start-enketo.sh Show resolved Hide resolved
files/service/scripts/start-odk.sh Outdated Show resolved Hide resolved
@lognaturel lognaturel requested a review from yanokwa March 9, 2023 05:18
@lognaturel lognaturel merged commit 7ac1b7c into getodk:next Mar 9, 2023
@lognaturel lognaturel deleted the email_vars branch March 9, 2023 17:17
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.

2 participants