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

Envopts #33

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Envopts #33

wants to merge 6 commits into from

Conversation

karantin2020
Copy link

This set of commits add configuration functionality to anv backend.
Func WithPreffix allows to add preffix to env var.
Func ToUpper uppercases vars

@aecepoglu
Copy link

aecepoglu commented Jul 28, 2018

so are opts supposed to be a list of processor functions? Do you have a test code written that shows how it's meant to be used?

Because from what I can tell, this helps with the ability to deal with config vars that are styled differently, and does it through an array of processor functions

@yaziine
Copy link

yaziine commented Aug 2, 2018

It's a good idea to have to those options. But what about adding this functionality to the others backends ?

@karantin2020
Copy link
Author

Hello. I think that those options must be separate for each backend (different types for different backends). That's why I added this type inside env backend package.
I agree with you about other backends

@yaziine
Copy link

yaziine commented Aug 6, 2018

Why should we have a different type for each backend ? This is a common use case so I think that we can have a common type .

@thekondor
Copy link

Hey, guys.
By the way I also missed a feature to look for environment variables with a prefix to a key added. In a case if anyone interested, I've recently shared my solution (actually in terms of my own confita-sugar package: https://github.com/thekondor/confita-sugar):

Copy link
Contributor

@rogpeppe rogpeppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for suggesting this. It's a nice idea, but the interaction between opts and the existing logic becomes quite involved, and also, when the options pattern is used, usually, adding extra of the same option is idempotent, but that's not the case here, as env.NewBackend(env.WithPrefix("X"), env.WithPrefix("X")) will add two "X" prefixes, not just one.

I'm wondering whether this PR is worth it. If you want your own custom key transformation logic, it seems like it would be pretty easy to roll your own backend with backend.Func.

We'll consider our options here.

return nil, backend.ErrNotFound
})
}

// WithPrefix adds preffix for searching env variable
func WithPrefix(preffix string) opt {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo:
s/preffix/prefix/

return nil, backend.ErrNotFound
})
}

// WithPrefix adds preffix for searching env variable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// WithPrefix is a backend option that causes the given prefix to be added to all
// configuration names before looking them up with os.Getenv.

?


// WithPrefix adds preffix for searching env variable
func WithPrefix(preffix string) opt {
if !strings.HasSuffix(preffix, "_") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's worth bothering with this. It makes the logic more complex and harder to explain, and it's quite possible someone might want to add a prefix without any intervening underscore.

@@ -11,20 +11,43 @@ import (
// NewBackend creates a configuration loader that loads from the environment.
// If the key is not found, this backend tries again by turning any kebabcase key to snakecase and
// lowercase letters to uppercase.
func NewBackend() backend.Backend {
func NewBackend(fns ...opt) backend.Backend {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically I'm afraid this is a backwardly incompatible change, as someone might have assigned the function to a variable of type func() backend.Backend.

Also, I don't think it's a good idea in general to have unexported functions that use unexported types (it makes the godoc hard to understand and means you can't declare variables of those types).

@@ -20,7 +20,7 @@ Confita is a library that loads configuration from multiple backends and stores
## Install

```sh
go get -u github.com/heetch/confita
go get -u github.com/karantin2020/confita
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a leftover from another fork.

return backend.Func("env", func(ctx context.Context, key string) ([]byte, error) {
val, ok := os.LookupEnv(key)
key = strings.Replace(key, "-", "_", -1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a backwardly incompatible change. The original logic would succeed if an environment variable exists with exactly the same name, but it won't any longer (I'm surprised the tests pass actually - we should fix that).

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.

5 participants