-
Notifications
You must be signed in to change notification settings - Fork 1
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
Swap to env vars for local dev instance config #728
Conversation
+ Install dotenv Gradle plugin to source env vars from a `.env` file for local development launches via `bootRun`. + Include `.env.template` (without any credentials). + Update README, including new instructions on wrapper project configuration.
Wanted to review with you then - assuming we like this route - with the larger team. Motivated by desire to minimize need for YAML file, especially at our clients where it can be a source of confusion. (Further prompted by cutover to Coder-based VMs at one client where we can't create the default I think the use of a .env file is more standard and expected, and we are moving to use env vars for instance configs more consistently in deployed environments. That was making it a bit weirder to have this YAML file only for local dev. Use of See xh/hoist-core@fcfa671 for updates to hoist-core util docs to further emphasize env vars. Only annoyance was extra complexity required by wrapper project configuration. The pluging this uses needs to be applied to the root project. For non-wrapper setups, would look like any other plugin. For Toolbox where we need to support wrapper, required different syntax + change to local wrapper setup to add |
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.
Looks good -- would be happy to discuss in more detail at any time
if (missingEnvVars) { | ||
throw new GradleException("Environment variables listed in .env.template not set in local .env file as required: ${missingEnvVars}") | ||
} | ||
} |
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.
Realized that this will only catch the case of vars that are not listed in the local .env
file at all (including the case where that file has not been created, in which case all vars in the template will be listed).
Does not catch the case where the user has an empty assignment, e.g. APP_TOOLBOX_DB_PASSWORD=
. That var will be there, just with a value of empty string.
In general there's no such thing as an env var having a value of null
- that's a special feature of the allVariablesOrNull()
method, where it returns null for vars in the template that are missing from .env
.
We could tighten this up to also throw on empty strings, but I dunno if we want that - a user might have a blank local MySQL password. We would also need to limit to APP_TOOLBOX
prefix only, as you might have some other empty env var on your machine.
I think it's fine as it is - ensures you create a local file, and if we ever add a new var to the template will catch that too. Just noting as it confused me this morning.
APP_TOOLBOX_DB_HOST=localhost | ||
APP_TOOLBOX_DB_SCHEMA=toolbox | ||
APP_TOOLBOX_DB_USER=toolbox | ||
APP_TOOLBOX_DB_PASSWORD=toolbox |
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.
As per our discussion in our team meeting, could remove these defaults from the template, as these values will not be used (as the comment at top explains).
I could go either way. They don't seem harmful to have here, and it's kind of nice to see a sample value (which might well be a working value). That said, they also should not be difficult for a developer to work out what values go in those slots, at least for these vars.
.env
file for local development launches viabootRun
..env.template
(without any credentials).