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

Default value for nested key #71

Closed
LStuker opened this issue May 20, 2015 · 20 comments · Fixed by jacobstr/confer#7
Closed

Default value for nested key #71

LStuker opened this issue May 20, 2015 · 20 comments · Fixed by jacobstr/confer#7

Comments

@LStuker
Copy link

LStuker commented May 20, 2015

To support yaml and json config files, it would be nice if it is possible to set default values for nested key.

Currently when I set a default value for.

viper.SetDefault("datastore.metric.host", "0.0.0.0")

The key would not be interpreted as a nested key and it is not possible to overwrite it with a yaml or json config file

@corpix
Copy link

corpix commented May 28, 2015

Same thing for toml config files

@dverbeek84
Copy link

I have the same experience. +1

@dverbeek84
Copy link

workaround for now:

if viper.Get("cluster.enabled") == nil {
  viper.SetDefault("cluster.enabled", false)
}

@LStuker
Copy link
Author

LStuker commented Jun 18, 2015

An implementation to set default values for nested keys would would break the current api of viper.
So it would be nice @spf13 would comment on this issue if viper should support this in feature.

Then someone can work on this and contribute to viper.

@anuaimi
Copy link

anuaimi commented Jul 26, 2015

I'm also facing this issue.

@kofalt
Copy link

kofalt commented Aug 14, 2015

@jacobstr I think that merge in your fork accidentally closed this issue.

@LStuker mind re-opening it please?

@LStuker
Copy link
Author

LStuker commented Aug 14, 2015

I don't have the permission to re-open the issue because I didn't closed it by my self. Only maintainer can do this. @jacobstr and @spf13 can you please re-open the issue and give an comment on this issue.

@jacobstr jacobstr reopened this Aug 14, 2015
@jacobstr
Copy link
Collaborator

Sorry guys! No idea why my merge closed this issue!

@kofalt
Copy link

kofalt commented Aug 14, 2015

I was trying to figure it out myself; educated guess is that the phrase resolved spf13#71 in the linked PR's description is the culprit.

I think triggering ticket closure from a fork is utterly bananas, but that's github's fault not anyone here :)

@thezelus
Copy link

thezelus commented Jan 7, 2016

I am running into the exact same issue when trying to set up default values for nested keys. These values are being treated like top level keys.

@spf13 - Will this be supported by Viper?

@spf13
Copy link
Owner

spf13 commented Jan 18, 2016

I think this should be supported by Viper. There's a clear need for it as indicated by this thread. There are definitely some challenges in implementing it, but I believe that the addition would be worthwhile.

For me the big question is expressed by the following scenario...

The app has set a default
viper.SetDefault("datastore.metric.host", "0.0.0.0")

In a config file I provide a value like
datastore.metric.port = 80
or
datastore.metric = map{ port:80 }

In these two scenarios what should happen or more importantly what was the users intent?

If we can come up with a sane way to handle these scenarios, then I'm all for it.

@ti-mo
Copy link

ti-mo commented Jan 21, 2016

@spf13 - in the scenario you described, I would expect some action to happen on 0.0.0.0:80. The documentation states that Viper uses a strict precedence order, so it's a bit surprising that nested values aren't subject to the same rules.

Merging nested data structures while respecting precedence levels in mind w/ a feature toggle (that defaults to off?) would be perfect IMO.

@shaunduncan
Copy link

I think I may have run into this issue as well. For now, a workaround I am performing is by using MergeConfig with a string reader seems to get me up and running with nested default values:

defaults := `
    [foo]
    [foo.bar]
    baz = 1
`

config.MergeConfig(strings.NewReader(defaults))
config.MergeConfig(realConfigFile)

@spf13
Copy link
Owner

spf13 commented Feb 9, 2016

Forgive me. I've misunderstood this thread completely. The default should work completely as you've outlined. I've been reading this as setting things at nested values in any layer.

At all layers except for the default one those two lines are actually two very different operations. I'll comment on that challenge.

metric.port = 80 is updating a nested value. I would expect in this case that any existing values (other than port) under datastore.metric would remain

metric = map{ port:80 } is replacing a value. I would expect in this case that any existing values in metric would go way as I'm setting the entire value of metric to this new map. I wouldn't expect this to merge.

This is the big challenge here. I don't think a global setting is the solution because it's reasonable to expect that a user would want to use both operations (update and replace). We dealt with the same thing in MongoDB (https://docs.mongodb.org/manual/reference/method/db.collection.update/#update-specific-fields).

We need to figure out a way to handle this across all of the different layers and formats in a sane way.

I think the right solution is to follow the pattern above with replace and update operations handled as outlined there. I think it will be complicated to get it right with all the layers, but should work consistently and enables end users to replace entire parent values or update a key without touching siblings.

Does any of this make sense?

@AkihiroSuda
Copy link

+1

@kofalt
Copy link

kofalt commented Feb 18, 2016

SGTM 👍

@ti-mo
Copy link

ti-mo commented Feb 19, 2016

@spf13 That would be the best of both worlds 👍

@jgsqware
Copy link
Contributor

I've tried to do the workaround from here #71 (comment)

I made it work with this:

if c.Get("params.p3") == nil {
  c.Set("params.p3", "333")
}

@aarondl
Copy link

aarondl commented Jul 13, 2016

Seems like there's consensus around a fix for this issue. Anything holding back implementation other than time?

@benoitmasson
Copy link
Contributor

I believe my PR (#195) solves this issue. Feel free to try the code and to send me feedback!

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 a pull request may close this issue.