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

WIP do not merge - optional config fields #10260

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .chloggen/confmap-optional.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: 'enhancement'

# 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: Introduce support for Optional type in configs

# One or more tracking issues or pull requests related to the change
issues: [10266]

# (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:

# 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: ['api']
2 changes: 1 addition & 1 deletion cmd/mdatagen/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/go-logr/logr v1.4.2 // indirect
github.com/go-logr/stdr v1.2.2 // indirect
github.com/go-viper/mapstructure/v2 v2.1.0 // indirect
github.com/go-viper/mapstructure/v2 v2.2.1 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/google/uuid v1.6.0 // indirect
github.com/inconshreveable/mousetrap v1.1.0 // indirect
Expand Down
4 changes: 2 additions & 2 deletions cmd/mdatagen/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cmd/mdatagen/internal/loader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func TestLoadMetadata(t *testing.T) {
component.StabilityLevelBeta: {"traces"},
component.StabilityLevelStable: {"metrics"},
},
Distributions: []string{},
Distributions: nil,
Codeowners: &Codeowners{
Active: []string{"dmitryax"},
},
Expand Down
2 changes: 1 addition & 1 deletion cmd/otelcorecol/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ require (
github.com/go-logr/logr v1.4.2 // indirect
github.com/go-logr/stdr v1.2.2 // indirect
github.com/go-ole/go-ole v1.2.6 // indirect
github.com/go-viper/mapstructure/v2 v2.1.0 // indirect
github.com/go-viper/mapstructure/v2 v2.2.1 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/golang/snappy v0.0.4 // indirect
github.com/google/uuid v1.6.0 // indirect
Expand Down
4 changes: 2 additions & 2 deletions cmd/otelcorecol/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion confmap/confmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ func decodeConfig(m *Conf, result any, errorUnused bool, skipTopLevelUnmarshaler
TagName: "mapstructure",
WeaklyTypedInput: false,
MatchName: caseSensitiveMatchName,
DecodeNil: true,
DecodeHook: mapstructure.ComposeDecodeHookFunc(
useExpandValue(),
expandNilStructPointersHookFunc(),
Expand Down Expand Up @@ -537,7 +538,9 @@ type Marshaler interface {
func zeroSliceHookFunc() mapstructure.DecodeHookFuncValue {
return func(from reflect.Value, to reflect.Value) (interface{}, error) {
if to.CanSet() && to.Kind() == reflect.Slice && from.Kind() == reflect.Slice {
to.Set(reflect.MakeSlice(to.Type(), from.Len(), from.Cap()))
if !from.IsNil() {
to.Set(reflect.MakeSlice(to.Type(), from.Len(), from.Cap()))
}
}

return from.Interface(), nil
Expand Down
12 changes: 6 additions & 6 deletions confmap/confmap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,10 @@ func TestExpandNilStructPointersHookFunc(t *testing.T) {
conf := NewFromStringMap(stringMap)
cfg := &TestConfig{}
assert.Nil(t, cfg.Struct)

require.NoError(t, conf.Unmarshal(cfg))
assert.Nil(t, cfg.Boolean)
// assert.False(t, *cfg.Boolean)
assert.Nil(t, cfg.Struct)
assert.Nil(t, cfg.Struct) // TODO does not match description of expandNilStructPointersHookFunc
assert.NotNil(t, cfg.MapStruct)
assert.Equal(t, &Struct{}, cfg.MapStruct["struct"])
}
Expand Down Expand Up @@ -153,6 +153,10 @@ func TestUnmarshalWithIgnoreUnused(t *testing.T) {
assert.NoError(t, conf.Unmarshal(&TestIDConfig{}, WithIgnoreUnused()))
}

type Struct struct {
Name string
}

type TestConfig struct {
Boolean *bool `mapstructure:"boolean"`
Struct *Struct `mapstructure:"struct"`
Expand All @@ -171,10 +175,6 @@ func (t TestConfig) Marshal(conf *Conf) error {
}))
}

type Struct struct {
Name string
}

type TestID string

func (tID *TestID) UnmarshalText(text []byte) error {
Expand Down
2 changes: 1 addition & 1 deletion confmap/converter/expandconverter/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ require (

require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/go-viper/mapstructure/v2 v2.1.0 // indirect
github.com/go-viper/mapstructure/v2 v2.2.1 // indirect
github.com/knadh/koanf/maps v0.1.1 // indirect
github.com/knadh/koanf/providers/confmap v0.1.0 // indirect
github.com/knadh/koanf/v2 v2.1.1 // indirect
Expand Down
4 changes: 2 additions & 2 deletions confmap/converter/expandconverter/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion confmap/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ module go.opentelemetry.io/collector/confmap
go 1.22.0

require (
github.com/go-viper/mapstructure/v2 v2.1.0
github.com/go-viper/mapstructure/v2 v2.2.1
github.com/knadh/koanf/maps v0.1.1
github.com/knadh/koanf/providers/confmap v0.1.0
github.com/knadh/koanf/v2 v2.1.1
Expand Down
4 changes: 2 additions & 2 deletions confmap/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion confmap/internal/e2e/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ require (

require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/go-viper/mapstructure/v2 v2.1.0 // indirect
github.com/go-viper/mapstructure/v2 v2.2.1 // indirect
github.com/knadh/koanf/maps v0.1.1 // indirect
github.com/knadh/koanf/providers/confmap v0.1.0 // indirect
github.com/knadh/koanf/v2 v2.1.1 // indirect
Expand Down
4 changes: 2 additions & 2 deletions confmap/internal/e2e/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

40 changes: 21 additions & 19 deletions confmap/internal/e2e/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ const (
)

type Test struct {
value string
targetField TargetField
expected any
resolveErr string
unmarshalErr string
value string
targetField TargetField
expected any
resolveErr string
unmarshalErrs []string
}

type TargetConfig[T any] struct {
Expand All @@ -54,8 +54,10 @@ func NewResolver(t testing.TB, path string) *confmap.Resolver {

func AssertExpectedMatch[T any](t *testing.T, tt Test, conf *confmap.Conf, cfg *TargetConfig[T]) {
err := conf.Unmarshal(cfg)
if tt.unmarshalErr != "" {
require.ErrorContains(t, err, tt.unmarshalErr)
if len(tt.unmarshalErrs) > 0 {
for _, unmarshalErr := range tt.unmarshalErrs {
require.ErrorContains(t, err, unmarshalErr)
}
return
}
require.NoError(t, err)
Expand Down Expand Up @@ -143,9 +145,9 @@ func TestStrictTypeCasting(t *testing.T) {
expected: "\"0123\"",
},
{
value: "\"0123\"",
targetField: TargetFieldInt,
unmarshalErr: "'field' expected type 'int', got unconvertible type 'string', value: '\"0123\"'",
value: "\"0123\"",
targetField: TargetFieldInt,
unmarshalErrs: []string{"'field' expected type 'int', got unconvertible type 'string', value: '\"0123\"'"},
},
{
value: "\"0123\"",
Expand All @@ -163,14 +165,14 @@ func TestStrictTypeCasting(t *testing.T) {
expected: "inline field with !!str 0123 expansion",
},
{
value: "t",
targetField: TargetFieldBool,
unmarshalErr: "'field' expected type 'bool', got unconvertible type 'string', value: 't'",
value: "t",
targetField: TargetFieldBool,
unmarshalErrs: []string{"'field' expected type", "got unconvertible type"},
},
{
value: "23",
targetField: TargetFieldBool,
unmarshalErr: "'field' expected type 'bool', got unconvertible type 'int', value: '23'",
value: "23",
targetField: TargetFieldBool,
unmarshalErrs: []string{"'field' expected type", "got unconvertible type"},
},
{
value: "{\"field\": 123}",
Expand Down Expand Up @@ -204,9 +206,9 @@ func TestStrictTypeCasting(t *testing.T) {
expected: true,
},
{
value: "true # comment with a ${env:hello.world} reference",
targetField: TargetFieldString,
unmarshalErr: `expected type 'string', got unconvertible type 'bool'`,
value: "true # comment with a ${env:hello.world} reference",
targetField: TargetFieldString,
unmarshalErrs: []string{"expected type", "got unconvertible type"},
},
{
value: "true # comment with a ${env:hello.world} reference",
Expand Down
62 changes: 62 additions & 0 deletions confmap/optional/optional.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

package optional // import "go.opentelemetry.io/collector/confmap/optional"

import "go.opentelemetry.io/collector/confmap"

// Optional is a type that can be used to represent a value that may or may not be present.
// It supports three flavors: Some(value), None(), and WithDefault(defaultValue).
type Optional[T any] struct {
hasValue bool
value T

defaultFn DefaultFunc[T]
}

type DefaultFunc[T any] func() T

var _ confmap.Unmarshaler = (*Optional[any])(nil)

// Some creates an Optional with a value.
func Some[T any](value T) Optional[T] {
return Optional[T]{value: value, hasValue: true}

Check warning on line 23 in confmap/optional/optional.go

View check run for this annotation

Codecov / codecov/patch

confmap/optional/optional.go#L22-L23

Added lines #L22 - L23 were not covered by tests
}

// None creates an Optional with no value.
func None[T any]() Optional[T] {
return Optional[T]{}
}

// WithDefault creates an Optional which has no value
// unless user config provides some, in which case
// the defaultFn is used to create the initial value,
// which may be overridden by the user provided config.
//
// The reason we pass a function instead of T directly
// is to allow this to work with T being a pointer,
// because we wouldn't want to copy the value of a pointer
// since it might reuse (and override) some shared state.
func WithDefault[T any](defaultFn DefaultFunc[T]) Optional[T] {
return Optional[T]{defaultFn: defaultFn}
}

func (o Optional[T]) HasValue() bool {
return o.hasValue
}

func (o Optional[T]) Value() T {
return o.value
}

func (o *Optional[T]) Unmarshal(conf *confmap.Conf) error {
if o.defaultFn != nil {
o.value = o.defaultFn()
o.hasValue = true
}
if err := conf.Unmarshal(&o.value); err != nil {
return err

Check warning on line 58 in confmap/optional/optional.go

View check run for this annotation

Codecov / codecov/patch

confmap/optional/optional.go#L58

Added line #L58 was not covered by tests
}
o.hasValue = true
return nil
}
Loading
Loading