kelseyhightower / envconfig

Golang library for managing configuration data from environment variables
MIT License
5.07k stars 382 forks source link

Support Struct fields #51

Closed evandigby closed 8 years ago

evandigby commented 8 years ago

Implements #36

This pull request proposes a way to allow for struct fields without falling into the trap of recursive types.

If you simply have it recursively call "Process" for sub-structs, if someone makes a recursive type (such as a linked list) it will of course cause a SO.

My proposed solution is to require the user set a non empty string value to the name of the struct field if you intend to provide values for the struct.

This follows fairly naturally with the structure of a YAML file and many other config formats:

normalfield: value
connection:
  username: guest
  url: localhost:1234

In YAML Becomes in the ENV:

export MYAPP_NORMALFIELD=value
export MYAPP_CONNECTION=x
export MYAPP_CONNECTION_USERNAME=guest
export MYAPP_CONNECTION_URL=localhost:1234

Although it's a bit clunky to have to put "x" or "true" or another value there, that can be fixed if it's ever decided to go to "LookupEnv".

The side-benefit of this method is that it will naturally work with the current code (as the code looks for a value for a field before it attempts to process the field). That value is then passed to processField.

The struct field will still follow the existing pattern which will first attempts to find a custom decoder before falling into the new code, so this shouldn't be a breaking change for those who have worked around this with a custom decoder.

Our use case for configuring our services, which I believe is not unique, is that each component we develop has its own "Config" type. When we develop a new service which utilizes that component we will add the components config type as a field in the service's "root" config type.

Example:

type serviceConfig struct {
     NumberOfThings string // Some regular top level config for the service
     Rabbit         *bus.RabbitMQ // Rabbit MQ Bus Component Config
}
evandigby commented 8 years ago

There are a few additional test cases that need to be validated, such as alternate names, default values, and handling the recursive case if a user sets the default value for a struct field to be set. The readme also needs to be updated.

I would be interested in feedback on this approach for solving embedded struct fields before taking the time to work through some of these issues in more detail.

Thanks!

teepark commented 8 years ago

@evandigby I like this for named struct fields! I think alternate names, default values etc are dealt with by the recursive call to Process().

The only thing I'd like to see (dovetailing with the conversation on #36) is killing the non-empty placeholder env variable requirement, and tacking the current field name (or envconfig:"tag") onto the prefix before the recursive call.

Oh and something about this in the README :)

evandigby commented 8 years ago

@teepark Thank you!

Regarding the prefix:

I believe passing the fully resolved "key" as the new "prefix" into the recursive call takes care of tacking the current field name on--or are you referring to something related but different? I may not be understanding the comment.

Regarding the non-empty placeholders:

Given the two options (or other options I'm not thinking of) presented in #36 do you have an opinion on how we handle removing the need for the placeholders?

The two solutions I mentioned would require a bit of a refactor to handle the special case in the "Process" function rather than the "processField" I believe. I'll take some time to think a way around this, because I much prefer it to be handled by process field.

The reason this is necessary is because "processField" is called only after "Process" has verified that the field exists in the environment. It will never call "processField" on a struct if the placeholder isn't set.

One solution would be to move the code that loads the field from the environment from "Process" into "processField" (or into its own function) and only call it for field types it expects to find something for. For now, the only time it wouldn't expect to see it is the struct--still a "special case", but I think it's more appropriate to handle that special case inside "processField" as opposed to "Process".

Does that make sense as a solution?

My next steps as I see them:

  1. Try the refactor of field loading into processFields to eliminate the need for a placeholder (unless you or someone else can point out reasons why that's a bad idea that I may be missing).
  2. Select a "limiting" option as described in #36 (do you have a preference?)
  3. Update README.