[TEP-0076] Add indexing into array for pipeline params reference #4855

Yongxuanzhang · 2022-05-11T13:50:38Z

Changes

This commit provides the indexing into array for params for pipeline level reference and gated by
alpha feature flag. Before this commit we can only refer to the whole
array param, with this feature we can refer to array's element such as
$(params.param-name[i]).

/kind feature

Submitter Checklist

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

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in
(if there are no user facing changes, use release note "NONE")

Release Notes

Indexing into array for pipeline params is now an alpha feature, element of array params can be accessed via $(params.param-name[i]).

tekton-robot · 2022-05-11T13:53:19Z

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/reconciler/taskrun/resources/apply.go	99.2%	99.2%	0.0

tekton-robot · 2022-05-11T14:13:09Z

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/reconciler/taskrun/resources/apply.go	99.2%	99.2%	0.0

tekton-robot · 2022-05-11T14:20:29Z

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/reconciler/taskrun/resources/apply.go	99.2%	99.2%	0.0

tekton-robot · 2022-05-11T14:21:13Z

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/reconciler/taskrun/resources/apply.go	99.2%	99.2%	0.0

Yongxuanzhang · 2022-05-11T14:34:33Z

/assign @ywluogg
Realized that this is independent but also part of tep0076

tekton-robot · 2022-05-11T14:34:35Z

@Yongxuanzhang: GitHub didn't allow me to assign the following users: ywluogg.

Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @ywluogg
Realized that this is independent but also part of tep0076

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Yongxuanzhang · 2022-05-11T14:37:58Z

/test pull-tekton-pipeline-build-tests

ywluogg · 2022-05-11T14:42:10Z

/assign ywluogg

docs/variables.md

pkg/reconciler/pipelinerun/resources/apply.go

ywluogg · 2022-05-16T14:24:24Z

pkg/reconciler/taskrun/resources/apply_test.go

@@ -222,6 +222,180 @@ var (
 		},
 	}

+	simpleTaskSpecArrayIndexing = &v1beta1.TaskSpec{


Are you planning to add those invalid indexing cases in this PR? Something like $(params['FOO'][2]) $(params['FOO'][-1])

I was hesitant to add the validation because I thought it is tricky.
Right now for these out of bound cases there will be no errors, and they will be treated like string.
I will think about how to add them in this PR.

SG! If you don't plan to add it in this PR, you can add a TODO and do it later, but we definitely need those tests for coverage and guards.

how hard would it be to add this validation? I think silently failing without errors can be confusing for users

It's bit complicated, let me see if there's an easy solution

Can we keep the validation in another PR? The release deadline is close

I agree with Dibyo, I wouldn't want to hold off on adding validation just to get the feature out the door. Can you explain more about what the challenge is here?

tekton-robot · 2022-05-16T15:28:36Z

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/reconciler/taskrun/resources/apply.go	99.2%	99.2%	0.0

tekton-robot · 2022-05-16T15:43:24Z

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/reconciler/taskrun/resources/apply.go	99.2%	99.2%	0.0

examples/v1beta1/taskruns/alpha/param_array_indexing.yaml

ywluogg

Except for the current open comments, others LGTM

tekton-robot · 2022-05-17T17:17:38Z

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/reconciler/taskrun/resources/apply.go	99.2%	99.2%	0.0

tekton-robot · 2022-05-17T19:31:30Z

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/reconciler/taskrun/resources/apply.go	99.2%	99.2%	0.0

ywluogg · 2022-06-02T18:18:47Z

Link this to #4723

tekton-robot · 2022-07-11T20:24:19Z

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/reconciler/pipelinerun/pipelinerun.go	86.2%	85.9%	-0.3
pkg/reconciler/pipelinerun/resources/apply.go	99.2%	99.3%	0.0
pkg/reconciler/pipelinerun/resources/validate_params.go	100.0%	98.8%	-1.2
pkg/substitution/substitution.go	64.9%	64.1%	-0.8

Yongxuanzhang · 2022-07-11T20:34:50Z

pkg/reconciler/pipelinerun/resources/validate_params_test.go

+	}
+}
+
+func TestValidateParamArrayIndex_invalid(t *testing.T) {


@lbernick is this the test case?

pkg/reconciler/pipelinerun/resources/validate_params_test.go

pkg/reconciler/pipelinerun/resources/validate_params.go

pkg/substitution/substitution.go

tekton-robot · 2022-07-11T20:37:14Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [lbernick]

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

tekton-robot · 2022-07-12T02:31:45Z

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/reconciler/pipelinerun/pipelinerun.go	86.2%	85.9%	-0.3
pkg/reconciler/pipelinerun/resources/apply.go	99.2%	99.3%	0.0
pkg/reconciler/pipelinerun/resources/validate_params.go	100.0%	98.8%	-1.2
pkg/substitution/substitution.go	64.9%	63.6%	-1.4

tekton-robot · 2022-07-12T02:44:22Z

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/reconciler/pipelinerun/pipelinerun.go	86.2%	85.9%	-0.3
pkg/reconciler/pipelinerun/resources/apply.go	99.2%	99.3%	0.0
pkg/reconciler/pipelinerun/resources/validate_params.go	100.0%	98.8%	-1.2
pkg/substitution/substitution.go	64.9%	63.6%	-1.4

tekton-robot · 2022-07-12T13:46:57Z

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/reconciler/pipelinerun/pipelinerun.go	86.2%	85.9%	-0.3
pkg/reconciler/pipelinerun/resources/apply.go	99.2%	99.3%	0.0
pkg/reconciler/pipelinerun/resources/validate_params.go	100.0%	98.8%	-1.2
pkg/substitution/substitution.go	60.7%	59.5%	-1.1

Yongxuanzhang · 2022-07-12T14:30:32Z

/retest

tekton-robot · 2022-07-12T16:01:44Z

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/reconciler/pipelinerun/pipelinerun.go	86.2%	85.9%	-0.3
pkg/reconciler/pipelinerun/resources/apply.go	99.2%	99.3%	0.0
pkg/reconciler/pipelinerun/resources/validate_params.go	100.0%	98.8%	-1.2
pkg/substitution/substitution.go	60.7%	59.5%	-1.1

Yongxuanzhang · 2022-07-12T16:41:19Z

/retest

tekton-robot · 2022-07-13T17:39:32Z

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/reconciler/pipelinerun/pipelinerun.go	86.3%	86.1%	-0.3
pkg/reconciler/pipelinerun/resources/apply.go	99.2%	99.3%	0.0
pkg/reconciler/pipelinerun/resources/validate_params.go	100.0%	98.7%	-1.3
pkg/substitution/substitution.go	60.7%	59.5%	-1.1

This commit provides the indexing into array for params and gated by alpha feature flag. Before this commit we can only refer to the whole array param, with this feature we can refer to array's element such as $(params.param-name[0]).

tekton-robot · 2022-07-13T19:05:09Z

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/reconciler/pipelinerun/pipelinerun.go	86.3%	86.1%	-0.3
pkg/reconciler/pipelinerun/resources/apply.go	99.2%	99.3%	0.0
pkg/reconciler/pipelinerun/resources/validate_params.go	100.0%	98.7%	-1.3
pkg/substitution/substitution.go	60.7%	61.9%	1.2

dibyom · 2022-07-13T20:28:48Z

Thanks @Yongxuanzhang
/lgtm

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 11, 2022

tekton-robot requested review from pritidesai and vdemeester May 11, 2022 13:50

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 11, 2022

Yongxuanzhang force-pushed the param-array-indexing branch from bfb2f90 to c17a358 Compare May 11, 2022 14:17

tekton-robot assigned ywluogg May 11, 2022

Yongxuanzhang marked this pull request as ready for review May 11, 2022 15:17

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2022

ywluogg reviewed May 16, 2022

View reviewed changes

Yongxuanzhang force-pushed the param-array-indexing branch from c17a358 to 7da92fa Compare May 16, 2022 14:44

Yongxuanzhang force-pushed the param-array-indexing branch from 7da92fa to 36d5a6e Compare May 16, 2022 15:38

ywluogg reviewed May 16, 2022

View reviewed changes

examples/v1beta1/taskruns/alpha/param_array_indexing.yaml Outdated Show resolved Hide resolved

ywluogg approved these changes May 16, 2022

View reviewed changes

Yongxuanzhang force-pushed the param-array-indexing branch from 36d5a6e to b5775fd Compare May 17, 2022 17:14

Yongxuanzhang force-pushed the param-array-indexing branch from b5775fd to 6115e03 Compare May 17, 2022 19:28

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2022

Yongxuanzhang force-pushed the param-array-indexing branch from 4d183f2 to 2e10695 Compare July 11, 2022 20:17

Yongxuanzhang requested a review from lbernick July 11, 2022 20:17

Yongxuanzhang commented Jul 11, 2022

View reviewed changes

lbernick approved these changes Jul 11, 2022

View reviewed changes

pkg/reconciler/pipelinerun/resources/validate_params_test.go Outdated Show resolved Hide resolved

pkg/reconciler/pipelinerun/resources/validate_params.go Outdated Show resolved Hide resolved

pkg/substitution/substitution.go Outdated Show resolved Hide resolved

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2022

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2022

Yongxuanzhang force-pushed the param-array-indexing branch from 2526da1 to 6366031 Compare July 12, 2022 13:41

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2022

Yongxuanzhang force-pushed the param-array-indexing branch from 6366031 to b4ac6b4 Compare July 12, 2022 15:58

pritidesai added this to the Pipelines v0.38 milestone Jul 12, 2022

Yongxuanzhang force-pushed the param-array-indexing branch from b4ac6b4 to 3a5b98e Compare July 13, 2022 17:35

[TEP-0076] Add indexing into array for params

f5b5b79

This commit provides the indexing into array for params and gated by alpha feature flag. Before this commit we can only refer to the whole array param, with this feature we can refer to array's element such as $(params.param-name[0]).

Yongxuanzhang force-pushed the param-array-indexing branch from 3a5b98e to f5b5b79 Compare July 13, 2022 19:00

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 13, 2022

tekton-robot assigned dibyom Jul 13, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2022

tekton-robot merged commit d3b8017 into tektoncd:main Jul 13, 2022

Yongxuanzhang mentioned this pull request Sep 27, 2022

Mark tep75&76 implemented tektoncd/community#833

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP-0076] Add indexing into array for pipeline params reference #4855

[TEP-0076] Add indexing into array for pipeline params reference #4855

Yongxuanzhang commented May 11, 2022 •

edited

Loading

tekton-robot commented May 11, 2022

tekton-robot commented May 11, 2022

tekton-robot commented May 11, 2022

tekton-robot commented May 11, 2022

Yongxuanzhang commented May 11, 2022

tekton-robot commented May 11, 2022

Yongxuanzhang commented May 11, 2022

ywluogg commented May 11, 2022

ywluogg May 16, 2022

Yongxuanzhang May 16, 2022

ywluogg May 16, 2022

dibyom Jul 7, 2022

Yongxuanzhang Jul 7, 2022 •

edited

Loading

Yongxuanzhang Jul 7, 2022

lbernick Jul 8, 2022

tekton-robot commented May 16, 2022

tekton-robot commented May 16, 2022

ywluogg left a comment •

edited

Loading

tekton-robot commented May 17, 2022

tekton-robot commented May 17, 2022

ywluogg commented Jun 2, 2022

tekton-robot commented Jul 11, 2022

Yongxuanzhang Jul 11, 2022

tekton-robot commented Jul 11, 2022

tekton-robot commented Jul 12, 2022

tekton-robot commented Jul 12, 2022

tekton-robot commented Jul 12, 2022

Yongxuanzhang commented Jul 12, 2022

tekton-robot commented Jul 12, 2022

Yongxuanzhang commented Jul 12, 2022

tekton-robot commented Jul 13, 2022

tekton-robot commented Jul 13, 2022

dibyom commented Jul 13, 2022

[TEP-0076] Add indexing into array for pipeline params reference #4855

[TEP-0076] Add indexing into array for pipeline params reference #4855

Conversation

Yongxuanzhang commented May 11, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 11, 2022

tekton-robot commented May 11, 2022

tekton-robot commented May 11, 2022

tekton-robot commented May 11, 2022

Yongxuanzhang commented May 11, 2022

tekton-robot commented May 11, 2022

Yongxuanzhang commented May 11, 2022

ywluogg commented May 11, 2022

ywluogg May 16, 2022

Choose a reason for hiding this comment

Yongxuanzhang May 16, 2022

Choose a reason for hiding this comment

ywluogg May 16, 2022

Choose a reason for hiding this comment

dibyom Jul 7, 2022

Choose a reason for hiding this comment

Yongxuanzhang Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Yongxuanzhang Jul 7, 2022

Choose a reason for hiding this comment

lbernick Jul 8, 2022

Choose a reason for hiding this comment

tekton-robot commented May 16, 2022

tekton-robot commented May 16, 2022

ywluogg left a comment • edited Loading

Choose a reason for hiding this comment

tekton-robot commented May 17, 2022

tekton-robot commented May 17, 2022

ywluogg commented Jun 2, 2022

tekton-robot commented Jul 11, 2022

Yongxuanzhang Jul 11, 2022

Choose a reason for hiding this comment

tekton-robot commented Jul 11, 2022

tekton-robot commented Jul 12, 2022

tekton-robot commented Jul 12, 2022

tekton-robot commented Jul 12, 2022

Yongxuanzhang commented Jul 12, 2022

tekton-robot commented Jul 12, 2022

Yongxuanzhang commented Jul 12, 2022

tekton-robot commented Jul 13, 2022

tekton-robot commented Jul 13, 2022

dibyom commented Jul 13, 2022

Yongxuanzhang commented May 11, 2022 •

edited

Loading

Yongxuanzhang Jul 7, 2022 •

edited

Loading

ywluogg left a comment •

edited

Loading