From 6155cc25ddaad253abcced4fcd8108d1be98e9c1 Mon Sep 17 00:00:00 2001 From: Antoine Toulme Date: Fri, 22 Dec 2023 11:11:39 -0800 Subject: [PATCH] Make the option `WithErrorUnused` enabled by default when unmarshaling configuration (#9154) **Description:** Make the option `WithErrorUnused` enabled by default when unmarshaling configuration The option `WithErrorUnused` is now enabled by default, and a new option `WithIgnoreUnused` is introduced to ignore errors about unused fields. **Link to tracking Issue:** This relates to #7102 to some extent. **Testing:** N/A **Documentation:** N/A --- .chloggen/make_error_unused_default.yaml | 27 +++++++++++++++++++ component/config.go | 2 +- confmap/confmap.go | 18 ++++++++++--- confmap/confmap_test.go | 3 ++- exporter/loggingexporter/config.go | 2 +- otelcol/internal/configunmarshaler/configs.go | 2 +- otelcol/unmarshaler.go | 2 +- otelcol/unmarshaler_test.go | 4 +-- receiver/otlpreceiver/config.go | 2 +- 9 files changed, 50 insertions(+), 12 deletions(-) create mode 100755 .chloggen/make_error_unused_default.yaml diff --git a/.chloggen/make_error_unused_default.yaml b/.chloggen/make_error_unused_default.yaml new file mode 100755 index 00000000000..d646877ea5b --- /dev/null +++ b/.chloggen/make_error_unused_default.yaml @@ -0,0 +1,27 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: breaking + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: confmap + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Make the option `WithErrorUnused` enabled by default when unmarshaling configuration + +# One or more tracking issues or pull requests related to the change +issues: [7102] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: | + The option `WithErrorUnused` is now enabled by default, and a new option `WithIgnoreUnused` is introduced to ignore + errors about unused fields. + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] \ No newline at end of file diff --git a/component/config.go b/component/config.go index e46ead72f06..f34f04b8db7 100644 --- a/component/config.go +++ b/component/config.go @@ -32,7 +32,7 @@ func UnmarshalConfig(conf *confmap.Conf, intoCfg Config) error { return cu.Unmarshal(conf) } - return conf.Unmarshal(intoCfg, confmap.WithErrorUnused()) + return conf.Unmarshal(intoCfg) } // ConfigValidator defines an optional interface for configurations to implement to do validation. diff --git a/confmap/confmap.go b/confmap/confmap.go index 254f0813433..905e42eecbe 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -51,15 +51,25 @@ type UnmarshalOption interface { } type unmarshalOption struct { - errorUnused bool + ignoreUnused bool } // WithErrorUnused sets an option to error when there are existing // keys in the original Conf that were unused in the decoding process -// (extra keys). +// (extra keys). This option is enabled by default and can be disabled with `WithIgnoreUnused`. +// Deprecated: this is now enabled by default. Use `WithIgnoreUnused` to disable. func WithErrorUnused() UnmarshalOption { return unmarshalOptionFunc(func(uo *unmarshalOption) { - uo.errorUnused = true + uo.ignoreUnused = false + }) +} + +// WithIgnoreUnused sets an option to ignore errors if existing +// keys in the original Conf were unused in the decoding process +// (extra keys). +func WithIgnoreUnused() UnmarshalOption { + return unmarshalOptionFunc(func(uo *unmarshalOption) { + uo.ignoreUnused = true }) } @@ -76,7 +86,7 @@ func (l *Conf) Unmarshal(result any, opts ...UnmarshalOption) error { for _, opt := range opts { opt.apply(&set) } - return decodeConfig(l, result, set.errorUnused) + return decodeConfig(l, result, !set.ignoreUnused) } type marshalOption struct{} diff --git a/confmap/confmap_test.go b/confmap/confmap_test.go index 211eca429aa..7cfab3fb4cb 100644 --- a/confmap/confmap_test.go +++ b/confmap/confmap_test.go @@ -149,7 +149,8 @@ func TestUnmarshalWithErrorUnused(t *testing.T) { "string": "this is a string", } conf := NewFromStringMap(stringMap) - assert.Error(t, conf.Unmarshal(&TestIDConfig{}, WithErrorUnused())) + assert.Error(t, conf.Unmarshal(&TestIDConfig{})) + assert.NoError(t, conf.Unmarshal(&TestIDConfig{}, WithIgnoreUnused())) } type TestConfig struct { diff --git a/exporter/loggingexporter/config.go b/exporter/loggingexporter/config.go index 469286b111d..d35cdd6fa4d 100644 --- a/exporter/loggingexporter/config.go +++ b/exporter/loggingexporter/config.go @@ -65,7 +65,7 @@ func (cfg *Config) Unmarshal(conf *confmap.Conf) error { return fmt.Errorf("'loglevel' and 'verbosity' are incompatible. Use only 'verbosity' instead") } - if err := conf.Unmarshal(cfg, confmap.WithErrorUnused()); err != nil { + if err := conf.Unmarshal(cfg); err != nil { return err } diff --git a/otelcol/internal/configunmarshaler/configs.go b/otelcol/internal/configunmarshaler/configs.go index 83996d5f015..ab226d3b670 100644 --- a/otelcol/internal/configunmarshaler/configs.go +++ b/otelcol/internal/configunmarshaler/configs.go @@ -23,7 +23,7 @@ func NewConfigs[F component.Factory](factories map[component.Type]F) *Configs[F] func (c *Configs[F]) Unmarshal(conf *confmap.Conf) error { rawCfgs := make(map[component.ID]map[string]any) - if err := conf.Unmarshal(&rawCfgs, confmap.WithErrorUnused()); err != nil { + if err := conf.Unmarshal(&rawCfgs); err != nil { return err } diff --git a/otelcol/unmarshaler.go b/otelcol/unmarshaler.go index 66b2a87a0c6..643e396db42 100644 --- a/otelcol/unmarshaler.go +++ b/otelcol/unmarshaler.go @@ -66,5 +66,5 @@ func unmarshal(v *confmap.Conf, factories Factories) (*configSettings, error) { }, } - return cfg, v.Unmarshal(&cfg, confmap.WithErrorUnused()) + return cfg, v.Unmarshal(&cfg) } diff --git a/otelcol/unmarshaler_test.go b/otelcol/unmarshaler_test.go index 1e7881a8a0d..18fd5f631db 100644 --- a/otelcol/unmarshaler_test.go +++ b/otelcol/unmarshaler_test.go @@ -135,7 +135,7 @@ func TestPipelineConfigUnmarshalError(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { pips := new(pipelines.Config) - err := tt.conf.Unmarshal(&pips, confmap.WithErrorUnused()) + err := tt.conf.Unmarshal(&pips) require.Error(t, err) assert.Contains(t, err.Error(), tt.expectError) }) @@ -203,7 +203,7 @@ func TestServiceUnmarshalError(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { - err := tt.conf.Unmarshal(&service.Config{}, confmap.WithErrorUnused()) + err := tt.conf.Unmarshal(&service.Config{}) require.Error(t, err) assert.Contains(t, err.Error(), tt.expectError) }) diff --git a/receiver/otlpreceiver/config.go b/receiver/otlpreceiver/config.go index 2234e2532d9..1f3ff5d7e68 100644 --- a/receiver/otlpreceiver/config.go +++ b/receiver/otlpreceiver/config.go @@ -60,7 +60,7 @@ func (cfg *Config) Validate() error { // Unmarshal a confmap.Conf into the config struct. func (cfg *Config) Unmarshal(conf *confmap.Conf) error { // first load the config normally - err := conf.Unmarshal(cfg, confmap.WithErrorUnused()) + err := conf.Unmarshal(cfg) if err != nil { return err }