change ResultRef.ResultsIndex from int to *int #7460

Yongxuanzhang · 2023-12-05T21:59:00Z

Changes

This commit closes #7392. When we introduced array results, we added validation funciton to check if the result reference is out of the array bound, in the cases of refercing a whole array and that array is empty, the resolved array index is 0, so the validation will error. Since the resolved index is only used when array indexing references exist, it is an optional field for ResultRef so we should change it to a pointer.

This is not an API change although ResultRef is in our API pkg, but it is not used in any of our APIs and it is only used in controller code.

/kind bug

Signed-off-by: Yongxuan Zhang [email protected]

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

NONE

tekton-robot · 2023-12-06T16:35:06Z

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	99.2%	98.3%	-0.8

tekton-robot · 2023-12-06T16:35:27Z

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/reconciler/pipelinerun/resources/resultrefresolution.go	99.2%	98.3%	-0.8

JeromeJu · 2023-12-06T22:22:10Z

pkg/apis/pipeline/v1/resultref.go

 	if looksLikeResultRef(substitutionExpression) {
 		subExpressions := strings.Split(substitutionExpression, ".")
 		// For string result: tasks.<taskName>.results.<stringResultName>
 		// For array result: tasks.<taskName>.results.<arrayResultName>[index]
 		if len(subExpressions) == 4 {
 			resultName, stringIdx := ParseResultName(subExpressions[3])
-			if stringIdx != "" {
+			if stringIdx != "" && stringIdx != "*" {


(just checking my understanding here 🤔 ) Is this fully related with the pointer change?

It is related. 😂
We only want to set non-nil value of result index for the case of array indexing. Otherwise we will return 0 in the case of [*] reference.That's the cause of the linked bug

examples/v1/pipelineruns/beta/7392-regression.yaml

QuanZhang-William · 2023-12-07T20:58:50Z

pkg/apis/pipeline/v1/resultref.go

 // Valid Example 3:
 // - Input: tasks.myTask.results.anArrayResult[1]
 // - Output: "myTask", "anArrayResult", 1, "", nil
+// Valid Example 4:
+// - Input: tasks.myTask.results.Result[*]


Maybe out call here that the this ouput is when the input references an empty array?

Oh, this is the case of referencing a whole array or object results, they may not be empty. I can clarify this in the docstring

QuanZhang-William · 2023-12-07T21:00:20Z

pkg/apis/pipeline/v1/resultref_test.go

@@ -28,6 +28,7 @@ import (
 )

 func TestNewResultReference(t *testing.T) {
+	idx := 1


Does this really covers the change? IIUC we need a test case that references an empty array with [*] and test the ResultIndex is nil?

~~Oh thanks, I think I forgot to add a test case here~~
Sorry I just recalled that it is included in the current test, I will update a bit to show that we will return nil

QuanZhang-William · 2023-12-07T21:00:45Z

pkg/apis/pipeline/v1beta1/resultref.go

 // Valid Example 3:
 // - Input: tasks.myTask.results.anArrayResult[1]
 // - Output: "myTask", "anArrayResult", 1, "", nil
+// Valid Example 4:
+// - Input: tasks.myTask.results.Result[*]


same as above

This commit closes 7392. When we introduced array results, we added validation funciton to check if the result reference is out of the array bound, in the cases of refercing a whole array and that array is empty, the resolved array index is 0, so the validation will error. Since the resolved index is only used when array indexing references exist, it is an optional field for ResultRef so we should change it to a pointer. Signed-off-by: Yongxuan Zhang [email protected]

QuanZhang-William · 2023-12-08T18:27:12Z

/lgtm

tekton-robot · 2023-12-08T18:34:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JeromeJu

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

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

tekton-robot added kind/bug Categorizes issue or PR as related to a bug. release-note-none Denotes a PR that doesnt merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 5, 2023

tekton-robot requested review from bobcatfish and pritidesai December 5, 2023 21:59

Yongxuanzhang force-pushed the fix-7392 branch 2 times, most recently from 6ad3693 to 42f0847 Compare December 6, 2023 16:26

JeromeJu self-assigned this Dec 6, 2023

JeromeJu reviewed Dec 6, 2023

View reviewed changes

QuanZhang-William reviewed Dec 7, 2023

View reviewed changes

Yongxuanzhang force-pushed the fix-7392 branch from 42f0847 to 238f101 Compare December 8, 2023 15:58

Yongxuanzhang requested review from JeromeJu and QuanZhang-William December 8, 2023 16:11

tekton-robot assigned QuanZhang-William Dec 8, 2023

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

JeromeJu approved these changes Dec 8, 2023

View reviewed changes

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

tekton-robot merged commit f9737b2 into tektoncd:main Dec 8, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change ResultRef.ResultsIndex from int to *int #7460

change ResultRef.ResultsIndex from int to *int #7460

Yongxuanzhang commented Dec 5, 2023 •

edited

Loading

tekton-robot commented Dec 6, 2023

tekton-robot commented Dec 6, 2023

JeromeJu Dec 6, 2023

Yongxuanzhang Dec 7, 2023

QuanZhang-William Dec 7, 2023

Yongxuanzhang Dec 8, 2023 •

edited

Loading

QuanZhang-William Dec 7, 2023

Yongxuanzhang Dec 8, 2023 •

edited

Loading

QuanZhang-William Dec 7, 2023

QuanZhang-William commented Dec 8, 2023

tekton-robot commented Dec 8, 2023

change ResultRef.ResultsIndex from int to *int #7460

change ResultRef.ResultsIndex from int to *int #7460

Conversation

Yongxuanzhang commented Dec 5, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Dec 6, 2023

tekton-robot commented Dec 6, 2023

JeromeJu Dec 6, 2023

Choose a reason for hiding this comment

Yongxuanzhang Dec 7, 2023

Choose a reason for hiding this comment

QuanZhang-William Dec 7, 2023

Choose a reason for hiding this comment

Yongxuanzhang Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

QuanZhang-William Dec 7, 2023

Choose a reason for hiding this comment

Yongxuanzhang Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

QuanZhang-William Dec 7, 2023

Choose a reason for hiding this comment

QuanZhang-William commented Dec 8, 2023

tekton-robot commented Dec 8, 2023

Yongxuanzhang commented Dec 5, 2023 •

edited

Loading

Yongxuanzhang Dec 8, 2023 •

edited

Loading

Yongxuanzhang Dec 8, 2023 •

edited

Loading