-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[receiver/awscloudwatch] Fix validation in some scenarios #14953
[receiver/awscloudwatch] Fix validation in some scenarios #14953
Conversation
Am I understanding correctly that |
Yes that is my understanding! I tried to think of other ways to solve this but this felt like the simplest. If there is a preferred way, I'm open to it! |
Ok, as proposed the config ends up in an ambiguous state where both The correct way to handle this is to implement a custom unmarshal function for the config. Basically, we can check if It should look pretty much like this (adapted from fileexporter): // Unmarshal a confmap.Conf into the config struct.
func (cfg *Config) Unmarshal(componentParser *confmap.Conf) error {
if componentParser == nil {
return errors.New("empty config for file exporter")
}
// first load the config normally
err := componentParser.Unmarshal(cfg, confmap.WithErrorUnused())
if err != nil {
return err
}
// check if user actually specified autodiscover, or if just autoinitialized
if !componentParser.IsSet("autodiscover") {
cfg.Autodiscover = nil
}
return nil
} |
@djaglowski that seemed to be a great solution, added a couple more tests to validate that these changes are safe and resolve the issue |
/easycla |
…etry#14953) * Fix validation of some awscloudwatch configs that used named groups
Description: Since the default config obtains an
autodiscover
config, the real config loading runs into issues more often than expected. We should remove the constriction so exclusive named config users can use the receiver.Link to tracking Issue: Resolves #14952
Testing: Existing tests and this is now a valid config:
Documentation: Updated documentation to reflect this change.