From 2aa5ac953a536ff62984ddb48a06b21c2002e2d6 Mon Sep 17 00:00:00 2001 From: pritidesai Date: Thu, 2 Feb 2023 09:33:09 -0800 Subject: [PATCH] array results and indexing into array promoted to beta Pipelines 0.38 introduced two alpha features - producing array results and indexing into array param. This commit is promoting these two features to beta such that these features can be used by the task authors and pipeline authors by setting enable-api-fields to either beta or alpha. These features are still not availalbe in stable API i.e. these features will not work with enable-api-fields set to stable Signed-off-by: pritidesai --- docs/pipelines.md | 21 +++++++------ docs/tasks.md | 2 +- ...ipelinerun-array-results-substitution.yaml | 0 .../pipelinerun-param-array-indexing.yaml | 0 pkg/apis/config/feature_flags.go | 11 +++++++ pkg/apis/config/feature_flags_test.go | 31 +++++++++++++++++++ pkg/apis/pipeline/v1/result_validation.go | 20 ++++++------ .../pipeline/v1/result_validation_test.go | 2 +- .../pipeline/v1beta1/result_validation.go | 20 ++++++------ .../v1beta1/result_validation_test.go | 14 ++++++++- pkg/reconciler/pipelinerun/resources/apply.go | 10 +++--- .../pipelinerun/resources/apply_test.go | 2 +- .../pipelinerun/resources/validate_params.go | 5 ++- .../resources/validate_params_test.go | 4 +-- pkg/reconciler/taskrun/resources/apply.go | 10 +++--- .../taskrun/resources/apply_test.go | 2 +- 16 files changed, 105 insertions(+), 49 deletions(-) rename examples/v1beta1/pipelineruns/{alpha => beta}/pipelinerun-array-results-substitution.yaml (100%) rename examples/v1beta1/pipelineruns/{alpha => beta}/pipelinerun-param-array-indexing.yaml (100%) diff --git a/docs/pipelines.md b/docs/pipelines.md index 909e9605f80..868c7e8aa98 100644 --- a/docs/pipelines.md +++ b/docs/pipelines.md @@ -943,17 +943,19 @@ Sharing `Results` between `Tasks` in a `Pipeline` happens via a `Result` and another receives it as a `Parameter` with a variable such as `$(tasks..results.)`. Pipeline support two new types of results and parameters: array `[]string` and object `map[string]string`. -Both are alpha features and can be enabled by setting `enable-api-fields` to `alpha`. +Array result is a beta feature and can be enabled by setting `enable-api-fields` to `beta` +and is also supported with `enable-api-fields` set to `alpha`. +Object result is an alpha feature and can be enabled by setting `enable-api-fields` to `alpha`. | Result Type | Parameter Type | Specification | `enable-api-fields` | -|-------------|----------------|--------------------------------------------------|-------------------| -| string | string | `$(tasks..results.)` | stable | -| array | array | `$(tasks..results.[*])` | alpha | -| array | string | `$(tasks..results.[i])` | alpha | -| object | object | `$(tasks..results.[*])` | alpha | -| object | string | `$(tasks..results..key)` | alpha | +|-------------|----------------|--------------------------------------------------|---------------------| +| string | string | `$(tasks..results.)` | stable | +| array | array | `$(tasks..results.[*])` | alpha or beta | +| array | string | `$(tasks..results.[i])` | alpha or beta | +| object | object | `$(tasks..results.[*])` | alpha | +| object | string | `$(tasks..results..key)` | alpha | -**Note:** Whole Array and Object `Results` (using star notation) cannot be referred in `script` and `args`. +**Note:** Whole Array and Object `Results` (using star notation) cannot be referred in `script`. **Note:** `Matrix` does not support `object` and `array` results. @@ -1040,7 +1042,8 @@ results: For an end-to-end example, see [`Results` in a `PipelineRun`](../examples/v1beta1/pipelineruns/pipelinerun-results.yaml). -Array and object results is supported as alpha feature, see [`Array Results` in a `PipelineRun`](../examples/v1beta1/pipelineruns/alpha/pipeline-emitting-results.yaml). +Object result is supported as alpha feature and array result is a beta feature, +see [`Array and Object Results` in a `PipelineRun`](../examples/v1beta1/pipelineruns/alpha/pipeline-emitting-results.yaml). ```yaml results: diff --git a/docs/tasks.md b/docs/tasks.md index 9ee19a97e63..9a521e5932d 100644 --- a/docs/tasks.md +++ b/docs/tasks.md @@ -834,7 +834,7 @@ or [at the `Pipeline` level](./pipelines.md#emitting-results-from-a-pipeline). #### Emitting Array `Results` Tekton Task also supports defining a result of type `array` and `object` in addition to `string`. -Emitting a task result of type `array` is an `alpha` feature implemented based on the +Emitting a task result of type `array` is a `beta` feature implemented based on the [TEP-0076](https://github.com/tektoncd/community/blob/main/teps/0076-array-result-types.md#emitting-array-results). You can initialize `array` results from a `task` using JSON escaped string, for example, to assign the following list of animals to an array result: diff --git a/examples/v1beta1/pipelineruns/alpha/pipelinerun-array-results-substitution.yaml b/examples/v1beta1/pipelineruns/beta/pipelinerun-array-results-substitution.yaml similarity index 100% rename from examples/v1beta1/pipelineruns/alpha/pipelinerun-array-results-substitution.yaml rename to examples/v1beta1/pipelineruns/beta/pipelinerun-array-results-substitution.yaml diff --git a/examples/v1beta1/pipelineruns/alpha/pipelinerun-param-array-indexing.yaml b/examples/v1beta1/pipelineruns/beta/pipelinerun-param-array-indexing.yaml similarity index 100% rename from examples/v1beta1/pipelineruns/alpha/pipelinerun-param-array-indexing.yaml rename to examples/v1beta1/pipelineruns/beta/pipelinerun-param-array-indexing.yaml diff --git a/pkg/apis/config/feature_flags.go b/pkg/apis/config/feature_flags.go index 199d4bc8732..305200b7fe9 100644 --- a/pkg/apis/config/feature_flags.go +++ b/pkg/apis/config/feature_flags.go @@ -323,6 +323,11 @@ func EnableBetaAPIFields(ctx context.Context) context.Context { return setEnableAPIFields(ctx, BetaAPIFields) } +// EnableStableAPIFields enables stable features in an existing context (for use in testing) +func EnableStableAPIFields(ctx context.Context) context.Context { + return setEnableAPIFields(ctx, StableAPIFields) +} + // CheckEnforceResourceVerificationMode returns true if the ResourceVerificationMode is EnforceResourceVerificationMode // else returns false func CheckEnforceResourceVerificationMode(ctx context.Context) bool { @@ -337,6 +342,12 @@ func CheckWarnResourceVerificationMode(ctx context.Context) bool { return cfg.FeatureFlags.ResourceVerificationMode == WarnResourceVerificationMode } +// CheckAlphaOrBetaAPIFields return true if the enable-api-fields is either set to alpha or set to beta +func CheckAlphaOrBetaAPIFields(ctx context.Context) bool { + cfg := FromContextOrDefaults(ctx) + return cfg.FeatureFlags.EnableAPIFields == AlphaAPIFields || cfg.FeatureFlags.EnableAPIFields == BetaAPIFields +} + func setEnableAPIFields(ctx context.Context, want string) context.Context { featureFlags, _ := NewFeatureFlagsFromMap(map[string]string{ "enable-api-fields": want, diff --git a/pkg/apis/config/feature_flags_test.go b/pkg/apis/config/feature_flags_test.go index 35c819854b0..2f85f14e98a 100644 --- a/pkg/apis/config/feature_flags_test.go +++ b/pkg/apis/config/feature_flags_test.go @@ -287,6 +287,37 @@ func TestCheckWarnResourceVerificationMode(t *testing.T) { } } +// TestCheckAlphaOrBetaAPIFields validates CheckAlphaOrBetaAPIFields for alpha, beta, and stable context +func TestCheckAlphaOrBetaAPIFields(t *testing.T) { + ctx := context.Background() + type testCase struct { + name string + c context.Context + expectedResult bool + } + testCases := []testCase{{ + name: "when enable-api-fields is set to alpha", + c: config.EnableAlphaAPIFields(ctx), + expectedResult: true, + }, { + name: "when enable-api-fields is set to beta", + c: config.EnableBetaAPIFields(ctx), + expectedResult: true, + }, { + name: "when enable-api-fields is set to stable", + c: config.EnableStableAPIFields(ctx), + expectedResult: false, + }} + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + e := config.CheckAlphaOrBetaAPIFields(tc.c) + if (tc.expectedResult && !e) || (!tc.expectedResult && e) { + t.Errorf("Failed to validate CheckAlphaOrBetaAPIFields for \"%v\", expected \"%v\" but got \"%v\"", tc.name, tc.expectedResult, e) + } + }) + } +} + func verifyConfigFileWithExpectedFeatureFlagsConfig(t *testing.T, fileName string, expectedConfig *config.FeatureFlags) { t.Helper() cm := test.ConfigMapFromTestFile(t, fileName) diff --git a/pkg/apis/pipeline/v1/result_validation.go b/pkg/apis/pipeline/v1/result_validation.go index 3ccb2907aaa..cdeca07cef3 100644 --- a/pkg/apis/pipeline/v1/result_validation.go +++ b/pkg/apis/pipeline/v1/result_validation.go @@ -33,21 +33,23 @@ func (tr TaskResult) Validate(ctx context.Context) (errs *apis.FieldError) { if !resultNameFormatRegex.MatchString(tr.Name) { return apis.ErrInvalidKeyName(tr.Name, "name", fmt.Sprintf("Name must consist of alphanumeric characters, '-', '_', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my-name', or 'my_name', regex used for validation is '%s')", ResultNameFormat)) } - // Array and Object are alpha features - if tr.Type == ResultsTypeArray || tr.Type == ResultsTypeObject { + + switch { + // Object are alpha features + case tr.Type == ResultsTypeObject: errs := validateObjectResult(tr) errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "results type", config.AlphaAPIFields)) return errs - } - + // Array results is a beta feature - check if the feature flag is set to "beta" or "alpha" + case tr.Type == ResultsTypeArray: + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "results type", config.BetaAPIFields)) + return errs // Resources created before the result. Type was introduced may not have Type set // and should be considered valid - if tr.Type == "" { + case tr.Type == "": return nil - } - - // By default the result type is string - if tr.Type != ResultsTypeString { + // By default, the result type is string + case tr.Type != ResultsTypeString: return apis.ErrInvalidValue(tr.Type, "type", "type must be string") } diff --git a/pkg/apis/pipeline/v1/result_validation_test.go b/pkg/apis/pipeline/v1/result_validation_test.go index 472d13851cc..9c2a6a2c4d9 100644 --- a/pkg/apis/pipeline/v1/result_validation_test.go +++ b/pkg/apis/pipeline/v1/result_validation_test.go @@ -122,7 +122,7 @@ func TestResultsValidateError(t *testing.T) { }, apiFields: "stable", expectedError: apis.FieldError{ - Message: "results type requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"", + Message: "results type requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\"", }, }, { name: "invalid object result type in stable", diff --git a/pkg/apis/pipeline/v1beta1/result_validation.go b/pkg/apis/pipeline/v1beta1/result_validation.go index 3e68bc44933..adda23e0b1d 100644 --- a/pkg/apis/pipeline/v1beta1/result_validation.go +++ b/pkg/apis/pipeline/v1beta1/result_validation.go @@ -27,21 +27,23 @@ func (tr TaskResult) Validate(ctx context.Context) (errs *apis.FieldError) { if !resultNameFormatRegex.MatchString(tr.Name) { return apis.ErrInvalidKeyName(tr.Name, "name", fmt.Sprintf("Name must consist of alphanumeric characters, '-', '_', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my-name', or 'my_name', regex used for validation is '%s')", ResultNameFormat)) } - // Array and Object is alpha feature - if tr.Type == ResultsTypeArray || tr.Type == ResultsTypeObject { + + switch { + // Object are alpha features + case tr.Type == ResultsTypeObject: errs := validateObjectResult(tr) errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "results type", config.AlphaAPIFields)) return errs - } - + // Array results is a beta feature - make sure the feature flag is set to "beta" + case tr.Type == ResultsTypeArray: + errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "results type", config.BetaAPIFields)) + return errs // Resources created before the result. Type was introduced may not have Type set // and should be considered valid - if tr.Type == "" { + case tr.Type == "": return nil - } - - // By default the result type is string - if tr.Type != ResultsTypeString { + // By default, the result type is string + case tr.Type != ResultsTypeString: return apis.ErrInvalidValue(tr.Type, "type", "type must be string") } diff --git a/pkg/apis/pipeline/v1beta1/result_validation_test.go b/pkg/apis/pipeline/v1beta1/result_validation_test.go index 566022d5646..682b3f64371 100644 --- a/pkg/apis/pipeline/v1beta1/result_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/result_validation_test.go @@ -58,6 +58,15 @@ func TestResultsValidate(t *testing.T) { }, apiFields: "alpha", + }, { + name: "valid result type array", + Result: v1beta1.TaskResult{ + Name: "MY-RESULT", + Type: v1beta1.ResultsTypeArray, + Description: "my great result", + }, + + apiFields: config.BetaAPIFields, }, { name: "valid result type object", Result: v1beta1.TaskResult{ @@ -74,6 +83,9 @@ func TestResultsValidate(t *testing.T) { if tt.apiFields == "alpha" { ctx = config.EnableAlphaAPIFields(ctx) } + if tt.apiFields == config.BetaAPIFields { + ctx = config.EnableBetaAPIFields(ctx) + } if err := tt.Result.Validate(ctx); err != nil { t.Errorf("TaskSpec.Validate() = %v", err) } @@ -122,7 +134,7 @@ func TestResultsValidateError(t *testing.T) { }, apiFields: "stable", expectedError: apis.FieldError{ - Message: "results type requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"", + Message: "results type requires \"enable-api-fields\" feature gate to be \"alpha\" or \"beta\" but it is \"stable\"", }, }, { name: "invalid object result type in stable", diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go index c43ec9e5dbf..f262a768b9f 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -56,7 +56,6 @@ func ApplyParameters(ctx context.Context, p *v1beta1.PipelineSpec, pr *v1beta1.P stringReplacements := map[string]string{} arrayReplacements := map[string][]string{} objectReplacements := map[string]map[string]string{} - cfg := config.FromContextOrDefaults(ctx) // Set all the default stringReplacements for _, p := range p.Params { @@ -64,8 +63,8 @@ func ApplyParameters(ctx context.Context, p *v1beta1.PipelineSpec, pr *v1beta1.P switch p.Default.Type { case v1beta1.ParamTypeArray: for _, pattern := range paramPatterns { - // array indexing for param is alpha feature - if cfg.FeatureFlags.EnableAPIFields == config.AlphaAPIFields { + // array indexing for param is beta feature - the feature flag can be either set to alpha or beta + if config.CheckAlphaOrBetaAPIFields(ctx) { for i := 0; i < len(p.Default.ArrayVal); i++ { stringReplacements[fmt.Sprintf(pattern+"[%d]", p.Name, i)] = p.Default.ArrayVal[i] } @@ -108,14 +107,13 @@ func paramsFromPipelineRun(ctx context.Context, pr *v1beta1.PipelineRun) (map[st stringReplacements := map[string]string{} arrayReplacements := map[string][]string{} objectReplacements := map[string]map[string]string{} - cfg := config.FromContextOrDefaults(ctx) for _, p := range pr.Spec.Params { switch p.Value.Type { case v1beta1.ParamTypeArray: for _, pattern := range paramPatterns { - // array indexing for param is alpha feature - if cfg.FeatureFlags.EnableAPIFields == config.AlphaAPIFields { + // array indexing for param is beta feature - the feature flag can be either set to alpha ot beta + if config.CheckAlphaOrBetaAPIFields(ctx) { for i := 0; i < len(p.Value.ArrayVal); i++ { stringReplacements[fmt.Sprintf(pattern+"[%d]", p.Name, i)] = p.Value.ArrayVal[i] } diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go index af3c0503318..0bdaa3c830a 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/pkg/reconciler/pipelinerun/resources/apply_test.go @@ -1728,7 +1728,7 @@ func TestApplyParameters(t *testing.T) { func TestApplyParameters_ArrayIndexing(t *testing.T) { ctx := context.Background() cfg := config.FromContextOrDefaults(ctx) - cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields + cfg.FeatureFlags.EnableAPIFields = config.BetaAPIFields ctx = config.ToContext(ctx, cfg) for _, tt := range []struct { name string diff --git a/pkg/reconciler/pipelinerun/resources/validate_params.go b/pkg/reconciler/pipelinerun/resources/validate_params.go index 363d18281c9..0b65d3683b9 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params.go +++ b/pkg/reconciler/pipelinerun/resources/validate_params.go @@ -89,10 +89,9 @@ func ValidateObjectParamRequiredKeys(pipelineParameters []v1beta1.ParamSpec, pip return nil } -// ValidateParamArrayIndex validate if the array indexing param reference target is existent +// ValidateParamArrayIndex validate if the array indexing param reference target is existent func ValidateParamArrayIndex(ctx context.Context, p *v1beta1.PipelineSpec, pr *v1beta1.PipelineRun) error { - cfg := config.FromContextOrDefaults(ctx) - if cfg.FeatureFlags.EnableAPIFields != config.AlphaAPIFields { + if !config.CheckAlphaOrBetaAPIFields(ctx) { return nil } diff --git a/pkg/reconciler/pipelinerun/resources/validate_params_test.go b/pkg/reconciler/pipelinerun/resources/validate_params_test.go index 27ae5f48479..685b06dd829 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params_test.go +++ b/pkg/reconciler/pipelinerun/resources/validate_params_test.go @@ -326,7 +326,7 @@ func TestValidateObjectParamRequiredKeys_Valid(t *testing.T) { func TestValidateParamArrayIndex_valid(t *testing.T) { ctx := context.Background() cfg := config.FromContextOrDefaults(ctx) - cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields + cfg.FeatureFlags.EnableAPIFields = config.BetaAPIFields ctx = config.ToContext(ctx, cfg) for _, tt := range []struct { name string @@ -508,7 +508,7 @@ func TestValidateParamArrayIndex_valid(t *testing.T) { func TestValidateParamArrayIndex_invalid(t *testing.T) { ctx := context.Background() cfg := config.FromContextOrDefaults(ctx) - cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields + cfg.FeatureFlags.EnableAPIFields = config.BetaAPIFields ctx = config.ToContext(ctx, cfg) for _, tt := range []struct { name string diff --git a/pkg/reconciler/taskrun/resources/apply.go b/pkg/reconciler/taskrun/resources/apply.go index 473fdb155d8..2291c58f032 100644 --- a/pkg/reconciler/taskrun/resources/apply.go +++ b/pkg/reconciler/taskrun/resources/apply.go @@ -56,7 +56,6 @@ func ApplyParameters(ctx context.Context, spec *v1beta1.TaskSpec, tr *v1beta1.Ta // that need to be further processed. stringReplacements := map[string]string{} arrayReplacements := map[string][]string{} - cfg := config.FromContextOrDefaults(ctx) // Set all the default stringReplacements for _, p := range defaults { @@ -64,8 +63,8 @@ func ApplyParameters(ctx context.Context, spec *v1beta1.TaskSpec, tr *v1beta1.Ta switch p.Default.Type { case v1beta1.ParamTypeArray: for _, pattern := range paramPatterns { - // array indexing for param is alpha feature - if cfg.FeatureFlags.EnableAPIFields == config.AlphaAPIFields { + // array indexing for param is beta feature + if config.CheckAlphaOrBetaAPIFields(ctx) { for i := 0; i < len(p.Default.ArrayVal); i++ { stringReplacements[fmt.Sprintf(pattern+"[%d]", p.Name, i)] = p.Default.ArrayVal[i] } @@ -100,14 +99,13 @@ func paramsFromTaskRun(ctx context.Context, tr *v1beta1.TaskRun) (map[string]str // that need to be further processed. stringReplacements := map[string]string{} arrayReplacements := map[string][]string{} - cfg := config.FromContextOrDefaults(ctx) for _, p := range tr.Spec.Params { switch p.Value.Type { case v1beta1.ParamTypeArray: for _, pattern := range paramPatterns { - // array indexing for param is alpha feature - if cfg.FeatureFlags.EnableAPIFields == config.AlphaAPIFields { + // array indexing for param is beta feature + if config.CheckAlphaOrBetaAPIFields(ctx) { for i := 0; i < len(p.Value.ArrayVal); i++ { stringReplacements[fmt.Sprintf(pattern+"[%d]", p.Name, i)] = p.Value.ArrayVal[i] } diff --git a/pkg/reconciler/taskrun/resources/apply_test.go b/pkg/reconciler/taskrun/resources/apply_test.go index 8ae622dde9d..01310f87a9d 100644 --- a/pkg/reconciler/taskrun/resources/apply_test.go +++ b/pkg/reconciler/taskrun/resources/apply_test.go @@ -1011,7 +1011,7 @@ func TestApplyParameters_ArrayIndexing(t *testing.T) { }) ctx := context.Background() cfg := config.FromContextOrDefaults(ctx) - cfg.FeatureFlags.EnableAPIFields = config.AlphaAPIFields + cfg.FeatureFlags.EnableAPIFields = config.BetaAPIFields ctx = config.ToContext(ctx, cfg) got := resources.ApplyParameters(ctx, simpleTaskSpecArrayIndexing, tr, dp...) if d := cmp.Diff(want, got); d != "" {