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 Unmarshaler should not accept viper #2596

Closed
bogdandrutu opened this issue Mar 3, 2021 · 2 comments
Closed

Custom Unmarshaler should not accept viper #2596

bogdandrutu opened this issue Mar 3, 2021 · 2 comments
Assignees
Labels
area:config help wanted Good issue for contributors to OpenTelemetry Service to pick up

Comments

@bogdandrutu
Copy link
Member

Currently the custom unmarshaler accepts viper:

// ConfigUnmarshaler interface is an optional interface that if implemented by a Factory,
// the configuration loading system will use to unmarshal the config.
type ConfigUnmarshaler interface {
	// Unmarshal is a function that un-marshals a viper data into a config struct in a custom way.
	// componentViperSection *viper.Viper
	//   The config for this specific component. May be nil or empty if no config available.
	// intoCfg interface{}
	//   An empty interface wrapping a pointer to the config struct to unmarshal into.
	Unmarshal(componentViperSection *viper.Viper, intoCfg interface{}) error
}

It would be good to not expose viper directly but an interface{} or map[string]interface{} in our public API, so if viper breaks compatibility or moves to viper v2 (which is already planned) we can upgrade without breaking compatibility.

@bogdandrutu bogdandrutu added area:config good first issue Good for newcomers help wanted Good issue for contributors to OpenTelemetry Service to pick up labels Mar 3, 2021
@bogdandrutu bogdandrutu added this to the Phase1-GA-Roadmap milestone Mar 3, 2021
@bogdandrutu bogdandrutu removed the good first issue Good for newcomers label Mar 10, 2021
@mx-psi
Copy link
Member

mx-psi commented Mar 12, 2021

I would be happy to work on a PR for fixing this and #2597 next week. Could I be assigned this?

@bogdandrutu
Copy link
Member Author

This is done.

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this issue Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config help wanted Good issue for contributors to OpenTelemetry Service to pick up
Projects
None yet
Development

No branches or pull requests

2 participants