-
-
Notifications
You must be signed in to change notification settings - Fork 69
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
base: main
Are you sure you want to change the base?
Conversation
866f68c
to
245732a
Compare
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). |
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. |
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. |
Congrats on the release of dry-system 1.0! Is there anything I can do to help move this card along? |
Just wondering if |
Closes #253
This PR adds 2 new configuration options to the settings provider source:
prefix
: sets the prefix of the environment variables that will be read. E.g with a prefix ofAPP1_
and a setting namedlog_level
, the settings provider will look for an environment variable namedAPP1_LOG_LEVEL
.register_as
: sets the namespace that the setting will be registered to. This enables settings objects to be organised according to their purpose e.gContainer['app.config.database']
and also allows multiple settings providers to be registered.Update: rebased to latest
main