Skip to content

Commit

Permalink
Move feature flag related functions to config pkg
Browse files Browse the repository at this point in the history 
- 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.
  • Loading branch information
@chuangw6
chuangw6 committed Jul 8, 2022
1 parent dc9ea6c commit 263ba92
Show file tree
Hide file tree
Showing 7 changed files with 70 additions and 63 deletions.
27 changes: 27 additions & 0 deletions pkg/apis/config/feature_flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package config

import (
"context"
"fmt"
"os"
"strconv"
Expand Down Expand Up @@ -187,3 +188,29 @@ func setEmbeddedStatus(cfgMap map[string]string, defaultValue string, feature *s
func NewFeatureFlagsFromConfigMap(config *corev1.ConfigMap) (*FeatureFlags, error) {
return NewFeatureFlagsFromMap(config.Data)
}

// GetContextBasedOnFeatureFlag returns a context based on the feature flag
func GetContextBasedOnFeatureFlag(featureFlag string) context.Context {
featureFlags, _ := NewFeatureFlagsFromMap(map[string]string{
"enable-api-fields": featureFlag,
})
cfg := &Config{
FeatureFlags: featureFlags,
}

return ToContext(context.Background(), cfg)
}

// EnableAlphaAPIFields enables alpha feature in an existing context
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)
}
14 changes: 7 additions & 7 deletions pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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 {
Expand Down Expand Up @@ -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{{
Expand All @@ -170,7 +170,7 @@ func TestPipelineRef_Valid(t *testing.T) {
Name: "branch",
Value: "baz",
}}}},
wc: enableAlphaAPIFields,
wc: config.EnableAlphaAPIFields,
}}

for _, ts := range tests {
Expand Down
29 changes: 8 additions & 21 deletions pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ func TestPipelineRun_Validate(t *testing.T) {
},
},
},
wc: enableAlphaAPIFields,
wc: config.EnableAlphaAPIFields,
}}

for _, ts := range tests {
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand Down Expand Up @@ -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{
Expand All @@ -570,7 +570,7 @@ func TestPipelineRunSpec_Validate(t *testing.T) {
}},
}},
},
withContext: enableAlphaAPIFields,
withContext: config.EnableAlphaAPIFields,
}}

for _, ps := range tests {
Expand Down Expand Up @@ -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)
}
5 changes: 3 additions & 2 deletions pkg/apis/pipeline/v1beta1/result_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (

"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"
Expand Down Expand Up @@ -68,7 +69,7 @@ func TestResultsValidate(t *testing.T) {
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ctx := getContextBasedOnFeatureFlag(tt.apiFields)
ctx := config.GetContextBasedOnFeatureFlag(tt.apiFields)
if err := tt.Result.Validate(ctx); err != nil {
t.Errorf("TaskSpec.Validate() = %v", err)
}
Expand Down Expand Up @@ -133,7 +134,7 @@ func TestResultsValidateError(t *testing.T) {
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ctx := getContextBasedOnFeatureFlag(tt.apiFields)
ctx := config.GetContextBasedOnFeatureFlag(tt.apiFields)
err := tt.Result.Validate(ctx)
if err == nil {
t.Fatalf("Expected an error, got nothing for %v", tt.Result)
Expand Down
20 changes: 5 additions & 15 deletions pkg/apis/pipeline/v1beta1/task_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ func TestTaskSpecValidate(t *testing.T) {
Workspaces: tt.fields.Workspaces,
Results: tt.fields.Results,
}
ctx := getContextBasedOnFeatureFlag("alpha")
ctx := config.GetContextBasedOnFeatureFlag("alpha")
ts.SetDefaults(ctx)
if err := ts.Validate(ctx); err != nil {
t.Errorf("TaskSpec.Validate() = %v", err)
Expand Down Expand Up @@ -1279,7 +1279,7 @@ func TestTaskSpecValidateError(t *testing.T) {
Workspaces: tt.fields.Workspaces,
Results: tt.fields.Results,
}
ctx := getContextBasedOnFeatureFlag("alpha")
ctx := config.GetContextBasedOnFeatureFlag("alpha")
ts.SetDefaults(ctx)
err := ts.Validate(ctx)
if err == nil {
Expand Down Expand Up @@ -1326,7 +1326,7 @@ func TestStepAndSidecarWorkspaces(t *testing.T) {
Sidecars: tt.fields.Sidecars,
Workspaces: tt.fields.Workspaces,
}
ctx := getContextBasedOnFeatureFlag("alpha")
ctx := config.GetContextBasedOnFeatureFlag("alpha")
ts.SetDefaults(ctx)
if err := ts.Validate(ctx); err != nil {
t.Errorf("TaskSpec.Validate() = %v", err)
Expand Down Expand Up @@ -1383,7 +1383,7 @@ func TestStepAndSidecarWorkspacesErrors(t *testing.T) {
Sidecars: tt.fields.Sidecars,
}

ctx := getContextBasedOnFeatureFlag("alpha")
ctx := config.GetContextBasedOnFeatureFlag("alpha")
ts.SetDefaults(ctx)
err := ts.Validate(ctx)
if err == nil {
Expand Down Expand Up @@ -1503,7 +1503,7 @@ 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 := config.GetContextBasedOnFeatureFlag(version)

ts.SetDefaults(ctx)
err := ts.Validate(ctx)
Expand All @@ -1520,16 +1520,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
Expand Down
17 changes: 9 additions & 8 deletions pkg/apis/pipeline/v1beta1/taskref_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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{{
Expand All @@ -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) {
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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) {
Expand Down
Loading

0 comments on commit 263ba92

Please sign in to comment.