[TEP-0076]Support Results Array Indexing #4911

Yongxuanzhang · 2022-05-26T00:46:13Z

Changes

This is part of work in TEP-0076.
This commit provides the support to refer array indexing results.
Previous this commit we support emitting array results so users can
write array results to task level, but we cannot pass array results via
index between tasks within one pipeline. This commit adds the support for this.

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

Support indexing array results substitution as an alpha feature.

A task can specify a type to produce array result, such as:

  results:
    - name: array-results
       type: array
       description: The array results

And the task script can populate result in an array form with:

echo -n "[\"hello\",\"world\"]" | tee $(results.array-results.path)

and we can refer to the array results elements via index in param like:
  params:
    - name: foo
      value: "$(tasks.task1.results.array-results[1])"

This feature is part of the TEP-0076.

tekton-robot · 2022-05-26T00:49:35Z

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/resources/resultrefresolution.go	94.0%	94.3%	0.3

tekton-robot · 2022-05-26T01:09:27Z

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/resources/resultrefresolution.go	94.0%	94.3%	0.3

ywluogg · 2022-05-26T15:01:01Z

/assign ywluogg

ywluogg

Mostly LGTM. Please fix those comments for test cases

pkg/reconciler/pipelinerun/resources/apply_test.go

ywluogg · 2022-05-26T19:43:44Z

pkg/apis/pipeline/v1beta1/resultref.go

+	// variableSubstitutionFormat matches format like $result.resultname, $result.resultname[int] and $result.resultname[*]
+	variableSubstitutionFormat = `\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9])*\*?\])?\)`
+	// excludeArrayIndexing will replace all `[int]` and `[*]` for parseExpression to extract result name
+	excludeArrayIndexing = `\[([0-9])*\*?\]`


Discussed offline about renaming

pkg/reconciler/pipelinerun/resources/apply_test.go

docs/variables.md

tekton-robot · 2022-05-30T21:47:07Z

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/resources/pipelinerunresolution.go	91.9%	92.0%	0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	94.0%	94.6%	0.6

tekton-robot · 2022-05-30T22:06:03Z

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/resources/pipelinerunresolution.go	91.9%	92.0%	0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	94.0%	94.6%	0.6

tekton-robot · 2022-05-30T22:19:27Z

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/resources/pipelinerunresolution.go	91.9%	92.0%	0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	94.0%	95.7%	1.7

tekton-robot · 2022-05-30T22:57:52Z

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/resources/pipelinerunresolution.go	91.9%	92.0%	0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	94.0%	95.7%	1.7

ywluogg

Can you also add examples into the release notes?

ywluogg · 2022-05-31T21:22:05Z

pkg/reconciler/pipelinerun/resources/apply_test.go

+	}, {
+		name: "Test array indexing result substitution out of bound - params",
+		resolvedResultRefs: ResolvedResultRefs{{
+			Value: *v1beta1.NewArrayOrString("arrayResultValueOne", "arrayResultValueTwo"),
+			ResultReference: v1beta1.ResultRef{
+				PipelineTask: "aTask",
+				Result:       "a.Result",
+			},
+			FromTaskRun: "aTaskRun",
+		}},
+		targets: PipelineRunState{{
+			PipelineTask: &v1beta1.PipelineTask{
+				Name:    "bTask",
+				TaskRef: &v1beta1.TaskRef{Name: "bTask"},
+				Params: []v1beta1.Param{{
+					Name:  "bParam",
+					Value: *v1beta1.NewArrayOrString(`$(tasks.aTask.results["a.Result"][3])`),
+				}},
+			},
+		}},
+		want: PipelineRunState{{
+			PipelineTask: &v1beta1.PipelineTask{
+				Name:    "bTask",
+				TaskRef: &v1beta1.TaskRef{Name: "bTask"},
+				Params: []v1beta1.Param{{
+					Name: "bParam",
+					// index validation is done in ResolveResultRefs() before ApplyTaskResults()
+					Value: *v1beta1.NewArrayOrString(`$(tasks.aTask.results["a.Result"][3])`),
+				}},
+			},
+		}},


Ah adding the tests in resultresolution_test should be good enough. But this test case makes it clear that out of bound cases here are handled in this specific way. I think keeping this case is fine. You can resolve this comment

pkg/reconciler/pipelinerun/resources/apply_test.go

tekton-robot · 2022-05-31T21:23:35Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

ywluogg · 2022-05-31T21:25:03Z

/lgtm

tekton-robot · 2022-05-31T21:25:11Z

@ywluogg: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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-06-01T14:05:17Z

Can you also add examples into the release notes?

Yes! I will add release notes soon

lbernick · 2022-06-01T16:39:53Z

examples/v1beta1/pipelineruns/alpha/pipelinerun-array-resluts-indexing.yaml

+        taskSpec:
+          results:
+            - name: array-results
+              description: The array resluts


there's a typo here and in the name of this file

isn't specifying type necessary here?

there's a typo here and in the name of this file

Thank you!

isn't specifying type necessary here?

Yes, I will add this, thanks for catching! It is not failing in this pr because the validation we discussed is implemented in this pr #4920

filename still needs to be fixed

Oh sorry my bad!

lbernick · 2022-06-01T16:42:15Z

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

@@ -306,8 +306,10 @@ func (t *ResolvedPipelineRunTask) skipBecauseResultReferencesAreMissing(facts *P
 	if t.checkParentsDone(facts) && t.hasResultReferences() {
 		resolvedResultRefs, pt, err := ResolveResultRefs(facts.State, PipelineRunState{t})
 		rprt := facts.State.ToMap()[pt]
-		if err != nil && (t.IsFinalTask(facts) || rprt.Skip(facts).SkippingReason == v1beta1.WhenExpressionsSkip) {
-			return true
+		if rprt != nil {


what's the reason for this change? it doesn't look related to results

Yes it is not so straightforward, not sure if I'm doing here is correct or there could be better solutions.

We need to validate if the referred index is out of bound of results array, and it is done in ResolveResultRefs, so the ResolveResultRefs will return err when out of bound and there are cases that the rprt is nil, so it will fail here if I don't add this check.

And in this case of this function we will still return true even we got err here. But it will return our err in another function runNextSchedulableTask

examples/v1beta1/pipelineruns/alpha/pipelinerun-array-resluts-indexing.yaml

pritidesai · 2022-06-01T17:23:42Z

docs/variables.md

@@ -22,6 +22,9 @@ For instructions on using variable substitutions see the relevant section of [th
 | `tasks.<taskName>.results.<resultName>` | The value of the `Task's` result. Can alter `Task` execution order within a `Pipeline`.) |
 | `tasks.<taskName>.results['<resultName>']` | (see above)) |
 | `tasks.<taskName>.results["<resultName>"]` | (see above)) |
+| `tasks.<taskName>.results.<resultName>[i]` | The ith value of the `Task's` array result. Can alter `Task` execution order within a `Pipeline`.) |


How about start notation - tasks.<taskName>.results.<resultName>[*]? is that something supported in this context?

star notation is not supported in this pr, it is in this one #4908

star notation is used to refer the whole array while this pr focus on the reference of array elements via index

tekton-robot · 2022-06-01T19:08:08Z

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/resources/pipelinerunresolution.go	91.9%	92.0%	0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	94.0%	94.6%	0.6

Yongxuanzhang · 2022-06-01T19:42:18Z

/retest

Yongxuanzhang · 2022-06-01T21:04:05Z

/retest

lbernick · 2022-06-02T15:45:29Z

happy to LGTM but @pritidesai just want to make sure your concerns are addressed first

Yongxuanzhang · 2022-06-02T17:16:06Z

happy to LGTM but @pritidesai just want to make sure your concerns are addressed first

Yeah sure! I will wait for @pritidesai's reviews

tekton-robot · 2022-06-02T17:23:04Z

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/resources/pipelinerunresolution.go	91.9%	92.0%	0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	94.0%	94.6%	0.6

Yongxuanzhang · 2022-06-02T17:56:20Z

/retest

ywluogg · 2022-06-02T18:20:45Z

Link this to #4723

lbernick · 2022-06-07T15:43:01Z

/lgtm

Yongxuanzhang · 2022-06-07T18:43:23Z

/retest

Yongxuanzhang · 2022-06-07T21:22:11Z

/retest

This is part of work in TEP-0076. This commit provides the support to refer array indexing results. Previous this commit we support emitting array results so users can write array results to task level, but we cannot pass array results via index between tasks within one pipeline. This commit adds the support for this.

tekton-robot · 2022-06-07T23:39: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/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.4%	93.5%	0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	94.0%	94.6%	0.6

lbernick · 2022-06-08T14:35:46Z

/lgtm

chuangw6 · 2022-06-09T21:00:53Z

pkg/apis/pipeline/v1beta1/resultref.go

+func parseExpression(substitutionExpression string) (string, string, int, error) {
 	subExpressions := strings.Split(substitutionExpression, ".")
 	if len(subExpressions) != 4 || subExpressions[0] != ResultTaskPart || subExpressions[2] != ResultResultPart {
-		return "", "", fmt.Errorf("Must be of the form %q", resultExpressionFormat)
+		return "", "", 0, fmt.Errorf("Must be of the form %q", resultExpressionFormat)
 	}
-	return subExpressions[1], subExpressions[3], nil
+
+	stringIdx := strings.TrimSuffix(strings.TrimPrefix(arrayIndexingRegex.FindString(subExpressions[3]), "["), "]")
+	subExpressions[3] = arrayIndexingRegex.ReplaceAllString(subExpressions[3], "")
+	if stringIdx != "" {
+		intIdx, _ := strconv.Atoi(stringIdx)
+		return subExpressions[1], subExpressions[3], intIdx, nil
+	}
+	return subExpressions[1], subExpressions[3], 0, nil


When the index part in bracket for array or the whole expression is invalid, or it's a reference to a string result, wouldn't it make more sense if we set the third return value to be -1 to indicate it's not array or invalid value for index?

afrittoli · 2022-06-21T20:50:16Z

/kind feature

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

tekton-robot requested review from bobcatfish and lbernick May 26, 2022 00:46

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 26, 2022

Yongxuanzhang force-pushed the results-array-indexing branch from dc30ce8 to f41d7ea Compare May 26, 2022 01:06

Yongxuanzhang marked this pull request as ready for review May 26, 2022 01:43

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 26, 2022

tekton-robot assigned ywluogg May 26, 2022

ywluogg reviewed May 26, 2022

View reviewed changes

Yongxuanzhang commented May 30, 2022

View reviewed changes

docs/variables.md Show resolved Hide resolved

vdemeester approved these changes May 30, 2022

View reviewed changes

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

Yongxuanzhang force-pushed the results-array-indexing branch from 222b662 to 9286433 Compare May 30, 2022 22:03

Yongxuanzhang force-pushed the results-array-indexing branch from 9286433 to 04e815a Compare May 30, 2022 22:15

Yongxuanzhang force-pushed the results-array-indexing branch from 04e815a to 4d2896c Compare May 30, 2022 22:53

ywluogg approved these changes May 31, 2022

View reviewed changes

lbernick reviewed Jun 1, 2022

View reviewed changes

pritidesai reviewed Jun 1, 2022

View reviewed changes

examples/v1beta1/pipelineruns/alpha/pipelinerun-array-resluts-indexing.yaml Outdated Show resolved Hide resolved

pritidesai reviewed Jun 1, 2022

View reviewed changes

Yongxuanzhang force-pushed the results-array-indexing branch from 4d2896c to 0dcdf25 Compare June 1, 2022 19:05

Yongxuanzhang requested review from pritidesai and lbernick June 1, 2022 19:05

Yongxuanzhang force-pushed the results-array-indexing branch from 0dcdf25 to 2e0298a Compare June 2, 2022 17:19

tekton-robot assigned lbernick Jun 7, 2022

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

Yongxuanzhang force-pushed the results-array-indexing branch from 2e0298a to 2ebdbff Compare June 7, 2022 23:37

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2022

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

tekton-robot merged commit 566e2e3 into tektoncd:main Jun 8, 2022

chuangw6 reviewed Jun 9, 2022

View reviewed changes

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

ywluogg mentioned this pull request Jun 22, 2022

Add ywluogg into Pipeline reviewer roles tektoncd/community#737

Merged

Yongxuanzhang mentioned this pull request Sep 27, 2022

Mark tep75&76 implemented tektoncd/community#833

Merged

[TEP-0076]Support Results Array Indexing #4911

[TEP-0076]Support Results Array Indexing #4911

Conversation

Yongxuanzhang commented May 26, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 26, 2022

tekton-robot commented May 26, 2022

ywluogg commented May 26, 2022

ywluogg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented May 30, 2022

tekton-robot commented May 30, 2022

tekton-robot commented May 30, 2022

tekton-robot commented May 30, 2022

ywluogg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented May 31, 2022

ywluogg commented May 31, 2022

tekton-robot commented May 31, 2022

Yongxuanzhang commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yongxuanzhang Jun 1, 2022 • edited Loading

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

Yongxuanzhang Jun 1, 2022 • edited Loading

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 Jun 1, 2022

Yongxuanzhang commented Jun 1, 2022

Yongxuanzhang commented Jun 1, 2022

lbernick commented Jun 2, 2022

Yongxuanzhang commented Jun 2, 2022 • edited Loading

tekton-robot commented Jun 2, 2022

Yongxuanzhang commented Jun 2, 2022

ywluogg commented Jun 2, 2022

lbernick commented Jun 7, 2022

Yongxuanzhang commented Jun 7, 2022

Yongxuanzhang commented Jun 7, 2022

tekton-robot commented Jun 7, 2022

lbernick commented Jun 8, 2022

Choose a reason for hiding this comment

afrittoli commented Jun 21, 2022

Yongxuanzhang commented May 26, 2022 •

edited

Loading

Yongxuanzhang Jun 1, 2022 •

edited

Loading

Yongxuanzhang Jun 1, 2022 •

edited

Loading

Yongxuanzhang commented Jun 2, 2022 •

edited

Loading