open-telemetry
/
opentelemetry-collector
mirror of https://github.com/open-telemetry/opentelemetry-collector.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[featuregate] Fix issue where StageDeprecated cause an error on register (#7586) 

* Update Deprecated to work like Stable but false

* Add changelog

* Fix lint

* Update README
This commit is contained in:
Tyler Helmuth 2023-05-09 18:06:42 -06:00 committed by GitHub
parent 8464b715a6
commit 9b978f74f3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 100 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
.chloggen/add-deprecated-stage-to-readme.yaml Executable file
View File

@ -0,0 +1,16 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix
# 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: Fix issue where `StageDeprecated` was not usable
# One or more tracking issues or pull requests related to the change
issues: [7586]
# (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:

12
featuregate/README.md
View File

@ -67,7 +67,11 @@ modeled after the [system used by Kubernetes](https://kubernetes.io/docs/referen
4. A `stable` feature gate will be removed in the version specified by its `ToVersion` value.
Features that prove unworkable in the `alpha` stage may be discontinued
without proceeding to the `beta` stage. Features that make it to the `beta`
stage will not be dropped and will eventually reach general availability
where the `Gate` that allowed them to be disabled during the `beta` stage
will be removed.
without proceeding to the `beta` stage. Instead, they will proceed to the
`deprecated` stage, which will feature is permanently disabled. A feature gate will
be removed once it has been `deprecated` for at least 2 releases of the collector.
Features that make it to the `beta` stage are intended to reach general availability but may still be discontinued.
If, after wider use, it is determined that the gate should be discontinued it will be reverted to the `alpha` stage
for 2 releases and then proceed to the `deprecated` stage. If instead it is ready for general availability it will
proceed to the `stable` stage.

62
featuregate/flag_test.go
View File

@ -32,83 +32,97 @@ func TestNewFlag(t *testing.T) {
{
name: "empty item",
input: "",
expected: map[string]bool{"alpha": false, "beta": true, "stable": true},
expectedStr: "-alpha,beta,stable",
expected: map[string]bool{"alpha": false, "beta": true, "deprecated": false, "stable": true},
expectedStr: "-alpha,beta,-deprecated,stable",
},
{
name: "simple enable alpha",
input: "alpha",
expected: map[string]bool{"alpha": true, "beta": true, "stable": true},
expectedStr: "alpha,beta,stable",
expected: map[string]bool{"alpha": true, "beta": true, "deprecated": false, "stable": true},
expectedStr: "alpha,beta,-deprecated,stable",
},
{
name: "plus enable alpha",
input: "+alpha",
expected: map[string]bool{"alpha": true, "beta": true, "stable": true},
expectedStr: "alpha,beta,stable",
expected: map[string]bool{"alpha": true, "beta": true, "deprecated": false, "stable": true},
expectedStr: "alpha,beta,-deprecated,stable",
},
{
name: "disabled beta",
input: "-beta",
expected: map[string]bool{"alpha": false, "beta": false, "stable": true},
expectedStr: "-alpha,-beta,stable",
expected: map[string]bool{"alpha": false, "beta": false, "deprecated": false, "stable": true},
expectedStr: "-alpha,-beta,-deprecated,stable",
},
{
name: "multiple items",
input: "-beta,alpha",
expected: map[string]bool{"alpha": true, "beta": false, "stable": true},
expectedStr: "alpha,-beta,stable",
expected: map[string]bool{"alpha": true, "beta": false, "deprecated": false, "stable": true},
expectedStr: "alpha,-beta,-deprecated,stable",
},
{
name: "multiple items with plus",
input: "-beta,+alpha",
expected: map[string]bool{"alpha": true, "beta": false, "stable": true},
expectedStr: "alpha,-beta,stable",
expected: map[string]bool{"alpha": true, "beta": false, "deprecated": false, "stable": true},
expectedStr: "alpha,-beta,-deprecated,stable",
},
{
name: "repeated items",
input: "alpha,-beta,-alpha",
expected: map[string]bool{"alpha": false, "beta": false, "stable": true},
expectedStr: "-alpha,-beta,stable",
expected: map[string]bool{"alpha": false, "beta": false, "deprecated": false, "stable": true},
expectedStr: "-alpha,-beta,-deprecated,stable",
},
{
name: "multiple plus items",
input: "+alpha,+beta",
expected: map[string]bool{"alpha": true, "beta": true, "stable": true},
expectedStr: "alpha,beta,stable",
expected: map[string]bool{"alpha": true, "beta": true, "deprecated": false, "stable": true},
expectedStr: "alpha,beta,-deprecated,stable",
},
{
name: "enable stable",
input: "stable",
expected: map[string]bool{"alpha": false, "beta": true, "stable": true},
expectedStr: "-alpha,beta,stable",
expected: map[string]bool{"alpha": false, "beta": true, "deprecated": false, "stable": true},
expectedStr: "-alpha,beta,-deprecated,stable",
},
{
name: "disable stable",
input: "-stable",
expectedSetErr: true,
expected: map[string]bool{"alpha": false, "beta": true, "stable": true},
expectedStr: "-alpha,beta,stable",
expected: map[string]bool{"alpha": false, "beta": true, "deprecated": false, "stable": true},
expectedStr: "-alpha,beta,-deprecated,stable",
},
{
name: "enable deprecated",
input: "deprecated",
expectedSetErr: true,
expected: map[string]bool{"alpha": false, "beta": true, "deprecated": false, "stable": true},
expectedStr: "-alpha,beta,-deprecated,stable",
},
{
name: "disable deprecated",
input: "-deprecated",
expected: map[string]bool{"alpha": false, "beta": true, "deprecated": false, "stable": true},
expectedStr: "-alpha,beta,-deprecated,stable",
},
{
name: "enable missing",
input: "missing",
expectedSetErr: true,
expected: map[string]bool{"alpha": false, "beta": true, "stable": true},
expectedStr: "-alpha,beta,stable",
expected: map[string]bool{"alpha": false, "beta": true, "deprecated": false, "stable": true},
expectedStr: "-alpha,beta,-deprecated,stable",
},
{
name: "disable missing",
input: "missing",
expectedSetErr: true,
expected: map[string]bool{"alpha": false, "beta": true, "stable": true},
expectedStr: "-alpha,beta,stable",
expected: map[string]bool{"alpha": false, "beta": true, "deprecated": false, "stable": true},
expectedStr: "-alpha,beta,-deprecated,stable",
},
} {
t.Run(tt.name, func(t *testing.T) {
reg := NewRegistry()
reg.MustRegister("alpha", StageAlpha)
reg.MustRegister("beta", StageBeta)
reg.MustRegister("deprecated", StageDeprecated, WithRegisterToVersion("1.0.0"))
reg.MustRegister("stable", StageStable, WithRegisterToVersion("1.0.0"))
v := NewFlag(reg)
if tt.expectedSetErr {

18
featuregate/registry.go
View File

@ -98,7 +98,7 @@ func (r *Registry) Register(id string, stage Stage, opts ...RegisterOption) (*Ga
opt.apply(g)
}
switch g.stage {
case StageAlpha:
case StageAlpha, StageDeprecated:
g.enabled = &atomic.Bool{}
case StageBeta, StageStable:
enabled := &atomic.Bool{}
@ -107,8 +107,8 @@ func (r *Registry) Register(id string, stage Stage, opts ...RegisterOption) (*Ga
default:
return nil, fmt.Errorf("unknown stage value %q for gate %q", stage, id)
}
if g.stage == StageStable && g.toVersion == "" {
return nil, fmt.Errorf("no removal version set for stable gate %q", id)
if (g.stage == StageStable || g.stage == StageDeprecated) && g.toVersion == "" {
return nil, fmt.Errorf("no removal version set for %v gate %q", g.stage.String(), id)
}
if _, loaded := r.gates.LoadOrStore(id, g); loaded {
return nil, fmt.Errorf("attempted to add pre-existing gate %q", id)
@ -123,13 +123,21 @@ func (r *Registry) Set(id string, enabled bool) error {
return fmt.Errorf("no such feature gate %q", id)
}
g := v.(*Gate)
if g.stage == StageStable {
switch g.stage {
case StageStable:
if !enabled {
return fmt.Errorf("feature gate %q is stable, can not be disabled", id)
}
fmt.Printf("Feature gate %q is stable and already enabled. It will be removed in version %v and continued use of the gate after version %v will result in an error.\n", id, g.toVersion, g.toVersion)
case StageDeprecated:
if enabled {
return fmt.Errorf("feature gate %q is deprecated, can not be enabled", id)
}
fmt.Printf("Feature gate %q is deprecated and already disabled. It will be removed in version %v and continued use of the gate after version %v will result in an error.\n", id, g.toVersion, g.toVersion)
default:
g.enabled.Store(enabled)
}
g.enabled.Store(enabled)
return nil
}

24
featuregate/registry_test.go
View File

@ -54,12 +54,20 @@ func TestRegistryApplyError(t *testing.T) {
r := NewRegistry()
assert.Error(t, r.Set("foo", true))
r.MustRegister("bar", StageAlpha)
assert.Error(t, r.Set("foo", true))
_, err := r.Register("foo", StageStable)
assert.Error(t, err)
assert.Error(t, r.Set("foo", true))
r.MustRegister("foo", StageStable, WithRegisterToVersion("next"))
assert.Error(t, r.Set("foo", false))
assert.Error(t, r.Set("deprecated", true))
_, err = r.Register("deprecated", StageDeprecated)
assert.Error(t, err)
assert.Error(t, r.Set("deprecated", true))
r.MustRegister("deprecated", StageDeprecated, WithRegisterToVersion("next"))
assert.Error(t, r.Set("deprecated", true))
}
func TestRegistryApply(t *testing.T) {
@ -115,6 +123,16 @@ func TestRegisterGateLifecycle(t *testing.T) {
enabled: true,
shouldErr: false,
},
{
name: "StageDeprecated Flag",
id: "test-gate",
stage: StageDeprecated,
opts: []RegisterOption{
WithRegisterToVersion("next"),
},
enabled: false,
shouldErr: false,
},
{
name: "Invalid stage",
id: "test-gate",
@ -127,6 +145,12 @@ func TestRegisterGateLifecycle(t *testing.T) {
stage: StageStable,
shouldErr: true,
},
{
name: "StageDeprecated gate missing removal version",
id: "test-gate",
stage: StageDeprecated,
shouldErr: true,
},
{
name: "Duplicate gate",
id: "existing-gate",

1
featuregate/stage_test.go
View File

@ -24,5 +24,6 @@ func TestStageString(t *testing.T) {
assert.Equal(t, "Alpha", StageAlpha.String())
assert.Equal(t, "Beta", StageBeta.String())
assert.Equal(t, "Stable", StageStable.String())
assert.Equal(t, "Deprecated", StageDeprecated.String())
assert.Equal(t, "Unknown", Stage(-1).String())
}