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

Swap to env vars for local dev instance config #728

Merged
merged 4 commits into from
Aug 16, 2024
Merged

Swap to env vars for local dev instance config #728

merged 4 commits into from
Aug 16, 2024

Conversation

amcclain
Copy link
Member

  • 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.

+ 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.
@amcclain amcclain requested a review from lbwexler August 14, 2024 16:42
@amcclain
Copy link
Member Author

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 /etc/hoist/conf config location for the file-based approach.)

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 .env.template and validation in build.groovy also allows us to ensure that any new + required configs are set by developers in their local .env file.

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 build.gradle. See included edits to README

Copy link
Member

@lbwexler lbwexler left a 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}")
}
}
Copy link
Member Author

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
Copy link
Member Author

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.

@amcclain amcclain merged commit 4b79fb6 into develop Aug 16, 2024
5 checks passed
@amcclain amcclain deleted the dotEnv branch August 16, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants