Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change ResultRef.ResultsIndex from int to *int #7460

Merged
merged 1 commit into from
Dec 8, 2023
Merged

change ResultRef.ResultsIndex from int to *int #7460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions examples/v1/pipelineruns/beta/7392-regression.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
apiVersion: tekton.dev/v1
JeromeJu marked this conversation as resolved.
Show resolved Hide resolved
kind: Task
metadata:
name: task-with-array-result
spec:
params:
- name: arrayVal1
description: "description3"
type: array
default: []
steps:
- name: step1
image: bash:latest
args:
- --images
- $(params.arrayVal1[*])
script: |
#!/usr/bin/env bash
for arg in "$@"; do
echo $arg
done
- name: step2
image: bash:latest
script: |
#!/usr/bin/env bash
echo -n '[]' | tee $(results.resultArray.path)
results:
- name: resultArray
description: "description4"
type: array

---
apiVersion: tekton.dev/v1
kind: PipelineRun
metadata:
name: pipelinerun-array-result
spec:
pipelineSpec:
tasks:
- name: task-1
taskRef:
name: task-with-array-result
params:
- name: arrayVal1
value: []
- name: task-2
taskRef:
name: task-with-array-result
params:
- name: arrayVal1
value: $(tasks.task-1.results.resultArray)
5 changes: 2 additions & 3 deletions pkg/apis/pipeline/v1/openapi_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 17 additions & 14 deletions pkg/apis/pipeline/v1/resultref.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (
type ResultRef struct {
PipelineTask string `json:"pipelineTask"`
Result string `json:"result"`
ResultsIndex int `json:"resultsIndex"`
ResultsIndex *int `json:"resultsIndex"`
Property string `json:"property"`
}

Expand Down Expand Up @@ -122,37 +122,40 @@ func stripVarSubExpression(expression string) string {
}

// parseExpression parses "task name", "result name", "array index" (iff it's an array result) and "object key name" (iff it's an object result)
// Valid Example 1:
// 1. Reference string result
// - Input: tasks.myTask.results.aStringResult
// - Output: "myTask", "aStringResult", -1, "", nil
// Valid Example 2:
// - Output: "myTask", "aStringResult", nil, "", nil
// 2. Reference Object value with key:
// - Input: tasks.myTask.results.anObjectResult.key1
// - Output: "myTask", "anObjectResult", 0, "key1", nil
// Valid Example 3:
// - Output: "myTask", "anObjectResult", nil, "key1", nil
// 3. Reference array elements with array indexing :
// - Input: tasks.myTask.results.anArrayResult[1]
// - Output: "myTask", "anArrayResult", 1, "", nil
// Invalid Example 1:
// 4. Referencing whole array or object result:
// - Input: tasks.myTask.results.Result[*]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@Yongxuanzhang Yongxuanzhang Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// - Output: "myTask", "Result", nil, "", nil
// Invalid Case:
// - Input: tasks.myTask.results.resultName.foo.bar
// - Output: "", "", 0, "", error
// - Output: "", "", nil, "", error
// TODO: may use regex for each type to handle possible reference formats
func parseExpression(substitutionExpression string) (string, string, int, string, error) {
func parseExpression(substitutionExpression string) (string, string, *int, string, error) {
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 != "*" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

intIdx, _ := strconv.Atoi(stringIdx)
return subExpressions[1], resultName, intIdx, "", nil
return subExpressions[1], resultName, &intIdx, "", nil
}
return subExpressions[1], resultName, 0, "", nil
return subExpressions[1], resultName, nil, "", nil
} else if len(subExpressions) == 5 {
// For object type result: tasks.<taskName>.results.<objectResultName>.<individualAttribute>
return subExpressions[1], subExpressions[3], 0, subExpressions[4], nil
return subExpressions[1], subExpressions[3], nil, subExpressions[4], nil
}
}
return "", "", 0, "", fmt.Errorf("must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat)
return "", "", nil, "", fmt.Errorf("must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat)
}

// ParseResultName parse the input string to extract resultName and result index.
Expand Down
6 changes: 4 additions & 2 deletions pkg/apis/pipeline/v1/resultref_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
)

func TestNewResultReference(t *testing.T) {
idx := 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@Yongxuanzhang Yongxuanzhang Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for _, tt := range []struct {
name string
param v1.Param
Expand All @@ -43,14 +44,15 @@ func TestNewResultReference(t *testing.T) {
Result: "sumResult",
}},
}, {
name: "refer whole array result",
name: "reference whole array result",
param: v1.Param{
Name: "param",
Value: *v1.NewStructuredValues("$(tasks.sumTask.results.sumResult[*])"),
},
want: []*v1.ResultRef{{
PipelineTask: "sumTask",
Result: "sumResult",
ResultsIndex: nil,
}},
}, {
name: "Test valid expression with single object result property",
Expand All @@ -72,7 +74,7 @@ func TestNewResultReference(t *testing.T) {
want: []*v1.ResultRef{{
PipelineTask: "sumTask",
Result: "sumResult",
ResultsIndex: 1,
ResultsIndex: &idx,
}},
}, {
name: "Test valid expression with multiple object result properties",
Expand Down
3 changes: 1 addition & 2 deletions pkg/apis/pipeline/v1/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1176,8 +1176,7 @@
},
"resultsIndex": {
"type": "integer",
"format": "int32",
"default": 0
"format": "int32"
}
}
},
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/pipeline/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions pkg/apis/pipeline/v1beta1/openapi_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 17 additions & 15 deletions pkg/apis/pipeline/v1beta1/resultref.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (
type ResultRef struct {
PipelineTask string `json:"pipelineTask"`
Result string `json:"result"`
ResultsIndex int `json:"resultsIndex"`
ResultsIndex *int `json:"resultsIndex"`
Property string `json:"property"`
}

Expand Down Expand Up @@ -157,38 +157,40 @@ func stripVarSubExpression(expression string) string {
}

// parseExpression parses "task name", "result name", "array index" (iff it's an array result) and "object key name" (iff it's an object result)
// Valid Example 1:
// 1. Reference string result
// - Input: tasks.myTask.results.aStringResult
// - Output: "myTask", "aStringResult", -1, "", nil
// Valid Example 2:
// - Output: "myTask", "aStringResult", nil, "", nil
// 2. Reference Object value with key:
// - Input: tasks.myTask.results.anObjectResult.key1
// - Output: "myTask", "anObjectResult", 0, "key1", nil
// Valid Example 3:
// - Output: "myTask", "anObjectResult", nil, "key1", nil
// 3. Reference array elements with array indexing :
// - Input: tasks.myTask.results.anArrayResult[1]
// - Output: "myTask", "anArrayResult", 1, "", nil
// Invalid Example 1:
// 4. Referencing whole array or object result:
// - Input: tasks.myTask.results.Result[*]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

// - Output: "myTask", "Result", nil, "", nil
// Invalid Case:
// - Input: tasks.myTask.results.resultName.foo.bar
// - Output: "", "", 0, "", error
// - Output: "", "", nil, "", error
// TODO: may use regex for each type to handle possible reference formats
func parseExpression(substitutionExpression string) (string, string, int, string, error) {
func parseExpression(substitutionExpression string) (string, string, *int, string, error) {
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 != "*" {
intIdx, _ := strconv.Atoi(stringIdx)
return subExpressions[1], resultName, intIdx, "", nil
return subExpressions[1], resultName, &intIdx, "", nil
}
return subExpressions[1], resultName, 0, "", nil
return subExpressions[1], resultName, nil, "", nil
} else if len(subExpressions) == 5 {
// For object type result: tasks.<taskName>.results.<objectResultName>.<individualAttribute>
return subExpressions[1], subExpressions[3], 0, subExpressions[4], nil
return subExpressions[1], subExpressions[3], nil, subExpressions[4], nil
}
}

return "", "", 0, "", fmt.Errorf("must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat)
return "", "", nil, "", fmt.Errorf("must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat)
}

// ParseResultName parse the input string to extract resultName and result index.
Expand Down
6 changes: 4 additions & 2 deletions pkg/apis/pipeline/v1beta1/resultref_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
)

func TestNewResultReference(t *testing.T) {
idx1 := 1
for _, tt := range []struct {
name string
param v1beta1.Param
Expand All @@ -42,14 +43,15 @@ func TestNewResultReference(t *testing.T) {
Result: "sumResult",
}},
}, {
name: "refer whole array result",
name: "reference whole array/object result",
param: v1beta1.Param{
Name: "param",
Value: *v1beta1.NewStructuredValues("$(tasks.sumTask.results.sumResult[*])"),
},
want: []*v1beta1.ResultRef{{
PipelineTask: "sumTask",
Result: "sumResult",
ResultsIndex: nil,
}},
}, {
name: "Test valid expression with single object result property",
Expand All @@ -71,7 +73,7 @@ func TestNewResultReference(t *testing.T) {
want: []*v1beta1.ResultRef{{
PipelineTask: "sumTask",
Result: "sumResult",
ResultsIndex: 1,
ResultsIndex: &idx1,
}},
}, {
name: "Test valid expression with multiple object result properties",
Expand Down
3 changes: 1 addition & 2 deletions pkg/apis/pipeline/v1beta1/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1780,8 +1780,7 @@
},
"resultsIndex": {
"type": "integer",
"format": "int32",
"default": 0
"format": "int32"
}
}
},
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions pkg/reconciler/pipelinerun/resources/resultrefresolution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ func ResolveResultRefs(pipelineRunState PipelineRunState, targets PipelineRunSta
func validateArrayResultsIndex(allResolvedResultRefs ResolvedResultRefs) error {
for _, r := range allResolvedResultRefs {
if r.Value.Type == v1.ParamTypeArray {
if r.ResultReference.ResultsIndex >= len(r.Value.ArrayVal) {
return fmt.Errorf("array Result Index %d for Task %s Result %s is out of bound of size %d", r.ResultReference.ResultsIndex, r.ResultReference.PipelineTask, r.ResultReference.Result, len(r.Value.ArrayVal))
if r.ResultReference.ResultsIndex != nil && *r.ResultReference.ResultsIndex >= len(r.Value.ArrayVal) {
return fmt.Errorf("array Result Index %d for Task %s Result %s is out of bound of size %d", *r.ResultReference.ResultsIndex, r.ResultReference.PipelineTask, r.ResultReference.Result, len(r.Value.ArrayVal))
}
}
}
Expand Down
Loading
Loading