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

Custom yaml unmarshaling (e.g. defaults) #26

Open
bmoylan opened this issue Jan 20, 2019 · 3 comments
Open

Custom yaml unmarshaling (e.g. defaults) #26

bmoylan opened this issue Jan 20, 2019 · 3 comments

Comments

@bmoylan
Copy link
Contributor

bmoylan commented Jan 20, 2019

witchcraft calls yaml.Unmarshal for you, but this makes custom handling like loading defaults difficult.

@bmoylan
Copy link
Contributor Author

bmoylan commented Jan 20, 2019

FYSA @ashrayjain

@nmiyake
Copy link
Contributor

nmiyake commented Jan 21, 2019

Thought about this further after our in-person discussion. I think the decision here stems on how opinionated we want to be in terms of how we deal with configuration.

As you point out in this issue, right now witchcraft is opinionated in its use of yaml.Unmarshal from go-yaml/yaml (aka gopkg.in/yaml.v2) to perform config unmarshaling. It's probably not unreasonable to allow this function to be configured by allowing the user to specify a custom func([]byte, interface{}) error used in all places where config is unmarshaled (and default the value to yaml.Unmarshal). This would allow the use of other YAML libraries/custom functionality as outlined here. However, because this generically plugs the unmarshal, it would also allow clients to unmarshal config as JSON or whatever else they want as well. Probably not a huge issue, but just worth flagging that allowing configuration of the Unmarshal behavior does technically allow a lot more to be configured. We would probably need to caveat that, in general, witchcraft configuration will be written in a manner that assumes the behavior of yaml.Unmarshal, so if the user provides their own implementation they are potentially responsible for supporting all of its behavior (tags such as ,inline, etc.).

If we decide that the usage of yaml.Unmarshal is one of the opinions of this library (which is also somewhat the case based on the default tags format we use for base config), then I don't think we should allow this to be configured. In this scenario, it's still possible to perform custom operations like setting/loading defaults for config, but it has to be structured as part of the yaml.Unmarshal or as a specialized config byte provider.

Right now I'm leaning slightly towards the first approach, but @bmoylan curious to hear your thoughts/opinion.

@nmiyake
Copy link
Contributor

nmiyake commented Jan 22, 2019

Concretely, if there's some function like structfields.SetToDefaults (analogous to https://github.com/creasty/defaults), then the approaches described above would be:

WithConfigUnmarshalFunction(func(data []byte, v interface{}) error {
	if err := structfields.SetToDefaults(v); err != nil {
		return err
	}
	return yaml.Unmarshal(data, v)
})

vs. implementing something like:

func (cfg *AppInstallConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
	if err := structfields.SetToDefaults(cfg); err != nil {
		return err
	}
	type RawAppInstallConfig AppInstallConfig
	rawCfg := RawAppInstallConfig(*cfg)
	if err := unmarshal(&rawCfg); err != nil {
		return err
	}
	*cfg = AppInstallConfig(rawCfg)
	return nil
}

For the install and runtime structs.

nmiyake added a commit that referenced this issue Mar 6, 2019
nmiyake added a commit that referenced this issue Mar 6, 2019
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

No branches or pull requests

2 participants