From a02aa9e48b08cc4bc95b13b3564d852e84b3a46f Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Tue, 24 Jan 2023 16:50:28 -0800 Subject: [PATCH] Deprecate Apply in favor of Set Trying to keep the API closer to some standard libraries like FlagsSet. If a getter is needed we should name it Lookup. Signed-off-by: Bogdan Drutu --- .chloggen/depapply.yaml | 12 ++++++++++++ featuregate/registry.go | 29 ++++++++++++++++++----------- featuregate/registry_test.go | 12 ++++++------ otelcol/collector_windows.go | 6 ++++-- otelcol/command.go | 6 ++++-- otelcol/config_test.go | 4 ++-- otelcol/otelcoltest/config_test.go | 4 ++-- 7 files changed, 48 insertions(+), 25 deletions(-) create mode 100755 .chloggen/depapply.yaml diff --git a/.chloggen/depapply.yaml b/.chloggen/depapply.yaml new file mode 100755 index 00000000000..4941e66972e --- /dev/null +++ b/.chloggen/depapply.yaml @@ -0,0 +1,12 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: deprecation + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: featuregate + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Deprecate Apply in favor of Set + +# One or more tracking issues or pull requests related to the change +issues: [7018] + diff --git a/featuregate/registry.go b/featuregate/registry.go index 3f696b86ba6..dc6c9864388 100644 --- a/featuregate/registry.go +++ b/featuregate/registry.go @@ -73,19 +73,12 @@ func WithRegisterRemovalVersion(version string) RegistryOption { }) } -// Apply a configuration in the form of a map of Gate identifiers to boolean values. -// Sets only those values provided in the map, other gate values are not changed. +// Deprecated: [v0.71.0] use Set. func (r *Registry) Apply(cfg map[string]bool) error { - for id, val := range cfg { - v, ok := r.gates.Load(id) - if !ok { - return fmt.Errorf("feature gate %s is unregistered", id) + for id, enabled := range cfg { + if err := r.Set(id, enabled); err != nil { + return err } - g := v.(*Gate) - if g.stage == StageStable { - return fmt.Errorf("feature gate %s is stable, can not be modified", id) - } - g.enabled.Store(val) } return nil } @@ -146,6 +139,20 @@ func (r *Registry) RegisterID(id string, stage Stage, opts ...RegistryOption) er return err } +// Set the enabled valued for a Gate identified by the given id. +func (r *Registry) Set(id string, enabled bool) error { + v, ok := r.gates.Load(id) + if !ok { + return fmt.Errorf("no such feature gate -%v", id) + } + g := v.(*Gate) + if g.stage == StageStable { + return fmt.Errorf("feature gate %s is stable, can not be modified", id) + } + g.enabled.Store(enabled) + return nil +} + // List returns a slice of copies of all registered Gates. func (r *Registry) List() []Gate { var ret []Gate diff --git a/featuregate/registry_test.go b/featuregate/registry_test.go index d00c0bfed3b..01de2b74ff2 100644 --- a/featuregate/registry_test.go +++ b/featuregate/registry_test.go @@ -28,7 +28,7 @@ func TestGlobalRegistry(t *testing.T) { func TestRegistry(t *testing.T) { r := NewRegistry() - id := "foo" + const id = "foo" assert.Empty(t, r.List()) @@ -37,7 +37,7 @@ func TestRegistry(t *testing.T) { assert.Len(t, r.List(), 1) assert.True(t, g.IsEnabled()) - assert.NoError(t, r.Apply(map[string]bool{id: false})) + assert.NoError(t, r.Set(id, false)) assert.False(t, g.IsEnabled()) _, err = r.Register(id, StageBeta) @@ -49,18 +49,18 @@ func TestRegistry(t *testing.T) { func TestRegistryApplyError(t *testing.T) { r := NewRegistry() - assert.Error(t, r.Apply(map[string]bool{"foo": true})) + assert.Error(t, r.Set("foo", true)) r.MustRegister("bar", StageAlpha) - assert.Error(t, r.Apply(map[string]bool{"foo": true})) + assert.Error(t, r.Set("foo", true)) r.MustRegister("foo", StageStable, WithRegisterRemovalVersion("next")) - assert.Error(t, r.Apply(map[string]bool{"foo": true})) + assert.Error(t, r.Set("foo", true)) } func TestRegistryApply(t *testing.T) { r := NewRegistry() fooGate := r.MustRegister("foo", StageAlpha, WithRegisterDescription("Test Gate")) assert.False(t, fooGate.IsEnabled()) - assert.NoError(t, r.Apply(map[string]bool{fooGate.ID(): true})) + assert.NoError(t, r.Set(fooGate.ID(), true)) assert.True(t, fooGate.IsEnabled()) } diff --git a/otelcol/collector_windows.go b/otelcol/collector_windows.go index dec8bdf6435..61daccb0299 100644 --- a/otelcol/collector_windows.go +++ b/otelcol/collector_windows.go @@ -94,8 +94,10 @@ func (s *windowsService) start(elog *eventlog.Log, colErrorChannel chan error) e return err } - if err := featuregate.GlobalRegistry().Apply(getFeatureGatesFlag(s.flags)); err != nil { - return err + for id, enabled := range getFeatureGatesFlag(flagSet) { + if err := featuregate.GlobalRegistry().Set(id, enabled); err != nil { + return err + } } var err error s.col, err = newWithWindowsEventLogCore(s.settings, s.flags, elog) diff --git a/otelcol/command.go b/otelcol/command.go index 207aa1088ec..ba2f350ac70 100644 --- a/otelcol/command.go +++ b/otelcol/command.go @@ -30,8 +30,10 @@ func NewCommand(set CollectorSettings) *cobra.Command { Version: set.BuildInfo.Version, SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) error { - if err := featuregate.GlobalRegistry().Apply(getFeatureGatesFlag(flagSet)); err != nil { - return err + for id, enabled := range getFeatureGatesFlag(flagSet) { + if err := featuregate.GlobalRegistry().Set(id, enabled); err != nil { + return err + } } if set.ConfigProvider == nil { var err error diff --git a/otelcol/config_test.go b/otelcol/config_test.go index ccd20df8430..e4c00e2bd88 100644 --- a/otelcol/config_test.go +++ b/otelcol/config_test.go @@ -255,9 +255,9 @@ func TestConfigValidate(t *testing.T) { }, } - require.NoError(t, featuregate.GlobalRegistry().Apply(map[string]bool{connectorsFeatureGate.ID(): true})) + require.NoError(t, featuregate.GlobalRegistry().Set(connectorsFeatureGate.ID(), true)) defer func() { - require.NoError(t, featuregate.GlobalRegistry().Apply(map[string]bool{connectorsFeatureGate.ID(): false})) + require.NoError(t, featuregate.GlobalRegistry().Set(connectorsFeatureGate.ID(), false)) }() for _, test := range testCases { t.Run(test.name, func(t *testing.T) { diff --git a/otelcol/otelcoltest/config_test.go b/otelcol/otelcoltest/config_test.go index 81a6b85d029..c1949db72db 100644 --- a/otelcol/otelcoltest/config_test.go +++ b/otelcol/otelcoltest/config_test.go @@ -71,9 +71,9 @@ func TestLoadConfigAndValidate(t *testing.T) { factories, err := NopFactories() assert.NoError(t, err) - require.NoError(t, featuregate.GlobalRegistry().Apply(map[string]bool{"otelcol.enableConnectors": true})) + require.NoError(t, featuregate.GlobalRegistry().Set("otelcol.enableConnectors", true)) defer func() { - require.NoError(t, featuregate.GlobalRegistry().Apply(map[string]bool{"otelcol.enableConnectors": false})) + require.NoError(t, featuregate.GlobalRegistry().Set("otelcol.enableConnectors", false)) }() cfgValidate, errValidate := LoadConfigAndValidate(filepath.Join("testdata", "config.yaml"), factories) require.NoError(t, errValidate)