From df4d68b6fb951a79a433cce32277866e45d193f8 Mon Sep 17 00:00:00 2001 From: Chuang Wang Date: Fri, 8 Jul 2022 13:13:46 -0700 Subject: [PATCH] Move feature flag related functions to config pkg - getContextBasedOnFeatureFlag - enableAlphaAPIFields Those two feature flag related functions are defined in v1beta1_test packages but used across v1beta1 and v1beta1_test. Since we cannot import v1beta1_test from v1beta1, moving it to the config package will be a solution for it to be used across different packages. Also getContextBasedOnFeatureFlag is overlapping with enableAlphaAPIFields. So it's sufficient to only keep enableAlphaAPIFields. --- pkg/apis/config/feature_flags.go | 15 ++++++++++ .../v1beta1/pipelineref_validation_test.go | 14 ++++----- .../v1beta1/pipelinerun_validation_test.go | 29 +++++-------------- .../v1beta1/result_validation_test.go | 12 ++++++-- .../pipeline/v1beta1/task_validation_test.go | 23 +++++---------- .../v1beta1/taskref_validation_test.go | 17 ++++++----- .../v1beta1/taskrun_validation_test.go | 21 +++++++------- 7 files changed, 68 insertions(+), 63 deletions(-) diff --git a/pkg/apis/config/feature_flags.go b/pkg/apis/config/feature_flags.go index 17ef479ae61..6e12c143785 100644 --- a/pkg/apis/config/feature_flags.go +++ b/pkg/apis/config/feature_flags.go @@ -17,6 +17,7 @@ limitations under the License. package config import ( + "context" "fmt" "os" "strconv" @@ -187,3 +188,17 @@ func setEmbeddedStatus(cfgMap map[string]string, defaultValue string, feature *s func NewFeatureFlagsFromConfigMap(config *corev1.ConfigMap) (*FeatureFlags, error) { return NewFeatureFlagsFromMap(config.Data) } + +// EnableAlphaAPIFields enables alpha feature in an existing context (for use in testing) +func EnableAlphaAPIFields(ctx context.Context) context.Context { + featureFlags, _ := NewFeatureFlagsFromMap(map[string]string{ + "enable-api-fields": "alpha", + }) + cfg := &Config{ + Defaults: &Defaults{ + DefaultTimeoutMinutes: 60, + }, + FeatureFlags: featureFlags, + } + return ToContext(ctx, cfg) +} diff --git a/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go index 24101b4d0e2..f5ba9c9d8f3 100644 --- a/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go @@ -86,7 +86,7 @@ func TestPipelineRef_Invalid(t *testing.T) { }, }, wantErr: apis.ErrMissingField("resolver"), - withContext: enableAlphaAPIFields, + withContext: config.EnableAlphaAPIFields, }, { name: "pipelineref resolver disallowed in conjunction with pipelineref name", ref: &v1beta1.PipelineRef{ @@ -96,7 +96,7 @@ func TestPipelineRef_Invalid(t *testing.T) { }, }, wantErr: apis.ErrMultipleOneOf("name", "resolver"), - withContext: enableAlphaAPIFields, + withContext: config.EnableAlphaAPIFields, }, { name: "pipelineref resolver disallowed in conjunction with pipelineref bundle", ref: &v1beta1.PipelineRef{ @@ -106,7 +106,7 @@ func TestPipelineRef_Invalid(t *testing.T) { }, }, wantErr: apis.ErrMultipleOneOf("bundle", "resolver"), - withContext: enableAlphaAPIFields, + withContext: config.EnableAlphaAPIFields, }, { name: "pipelineref resource disallowed in conjunction with pipelineref name", ref: &v1beta1.PipelineRef{ @@ -119,7 +119,7 @@ func TestPipelineRef_Invalid(t *testing.T) { }, }, wantErr: apis.ErrMultipleOneOf("name", "resource").Also(apis.ErrMissingField("resolver")), - withContext: enableAlphaAPIFields, + withContext: config.EnableAlphaAPIFields, }, { name: "pipelineref resource disallowed in conjunction with pipelineref bundle", ref: &v1beta1.PipelineRef{ @@ -132,7 +132,7 @@ func TestPipelineRef_Invalid(t *testing.T) { }, }, wantErr: apis.ErrMultipleOneOf("bundle", "resource").Also(apis.ErrMissingField("resolver")), - withContext: enableAlphaAPIFields, + withContext: config.EnableAlphaAPIFields, }} for _, tc := range tests { @@ -160,7 +160,7 @@ func TestPipelineRef_Valid(t *testing.T) { }, { name: "alpha feature: valid resolver", ref: &v1beta1.PipelineRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}}, - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "alpha feature: valid resolver with resource parameters", ref: &v1beta1.PipelineRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git", Resource: []v1beta1.ResolverParam{{ @@ -170,7 +170,7 @@ func TestPipelineRef_Valid(t *testing.T) { Name: "branch", Value: "baz", }}}}, - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }} for _, ts := range tests { diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go index 48c93ba6c4b..d6eb2dcbd4f 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go @@ -289,7 +289,7 @@ func TestPipelineRun_Validate(t *testing.T) { }, }, }, - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }} for _, ts := range tests { @@ -378,7 +378,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { }, }, wantErr: apis.ErrMultipleOneOf("taskRunSpecs[0].stepOverrides[1].name"), - withContext: enableAlphaAPIFields, + withContext: config.EnableAlphaAPIFields, }, { name: "stepOverride disallowed without alpha feature gate", spec: v1beta1.PipelineRunSpec{ @@ -429,7 +429,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { }, }, wantErr: apis.ErrMissingField("taskRunSpecs[0].stepOverrides[0].name"), - withContext: enableAlphaAPIFields, + withContext: config.EnableAlphaAPIFields, }, { name: "duplicate sidecarOverride names", spec: v1beta1.PipelineRunSpec{ @@ -444,7 +444,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { }, }, wantErr: apis.ErrMultipleOneOf("taskRunSpecs[0].sidecarOverrides[1].name"), - withContext: enableAlphaAPIFields, + withContext: config.EnableAlphaAPIFields, }, { name: "missing sidecarOverride name", spec: v1beta1.PipelineRunSpec{ @@ -461,7 +461,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { }, }, wantErr: apis.ErrMissingField("taskRunSpecs[0].sidecarOverrides[0].name"), - withContext: enableAlphaAPIFields, + withContext: config.EnableAlphaAPIFields, }, { name: "invalid both step-level (stepOverrides.resources) and task-level (taskRunSpecs.resources) resource requirements configured", spec: v1beta1.PipelineRunSpec{ @@ -485,7 +485,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { "taskRunSpecs[0].stepOverrides.resources", "taskRunSpecs[0].computeResources", ), - withContext: enableAlphaAPIFields, + withContext: config.EnableAlphaAPIFields, }, { name: "computeResources disallowed without alpha feature gate", spec: v1beta1.PipelineRunSpec{ @@ -547,7 +547,7 @@ func TestPipelineRunSpec_Validate(t *testing.T) { }, }}, }, - withContext: enableAlphaAPIFields, + withContext: config.EnableAlphaAPIFields, }, { name: "valid sidecar and task-level (taskRunSpecs.resources) resource requirements configured", spec: v1beta1.PipelineRunSpec{ @@ -570,7 +570,7 @@ func TestPipelineRunSpec_Validate(t *testing.T) { }}, }}, }, - withContext: enableAlphaAPIFields, + withContext: config.EnableAlphaAPIFields, }} for _, ps := range tests { @@ -840,16 +840,3 @@ func TestPipelineRunWithTimeout_Validate(t *testing.T) { }) } } - -func enableAlphaAPIFields(ctx context.Context) context.Context { - featureFlags, _ := config.NewFeatureFlagsFromMap(map[string]string{ - "enable-api-fields": "alpha", - }) - cfg := &config.Config{ - Defaults: &config.Defaults{ - DefaultTimeoutMinutes: 60, - }, - FeatureFlags: featureFlags, - } - return config.ToContext(ctx, cfg) -} diff --git a/pkg/apis/pipeline/v1beta1/result_validation_test.go b/pkg/apis/pipeline/v1beta1/result_validation_test.go index 728e84dcfbe..87c60f04646 100644 --- a/pkg/apis/pipeline/v1beta1/result_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/result_validation_test.go @@ -17,10 +17,12 @@ limitations under the License. package v1beta1_test import ( + "context" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/test/diff" "knative.dev/pkg/apis" @@ -68,7 +70,10 @@ func TestResultsValidate(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := getContextBasedOnFeatureFlag(tt.apiFields) + ctx := context.Background() + if tt.apiFields == "alpha" { + ctx = config.EnableAlphaAPIFields(ctx) + } if err := tt.Result.Validate(ctx); err != nil { t.Errorf("TaskSpec.Validate() = %v", err) } @@ -133,7 +138,10 @@ func TestResultsValidateError(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := getContextBasedOnFeatureFlag(tt.apiFields) + ctx := context.Background() + if tt.apiFields == "alpha" { + ctx = config.EnableAlphaAPIFields(ctx) + } err := tt.Result.Validate(ctx) if err == nil { t.Fatalf("Expected an error, got nothing for %v", tt.Result) diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index e08433eb336..934e00f38c3 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -427,7 +427,7 @@ func TestTaskSpecValidate(t *testing.T) { Workspaces: tt.fields.Workspaces, Results: tt.fields.Results, } - ctx := getContextBasedOnFeatureFlag("alpha") + ctx := config.EnableAlphaAPIFields(context.Background()) ts.SetDefaults(ctx) if err := ts.Validate(ctx); err != nil { t.Errorf("TaskSpec.Validate() = %v", err) @@ -1279,7 +1279,7 @@ func TestTaskSpecValidateError(t *testing.T) { Workspaces: tt.fields.Workspaces, Results: tt.fields.Results, } - ctx := getContextBasedOnFeatureFlag("alpha") + ctx := config.EnableAlphaAPIFields(context.Background()) ts.SetDefaults(ctx) err := ts.Validate(ctx) if err == nil { @@ -1326,7 +1326,7 @@ func TestStepAndSidecarWorkspaces(t *testing.T) { Sidecars: tt.fields.Sidecars, Workspaces: tt.fields.Workspaces, } - ctx := getContextBasedOnFeatureFlag("alpha") + ctx := config.EnableAlphaAPIFields(context.Background()) ts.SetDefaults(ctx) if err := ts.Validate(ctx); err != nil { t.Errorf("TaskSpec.Validate() = %v", err) @@ -1383,7 +1383,7 @@ func TestStepAndSidecarWorkspacesErrors(t *testing.T) { Sidecars: tt.fields.Sidecars, } - ctx := getContextBasedOnFeatureFlag("alpha") + ctx := config.EnableAlphaAPIFields(context.Background()) ts.SetDefaults(ctx) err := ts.Validate(ctx) if err == nil { @@ -1503,7 +1503,10 @@ func TestIncompatibleAPIVersions(t *testing.T) { testName := fmt.Sprintf("(using %s) %s", version, tt.name) t.Run(testName, func(t *testing.T) { ts := tt.spec - ctx := getContextBasedOnFeatureFlag(version) + ctx := context.Background() + if version == "alpha" { + ctx = config.EnableAlphaAPIFields(ctx) + } ts.SetDefaults(ctx) err := ts.Validate(ctx) @@ -1520,16 +1523,6 @@ func TestIncompatibleAPIVersions(t *testing.T) { } } -func getContextBasedOnFeatureFlag(featureFlag string) context.Context { - featureFlags, _ := config.NewFeatureFlagsFromMap(map[string]string{ - "enable-api-fields": featureFlag, - }) - cfg := &config.Config{ - FeatureFlags: featureFlags, - } - - return config.ToContext(context.Background(), cfg) -} func TestSubstitutedContext(t *testing.T) { type fields struct { Params []v1beta1.ParamSpec diff --git a/pkg/apis/pipeline/v1beta1/taskref_validation_test.go b/pkg/apis/pipeline/v1beta1/taskref_validation_test.go index 9e0be193f2d..70efba6ddfb 100644 --- a/pkg/apis/pipeline/v1beta1/taskref_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/taskref_validation_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/test/diff" "knative.dev/pkg/apis" @@ -39,7 +40,7 @@ func TestTaskRef_Valid(t *testing.T) { }, { name: "alpha feature: valid resolver", taskRef: &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git"}}, - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "alpha feature: valid resolver with resource parameters", taskRef: &v1beta1.TaskRef{ResolverRef: v1beta1.ResolverRef{Resolver: "git", Resource: []v1beta1.ResolverParam{{ @@ -49,13 +50,13 @@ func TestTaskRef_Valid(t *testing.T) { Name: "branch", Value: "baz", }}}}, - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "valid bundle", taskRef: &v1beta1.TaskRef{ Name: "bundled-task", Bundle: "gcr.io/my-bundle"}, - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }} for _, ts := range tests { t.Run(ts.name, func(t *testing.T) { @@ -126,7 +127,7 @@ func TestTaskRef_Invalid(t *testing.T) { }, }, wantErr: apis.ErrMissingField("resolver"), - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "taskref resolver disallowed in conjunction with taskref name", taskRef: &v1beta1.TaskRef{ @@ -136,7 +137,7 @@ func TestTaskRef_Invalid(t *testing.T) { }, }, wantErr: apis.ErrMultipleOneOf("name", "resolver"), - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "taskref resolver disallowed in conjunction with taskref bundle", taskRef: &v1beta1.TaskRef{ @@ -146,7 +147,7 @@ func TestTaskRef_Invalid(t *testing.T) { }, }, wantErr: apis.ErrMultipleOneOf("bundle", "resolver"), - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "taskref resource disallowed in conjunction with taskref name", taskRef: &v1beta1.TaskRef{ @@ -159,7 +160,7 @@ func TestTaskRef_Invalid(t *testing.T) { }, }, wantErr: apis.ErrMultipleOneOf("name", "resource").Also(apis.ErrMissingField("resolver")), - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "taskref resource disallowed in conjunction with taskref bundle", taskRef: &v1beta1.TaskRef{ @@ -172,7 +173,7 @@ func TestTaskRef_Invalid(t *testing.T) { }, }, wantErr: apis.ErrMultipleOneOf("bundle", "resource").Also(apis.ErrMissingField("resolver")), - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }} for _, ts := range tests { t.Run(ts.name, func(t *testing.T) { diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go index ba73a248bc0..94c2392e367 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go @@ -22,6 +22,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" resource "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1" "github.com/tektoncd/pipeline/test/diff" @@ -83,7 +84,7 @@ func TestTaskRun_Validate(t *testing.T) { }}, }, }, - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }} for _, ts := range tests { t.Run(ts.name, func(t *testing.T) { @@ -242,7 +243,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { TaskRef: &v1beta1.TaskRef{Name: "mytask"}, }, wantErr: apis.ErrMultipleOneOf("params[myobjectparam].name"), - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "using debug when apifields stable", spec: v1beta1.TaskRunSpec{ @@ -265,7 +266,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { }, }, wantErr: apis.ErrInvalidValue("breakito is not a valid breakpoint. Available valid breakpoints include [onFailure]", "debug.breakpoint"), - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "stepOverride disallowed without alpha feature gate", spec: v1beta1.TaskRunSpec{ @@ -311,7 +312,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { }}, }, wantErr: apis.ErrMultipleOneOf("stepOverrides[1].name"), - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "missing stepOverride names", spec: v1beta1.TaskRunSpec{ @@ -323,7 +324,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { }}, }, wantErr: apis.ErrMissingField("stepOverrides[0].name"), - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "duplicate sidecarOverride names", spec: v1beta1.TaskRunSpec{ @@ -341,7 +342,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { }}, }, wantErr: apis.ErrMultipleOneOf("sidecarOverrides[1].name"), - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "missing sidecarOverride names", spec: v1beta1.TaskRunSpec{ @@ -353,7 +354,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { }}, }, wantErr: apis.ErrMissingField("sidecarOverrides[0].name"), - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "invalid both step-level (stepOverrides.resources) and task-level (spec.computeResources) resource requirements", spec: v1beta1.TaskRunSpec{ @@ -376,7 +377,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { "stepOverrides.resources", "computeResources", ), - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "computeResources disallowed without alpha feature gate", spec: v1beta1.TaskRunSpec{ @@ -468,7 +469,7 @@ func TestTaskRunSpec_Validate(t *testing.T) { }, }, }, - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }, { name: "valid sidecar and task-level (spec.resources) resource requirements", spec: v1beta1.TaskRunSpec{ @@ -487,7 +488,7 @@ func TestTaskRunSpec_Validate(t *testing.T) { }, }}, }, - wc: enableAlphaAPIFields, + wc: config.EnableAlphaAPIFields, }} for _, ts := range tests {