spf13 / viper

Go configuration with fangs
MIT License
26.86k stars 2.01k forks source link

`svType != tvType` during config merge when keys can have different types #1142

Closed Envek closed 2 years ago

Envek commented 3 years ago

In Lefhook git hooks manager we had ability to skip some git hooks in user's local config. That was done via skip boolean setting in the config where usually it wasn't specified in main config defaulting to false, but in local config user could set it to true. However, reverse situations also exists in the wild.

Recently we decided to allow users not only skip hooks always (true) or never (false) but also by more complex conditions (e.g. during rebase and merge). This means, that this setting now can have different types of values in different configs: either boolean, string, or list of strings.

However, when we tried to merge configs with these “multitype” setting, values were ignored. I'm not sure whether it is a bug in Viper (I couldn't find any reasoning for the current implementation), but at least surprising behavior. If there is a way to properly handle this situation without changing Viper, please let us know!

Expected behavior (what you expected to happen): When I merge two configs with keys of different types, values are being overriden by values from merging config.

Actual behavior (what actually happened): New values are ignored with following messages printed to stderr:

ERROR 2021/05/22 12:16:28 svType != tvType; key=skip, st=string, tt=bool, sv=rebase, tv=true
ERROR 2021/05/22 12:16:28 svType != tvType; key=skip, st=[]interface {}, tt=bool, sv=[rebase merge], tv=true

Repl.it link: https://replit.com/@Envek/Viper-example#main.go

Code reproducing the issue:

viper := viper.New()
viper.AddConfigPath(".")

viper.SetConfigName("lefthook")
_ = viper.ReadInConfig()

viper.SetConfigName("lefthook-local")
_ = viper.MergeInConfig()

fmt.Printf("\nAll configuration: %+v\n\n", viper.AllSettings())

Configuration files:

lefthook.yml ```yml post-checkout: piped: true scripts: 01-bundle-checkinstall: skip: true 02-db-migrate: skip: true 03-crystalball-update: skip: true ```
lefthook-local.yml ```yml post-checkout: scripts: 01-bundle-checkinstall: skip: false 02-db-migrate: skip: rebase 03-crystalball-update: skip: - rebase - merge ```

Environment:

Anything else we should know?: Pull request in Lefthook where this behavior was discovered (more context can be found here): https://github.com/evilmartians/lefthook/pull/173 Similar issue in Viper: https://github.com/spf13/viper/issues/1134 Commit that introduces code of interest: 991d18a

And thank you for Viper!

github-actions[bot] commented 3 years ago

👋 Thanks for reporting!

A maintainer will take a look at your issue shortly. 👀

In the meantime: We are working on Viper v2 and we would love to hear your thoughts about what you like or don't like about Viper, so we can improve or fix those issues.

⏰ If you have a couple minutes, please take some time and share your thoughts: https://forms.gle/R6faU74qPRPAzchZ9

📣 If you've already given us your feedback, you can still help by spreading the news, either by sharing the above link or telling people about this on Twitter:

https://twitter.com/sagikazarmark/status/1306904078967074816

Thank you! ❤️

sagikazarmark commented 2 years ago

@Envek I've tested the patch that I've just merged into master and it seems to be working with your repl.