[TEP-0076] Add indexing into array for taskrun params reference #5132

Yongxuanzhang · 2022-07-13T15:22:55Z

Changes

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 for taskrun params, with this feature we can refer to array's element such as
$(params.param-name[0]).

/kind feature

Submitter Checklist

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

Has Docs included if any changes are user facing
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)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

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

tekton-robot · 2022-07-13T15:22:58Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Yongxuanzhang · 2022-07-13T15:24:42Z

/kind feature

lbernick · 2022-07-14T13:57:09Z

pkg/reconciler/taskrun/resources/apply.go

@@ -58,6 +59,12 @@ func ApplyParameters(spec *v1beta1.TaskSpec, tr *v1beta1.TaskRun, defaults ...v1
 			switch p.Default.Type {
 			case v1beta1.ParamTypeArray:
 				for _, pattern := range patterns {
+					// array indexing for param is alpha feature


out of scope for this PR, but these two for loops have a lot of duplicate code that could be consolidated.

yes, we can also refactor the code in pipelinerun as well

pkg/reconciler/taskrun/resources/apply_test.go

lbernick · 2022-07-14T14:00:27Z

pkg/reconciler/taskrun/validate_resources.go

@@ -353,3 +369,200 @@ func missingKeysofObjectResults(tr *v1beta1.TaskRun, specResults []v1beta1.TaskR
 	}
 	return findMissingKeys(neededKeys, providedKeys)
 }
+
+func validateParamArrayIndex(ctx context.Context, params []v1beta1.Param, spec *v1beta1.TaskSpec) error {


this block of code looks extremely similar to what was introduced in the pipelinerun reconciler. Can any of it be moved to a common location?

yes! I was thinking if we want to merge the results code from taskrun and pipelinerun into one pkg then they can share functions like this.

Right now I'm not so sure where can I put it. Maybe pkg/reconciler/resources.go ?

Oh but this function is different, the one in pipelinerun will traverse all pipelinespec fields and this is to traverse TaskSpec, another function extractParamIndex is the same and can be merged

I'm not sure if you're referring to extractParamIndex here or extractParamIndexes in the PR reconciler but either way yes they could be pulled out. I'd create a new package pkg/params and put it there in a separate PR

sure, so maybe this is part of our refactoring work later?

I'm ok with this for now, but typically I'd caution against saving refactoring work for after a feature is implemented. I don't think the changes I'm suggesting will take much time.

tekton-robot · 2022-07-14T14:40:16Z

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.3%	0.0
pkg/reconciler/taskrun/taskrun.go	80.9%	80.3%	-0.6
pkg/reconciler/taskrun/validate_resources.go	96.2%	94.6%	-1.5

Yongxuanzhang · 2022-07-14T14:44:10Z

@ywluogg

Yongxuanzhang · 2022-07-14T14:44:20Z

#4723

tekton-robot · 2022-07-14T14:56:14Z

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.3%	0.0
pkg/reconciler/taskrun/taskrun.go	80.9%	80.3%	-0.6
pkg/reconciler/taskrun/validate_resources.go	96.2%	94.6%	-1.5

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 for taskrun params, with this feature we can refer to array's element such as $(params.param-name[0]).

tekton-robot · 2022-07-14T15:56:40Z

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.3%	0.0
pkg/reconciler/taskrun/taskrun.go	80.9%	80.3%	-0.6
pkg/reconciler/taskrun/validate_resources.go	96.2%	97.7%	1.5

ywluogg · 2022-07-14T17:40:54Z

/assign @ywluogg

lbernick · 2022-07-14T17:59:38Z

pkg/reconciler/taskrun/validate_resources.go

@@ -353,3 +369,200 @@ func missingKeysofObjectResults(tr *v1beta1.TaskRun, specResults []v1beta1.TaskR
 	}
 	return findMissingKeys(neededKeys, providedKeys)
 }
+
+func validateParamArrayIndex(ctx context.Context, params []v1beta1.Param, spec *v1beta1.TaskSpec) error {


I'm ok with this for now, but typically I'd caution against saving refactoring work for after a feature is implemented. I don't think the changes I'm suggesting will take much time.

pkg/reconciler/taskrun/resources/apply_test.go

tekton-robot · 2022-07-14T18:01:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [lbernick]

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

ywluogg · 2022-07-14T19:28:38Z

/lgtm

Yongxuanzhang · 2022-07-14T19:34:48Z

/retest

Yongxuanzhang · 2022-07-14T19:41:46Z

/retest

Yongxuanzhang · 2022-07-14T20:13:06Z

/retest

Yongxuanzhang · 2022-07-14T20:20:13Z

/retest-required

Yongxuanzhang · 2022-07-14T20:30:08Z

/retest-required

Yongxuanzhang · 2022-07-14T20:37:39Z

/retest-required

Yongxuanzhang · 2022-07-14T20:53:39Z

/retest-required

tekton-robot requested review from bobcatfish and jerop July 13, 2022 15:23

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 13, 2022

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 13, 2022

Yongxuanzhang force-pushed the param-array-indexing-taskrun branch 2 times, most recently from 2f5fda9 to 082d60c Compare July 13, 2022 22:01

lbernick reviewed Jul 14, 2022

View reviewed changes

Yongxuanzhang marked this pull request as ready for review July 14, 2022 14:35

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 Jul 14, 2022

tekton-robot requested review from abayer and afrittoli July 14, 2022 14:35

Yongxuanzhang force-pushed the param-array-indexing-taskrun branch from 2309bca to 754a52c Compare July 14, 2022 15:51

Yongxuanzhang requested review from dibyom and lbernick July 14, 2022 16:38

tekton-robot assigned ywluogg Jul 14, 2022

lbernick approved these changes Jul 14, 2022

View reviewed changes

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

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

tekton-robot merged commit 32b01cf into tektoncd:main Jul 14, 2022

ywluogg mentioned this pull request Jul 15, 2022

[TEP 0075, 0076]: Structured object / arrays in Results and Params #4723

Closed

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 taskrun params reference #5132

[TEP-0076] Add indexing into array for taskrun params reference #5132

Yongxuanzhang commented Jul 13, 2022 •

edited

Loading

tekton-robot commented Jul 13, 2022

Yongxuanzhang commented Jul 13, 2022

lbernick Jul 14, 2022

Yongxuanzhang Jul 14, 2022

lbernick Jul 14, 2022

Yongxuanzhang Jul 14, 2022

Yongxuanzhang Jul 14, 2022

lbernick Jul 14, 2022

Yongxuanzhang Jul 14, 2022

lbernick Jul 14, 2022

tekton-robot commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

tekton-robot commented Jul 14, 2022

tekton-robot commented Jul 14, 2022

ywluogg commented Jul 14, 2022

lbernick Jul 14, 2022

tekton-robot commented Jul 14, 2022

ywluogg commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

[TEP-0076] Add indexing into array for taskrun params reference #5132

[TEP-0076] Add indexing into array for taskrun params reference #5132

Conversation

Yongxuanzhang commented Jul 13, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jul 13, 2022

Yongxuanzhang commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

tekton-robot commented Jul 14, 2022

tekton-robot commented Jul 14, 2022

ywluogg commented Jul 14, 2022

Choose a reason for hiding this comment

tekton-robot commented Jul 14, 2022

ywluogg commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 14, 2022

Yongxuanzhang commented Jul 13, 2022 •

edited

Loading