Add a new Unset function to remove a key from viper.

[i02sopop]

In some circunstances, you want to remove a key for your config in viper, so, instead of setting the key value to nil, it's better to have a new Unset function to remove the key from the inner map.

Since this is my first PR to the project, please let me know if I need to do something else.

[CLAassistant]

All committers have signed the CLA.

[anjannath]

What is holding this from getting merged? :-/

[i02sopop]

Should I do anything else to get this PR merged?

[gjrtimmer]

@spf13 When will this be merged ?

[aca]

I want to load secrets from environment variable, (I believe It is general way to set credentials)
and use "WriteConfig" without saving this secrets to file.
There should be a way to unset value.
Is there anything wrong with this approach?

[Erog38]

@spf13 Is there any chance of seeing this in Viper mainstream anytime soon?

[makew0rld]

@sagikazarmark Thanks for maintaining Viper! Could this be merged?

[nodox]

@spf13 have you stopped accepting PRs for this feature?

[d1ngd0]

Has this gone stale? Does anything need to be done to merge this in. I am currently using viper as a way of storing dynamic config, and not being able to delete is a blocker. If anything needs to be done here I would be willing to do it.

[makew0rld]

Well the maintainers have never commented, so I think they just never looked at it. No idea if it's stale, but Viper hasn't changed a lot so probably not.

[i02sopop]

If in two and a half years he has not reviewed this PR I guess he's not interested in accepting contributions, so I'd suggest forking the repo if you want the change. I'm not using Viper anymore, that's why I'm not forking it myself :).

[sagikazarmark]

Personally, I don't think Unset fits into Viper, especially because "unsetting" a value is not that trivial and this PR only takes manually set values into account. How about config values from other sources? Even if you don't need that someone might and it leads to a whole lot of confusion.

Viper is mainly for collecting config from various sources and providing a reader interface for it. There are probably workarounds that can work without an Unset.

So I'd say, this won't land in Viper, at least for now (until we figure out a way forward for Viper: https://github.com/spf13/viper/projects/1).

[spf13]

I agree with Mark. I think this is outside of the normal behavior and scope. If we are wrong, provide a compelling and common use case to why unset is necessary.

…

On Wed, Dec 16, 2020 at 1:51 PM Márk Sági-Kazár ***@***.***> wrote: Personally, I don't think Unset fits into Viper, especially because "unsetting" a value is not that trivial and this PR only takes manually set values into account. How about config values from other sources? Even if you don't need that someone might and it leads to a whole lot of confusion. Viper is mainly for collecting config from various sources and providing a reader interface for it. There are probably workarounds that can work without an Unset. So I'd say, this won't land in Viper, at least for now (until we figure out a way forward for Viper: https://github.com/spf13/viper/projects/1). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#519 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABKKZBU7OR4GPHKVGEDXZDSVD6SNANCNFSM4FHDRGNA> .

[Vortelf]

@spf13 I have a use case but it's not with unsetting from v.override but from v.config
I'm changing the structure of the config file and want to update the config file without leaving leftover configurations.
Old structure

#Old structure
dockerTimeout: 30s
alwaysPull: true
#New Structure
docker:
  timeout: 30s
  alwaysPull: true

Configuration Structure

type Configuration struct {
	Dockercfg Dockercfg            `mapstructure:"docker"`
}
type Dockercfg struct {
	Timeout time.Duration `mapstructure:"timeout"`
        AlwaysPull bool       `mapstructure:"alwayspull"`
}

I can easily check if the old configs are set and reassign their value to the new keys. The issue is when I want to save the file - the old config keys are still present.
That is why I've written an Unset function to clean the config from the unnecessary old keys.

// Unset a key from config loaded from file
func Unset(key string) {v.Unset(key)}
func (v *Viper) Unset(key string) {
	path := strings.Split(v.realKey(strings.ToLower(key)), v.keyDelim)
	lastKey := strings.ToLower(path[len(path)-1])
	configMap := deepSearch(v.config, path[0:len(path)-1])

	delete(configMap, lastKey)
}

[Rolice]

Hello, there is indeed interest in such functionality. @i02sopop, it seems that you haven't signed the license clause that you are freely contributing to this project. Then the authors may merge it.

[i02sopop]

@Rolice Done. I've also rebased with master.

[Vortelf]

@i02sopop You should at least also deepSearch(v.config, path[0:len(path)-1]) and delete from there. Ideally unset from every possible map and clear alias.

	config         map[string]interface{}
	override       map[string]interface{}
	defaults       map[string]interface{}
	kvstore        map[string]interface{}

[i02sopop]

@Vortelf Done. I've also added an additional test.

[coreybutler]

@spf13 - the compelling reason I can think of is output control. The argument you cited suggests viper is mainly for reading configs and providing a reader interface. While it may "mainly" focus on reads, it ships with functions like WriteConfig. As of now, developers can control what is read into an app, but not what's written out. That feels incomplete. An unset operation is one way to provide developers control over the output, such as cleaning up runtime flags/configurations that shouldn't exist when WriteConfig is executed.

If unset is really that objectionable, perhaps alternative/additional writing methods would be more appropriate. Off the top of my head, something like:

func (v *Viper) WriteConfig(ignoredAttributes ...string) error {
  ....
}

or

func (v *Viper) WriteConfigIgnore(ignoredAttributes []string) error {
  ....
}

or some variation.

My real world use case: I have a CLI app that will leverage a config file by default. It allows an alternative configuration path to be supplied as a flag. When a command runs this way and saves modified data to the config, it always has a config: "./myconfig.yml" in the output, which is completely unnecessary. There are also config values which are nil/empty by default. These shouldn't be in the output either, but they are always written as property: "". Again, these extraneous values shouldn't be in the output after WriteConfig is executed, but there is no way to control that.

[spf13]

I agree it should be handled on the write functionality side. Let's give some thought to how and then send a PR when ready.

…

On Thu, Jun 10, 2021 at 12:34 AM Corey Butler ***@***.***> wrote: @spf13 <https://github.com/spf13> - the compelling reason I can think of is output control. The argument you cited suggests viper is mainly for reading configs and providing a reader interface. While it may "mainly" focus on reads, it ships with functions like WriteConfig. As of now, developers can control what is read into an app, but not what's written out. That feels incomplete. An unset operation is one way to provide developers control over the output, such as cleaning up runtime flags/configurations that shouldn't exist when WriteConfig is executed. If unset is really that objectionable, perhaps alternative/additional writing methods would be more appropriate. Off the top of my head, something like: func (v *Viper) WriteConfig(ignoredAttributes ...string) error { .... } or func (v *Viper) WriteConfigIgnore(ignoredAttributes []string) error { .... } or some variation. My real world use case: I have a CLI app that will leverage a config file by default. It allows an alternative configuration path to be supplied as a flag. When a command runs this way and saves modified data to the config, it always has a config: "./myconfig.yml" in the output, which is completely unnecessary. There are also config values which are nil/empty by default. These shouldn't be in the output either, but they are always written as property: "". Again, these extraneous values shouldn't be in the output after WriteConfig is executed, but there is no way to control that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#519 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABKKZBIJVWMMAXBR5IDKDDTSA6ETANCNFSM4FHDRGNA> .

[sagikazarmark]

I still think using WriteConfig to "save" configuration is a mistake, exactly because the configuration comes from multiple different sources and you probably don't want to output all of them. Adding Unset will not make things better in terms of scaring people away from using WriteConfig.

As for controlling output: you can get every key using AllSettings and AllKeys. You can use those to filter your output and write the result to wherever you need it.

In Viper v2, I'd like to deprecate every writing method.

[spf13]

This makes the most sense to me. Fetching the settings into a data structure that you can then manipulate and save as you see fit.

…

On Thu, Jun 10, 2021 at 8:02 AM Márk Sági-Kazár ***@***.***> wrote: I still think using WriteConfig to "save" configuration is a mistake, exactly because the configuration comes from multiple different sources and you probably don't want to output all of them. Adding Unset will not make things better in terms of scaring people away from using WriteConfig. As for controlling output: you can get every key using AllSettings and AllKeys. You can use those to filter your output and write the result to wherever you need it. In Viper v2, I'd like to deprecate every writing method. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#519 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABKKZESDFMAM7MYG7PFRQTTSCSXHANCNFSM4FHDRGNA> .

[coreybutler]

If the write methods will ultimately be removed, I'd recommend providing an example for implementing the soon to be deprecated functionality, either in the new README or the wiki. Otherwise this will continue to be a question.

FWIW, I implemented a custom write method using the following:

func Save(ignoredkeys ...string) error {
	file := viper.ConfigFileUsed()
	if len(file) == 0 {
		file = "./default.yml"
	}

	configMap := viper.AllSettings()
	for _, key := range ignoredkeys {
		delete(configMap, key)
	}

	content, err := yaml.Marshal(configMap)
	if err != nil {
		return err
	}

	fs.WriteTextFile(file, string(content))

	return nil
}

For anyone wanting to implement this on their own today, fs.WriteTextFile is just part of a simple wrapper lib I commonly use (see source). It could be replaced with something more generic like ioutil.WriteFile(path, []byte(content), perm).

[williambrode]

I just want to point out that while @coreybutler 's Save function is a helpful tool - it doesn't actually remove the ignored keys from the runtime viper object. To do that you'd probably recreate the it from the config file. But as pointed out, this all gets really ugly really fast because it wasn't designed for this purpose. Better to just choose in your app when to write things directly to a config file (without viper), and then only use viper for runtime config. But someone can correct me if I'm wrong.

[i02sopop]

@spf13 I'm not sure if this PR is relevant anymore, I've stopped using Viper some time ago, but if you're interested in this functionality I can implement any suggestion you have. If not, I'm happy to close the PR.

[Rolice]

Hey, thanks for answering. I think it would be helpful to include this functionality, it does not modify existing behavior but provides extra one that can be used on-demand.

[mateusz-lisik]

Six years, come on guys

[haaawk]

Sad to learn that Unset is not available and probably never will. Just hit a need for it too.