check beta feature flag for v1 TaskSpec's ValidateParamArrayIndex #6613

Yongxuanzhang · 2023-05-02T20:31:32Z

Changes

This commit closes #6607. TaskSpec's member function ValidateParamArrayIndex checks alpha feature flag, but array indexing is promoted to beta feature and we should check beta flag instead. This commit fixes this issue and update doc to explicitly include array indexing as beta feature.

/kind bug

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

check beta feature flag for v1 TaskSpec's ValidateParamArrayIndex instead of alpha flag, since array indexing is beta feature

tekton-robot · 2023-05-02T20:37:59Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/task_validation.go	96.9%	96.9%	-0.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.0%	-0.0

tekton-robot · 2023-05-02T20:39:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/task_validation.go	96.9%	96.9%	-0.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.0%	-0.0

lbernick · 2023-05-02T20:42:50Z

pkg/apis/pipeline/v1beta1/task_validation.go

@@ -612,8 +612,7 @@ func isParamRefs(s string) bool {
 // - `trParams` are params from taskrun.
 // - `taskSpec` contains params declarations.
 func (ts *TaskSpec) ValidateParamArrayIndex(ctx context.Context, params Params) error {
-	cfg := config.FromContextOrDefaults(ctx)
-	if cfg.FeatureFlags.EnableAPIFields != config.AlphaAPIFields {
+	if !config.CheckAlphaOrBetaAPIFields(ctx) {


This should be removed, since setting "stable" for v1beta1 also enables beta features

Oh 🤯
how about v1? Should we remove it from v1?

In v1 we do check for beta API fields, so it makes sense to keep in v1

lbernick · 2023-05-02T20:43:07Z

/approve cancel

lbernick · 2023-05-03T14:39:32Z

I think we should consider whether this function should return an error, instead of nil, if a task is using a feature where "enable-api-fields" is set to the wrong value. This is basically saying that if a task is using array param indexing and the index is out of bounds, this won't return an error if "enable-api-fields" is beta.

tekton-robot · 2023-05-03T14:43:37Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/task_validation.go	96.9%	96.9%	-0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.4%	0.3
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.3%	0.3

tekton-robot · 2023-05-03T14:45:26Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/task_validation.go	96.9%	96.9%	-0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.4%	0.3
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.3%	0.3

Yongxuanzhang · 2023-05-03T14:52:29Z

I think we should consider whether this function should return an error, instead of nil, if a task is using a feature where "enable-api-fields" is set to the wrong value.

Can we set "enable-api-fields" to random value? I think this function can validate it

pipeline/pkg/apis/config/feature_flags.go

Lines 201 to 213 in 09d422c

    
           func setEnabledAPIFields(cfgMap map[string]string, defaultValue string, feature *string) error { 
        
           	value := defaultValue 
        
           	if cfg, ok := cfgMap[enableAPIFields]; ok { 
        
           		value = strings.ToLower(cfg) 
        
           	} 
        
           	switch value { 
        
           	case AlphaAPIFields, BetaAPIFields, StableAPIFields: 
        
           		*feature = value 
        
           	default: 
        
           		return fmt.Errorf("invalid value for feature flag %q: %q", enableAPIFields, value) 
        
           	} 
        
           	return nil 
        
           }

This is basically saying that if a task is using array param indexing and the index is out of bounds, this won't return an error if "enable-api-fields" is beta.

Did you mean v1 or v1beta1 task?

tekton-robot · 2023-05-03T15:09:33Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/task_validation.go	96.9%	96.9%	-0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.4%	0.3
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.3%	0.3

tekton-robot · 2023-05-03T15:10:05Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/task_validation.go	96.9%	96.9%	-0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.4%	0.3
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.3%	0.3

lbernick · 2023-05-09T16:33:33Z

@Yongxuanzhang based on @jerop's feedback on #6617 let's merge your PR first. I think you should remove the reconciler checks for alpha/beta flags, since with this change it would be possible to create a v1beta1 task with array indexing but then have it not work correctly in the reconciler. Please also pull in the tests TestPipelineWithBetaFields and TestTaskWithBetaFields from #6617.

We'll also need a release note indicating the bug fix of not requiring the "beta" feature flag for array indexing in beta tasks/pipelines.

lbernick · 2023-05-09T17:41:15Z

@Yongxuanzhang can you also add array indexing to the list of beta features?

Yongxuanzhang · 2023-05-09T18:03:03Z

@Yongxuanzhang can you also add array indexing to the list of beta features?

I think array indexing is already in the list(it's part of tep-76), what's missing is the tep-75 object param and results maybe I can open another small pr to update it?

This commit closes tektoncd#6607. V1 TaskSpec's member function ValidateParamArrayIndex checks alpha feature flag, but array indexing is promoted to beta feature and we should check beta flag instead. And for v1beta1 the beta is on by default so we don't check the beta flag. This commit fixes this issue and also update doc. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

tekton-robot · 2023-05-09T18:22:13Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/task_validation.go	96.9%	96.9%	-0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.4%	0.3
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.3%	0.3

tekton-robot · 2023-05-09T18:22:28Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/task_validation.go	96.9%	96.9%	-0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.1%	99.4%	0.3
pkg/apis/pipeline/v1beta1/task_validation.go	97.0%	97.3%	0.3

jerop

/lgtm

tekton-robot · 2023-05-09T18:51:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop, lbernick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jerop,lbernick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jerop · 2023-05-09T18:52:02Z

/test pull-tekton-pipeline-build-tests

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 2, 2023

tekton-robot requested review from lbernick and pritidesai May 2, 2023 20:31

lbernick approved these changes May 2, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 2, 2023

lbernick reviewed May 2, 2023

View reviewed changes

tekton-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 2, 2023

Yongxuanzhang force-pushed the fix-6607 branch from e1461c0 to fadc2ba Compare May 3, 2023 14:38

Yongxuanzhang changed the title ~~check beta feature flag for TaskSpec's ValidateParamArrayIndex~~ check beta feature flag for v1 TaskSpec's ValidateParamArrayIndex May 3, 2023

Yongxuanzhang force-pushed the fix-6607 branch from fadc2ba to 234cf51 Compare May 3, 2023 15:02

lbernick mentioned this pull request May 9, 2023

Cleanup: Move array indexing validation out of apis package #6617

Merged

4 tasks

Yongxuanzhang force-pushed the fix-6607 branch from 234cf51 to ae9c053 Compare May 9, 2023 18:13

Yongxuanzhang requested a review from lbernick May 9, 2023 18:14

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels May 9, 2023

lbernick approved these changes May 9, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 9, 2023

jerop approved these changes May 9, 2023

View reviewed changes

tekton-robot assigned jerop May 9, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 9, 2023

tekton-robot merged commit 1df3fc5 into tektoncd:main May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check beta feature flag for v1 TaskSpec's ValidateParamArrayIndex #6613

check beta feature flag for v1 TaskSpec's ValidateParamArrayIndex #6613

Yongxuanzhang commented May 2, 2023 •

edited

Loading

tekton-robot commented May 2, 2023

tekton-robot commented May 2, 2023

lbernick May 2, 2023

Yongxuanzhang May 2, 2023

lbernick May 3, 2023

Yongxuanzhang May 3, 2023

lbernick commented May 2, 2023

lbernick commented May 3, 2023

tekton-robot commented May 3, 2023

tekton-robot commented May 3, 2023

Yongxuanzhang commented May 3, 2023

tekton-robot commented May 3, 2023

tekton-robot commented May 3, 2023

lbernick commented May 9, 2023 •

edited

Loading

lbernick commented May 9, 2023

Yongxuanzhang commented May 9, 2023

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

jerop left a comment

tekton-robot commented May 9, 2023

jerop commented May 9, 2023

check beta feature flag for v1 TaskSpec's ValidateParamArrayIndex #6613

check beta feature flag for v1 TaskSpec's ValidateParamArrayIndex #6613

Conversation

Yongxuanzhang commented May 2, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 2, 2023

tekton-robot commented May 2, 2023

lbernick May 2, 2023

Choose a reason for hiding this comment

Yongxuanzhang May 2, 2023

Choose a reason for hiding this comment

lbernick May 3, 2023

Choose a reason for hiding this comment

Yongxuanzhang May 3, 2023

Choose a reason for hiding this comment

lbernick commented May 2, 2023

lbernick commented May 3, 2023

tekton-robot commented May 3, 2023

tekton-robot commented May 3, 2023

Yongxuanzhang commented May 3, 2023

tekton-robot commented May 3, 2023

tekton-robot commented May 3, 2023

lbernick commented May 9, 2023 • edited Loading

lbernick commented May 9, 2023

Yongxuanzhang commented May 9, 2023

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented May 9, 2023

jerop commented May 9, 2023

Yongxuanzhang commented May 2, 2023 •

edited

Loading

lbernick commented May 9, 2023 •

edited

Loading