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 env variable prefix and setting namespace to be configured in settings provider #258

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeremiahrose
Copy link

@jeremiahrose jeremiahrose commented Nov 2, 2022

Closes #253

This PR adds 2 new configuration options to the settings provider source:

  1. prefix: sets the prefix of the environment variables that will be read. E.g with a prefix of APP1_ and a setting named log_level, the settings provider will look for an environment variable named APP1_LOG_LEVEL.

  2. register_as: sets the namespace that the setting will be registered to. This enables settings objects to be organised according to their purpose e.g Container['app.config.database'] and also allows multiple settings providers to be registered.

Update: rebased to latest main

@jeremiahrose jeremiahrose force-pushed the custom-env-variable-prefix-for-settings-provider branch from 866f68c to 245732a Compare November 2, 2022 22:41
@solnic
Copy link
Member

solnic commented Nov 4, 2022

Thank you for working on this but this has to wait. We're in the process of releasing dry-system 1.0.0 and then Hanami 2.0.0 and it's code freeze now. We'll get back to this PR after we're done with the Hanami release (so, before the end of the month).

@timriley
Copy link
Member

timriley commented Nov 4, 2022

Thanks @jeremiahrose for this contribution, and thanks @solnic for saying what I was planning to get around to saying too :)

In terms the viability of this feature, I'd definitely be confident in it being a reasonable post-1.0.0 addition. Nothing here e.g. looks like it'd be a breaking change to any of the APIs release to 1.0.0, so it'd be a straightforward expansion/addition of our existing APIs.

However, before merging anything, I expect I'd probably want to have a think about different possible ways of satisfying the end goal here. One thing I worry about with adding built-in support for very specific features (like a variable prefix) is that if we continue that same approach for any other adjustments people want with settings loading, we may end up continuing to glom things on and end up with a hard-to-understand combination of different configs around settings loading (think the homer car, and honestly dry-system is probably already a little like that, so I'd like to be quite intentional in the way we add additional features from here).

For example, another option here could be to instead add support for a single configurable "settings store" (which is something we actually have for Hanami's settings implementation). By default, this would pulling settings straight from the env like we do now. But this would also give us a way to satisfy your needs through providing a custom "prefixed names settings store" that pulls from the env using a provided prefix. This kind of arrangement may ultimately be more flexible to serving different needs without us having to add too much complexity to this part of dry-system.

@jeremiahrose
Copy link
Author

another option here could be to instead add support for a single configurable "settings store" (which is something we actually have for Hanami's settings implementation). By default, this would pulling settings straight from the env like we do now. But this would also give us a way to satisfy your needs through providing a custom "prefixed names settings store" that pulls from the env using a provided prefix.

I guess that could tie in with the current changes quite well, just make the "prefixed names settings store" the default one. I think being able to set environment variable prefixes is a fairly standard feature for an environment loader that most users will want.

@jeremiahrose
Copy link
Author

Congrats on the release of dry-system 1.0! Is there anything I can do to help move this card along?

@jeremiahrose
Copy link
Author

Just wondering if dry-system is still in code freeze and is there anything I can do to help move this card along?

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.

Allow custom environment variable prefixes in settings provider
3 participants