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

Jump to bottom

[TEP-0076]Support Results Array Indexing #4911

Merged
merged 1 commit into from
Jun 8, 2022
Merged

[TEP-0076]Support Results Array Indexing #4911

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
3 changes: 3 additions & 0 deletions docs/variables.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.) |
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

| `tasks.<taskName>.results['<resultName>'][i]` | (see above)) |
| `tasks.<taskName>.results["<resultName>"][i]` | (see above)) |
Yongxuanzhang marked this conversation as resolved.
Show resolved Hide resolved
| `workspaces.<workspaceName>.bound` | Whether a `Workspace` has been bound or not. "false" if the `Workspace` declaration has `optional: true` and the Workspace binding was omitted by the PipelineRun. |
| `context.pipelineRun.name` | The name of the `PipelineRun` that this `Pipeline` is running in. |
| `context.pipelineRun.namespace` | The namespace of the `PipelineRun` that this `Pipeline` is running in. |
Expand Down
9 changes: 8 additions & 1 deletion 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.

24 changes: 19 additions & 5 deletions pkg/apis/pipeline/v1beta1/resultref.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@ package v1beta1
import (
"fmt"
"regexp"
"strconv"
"strings"
)

// ResultRef is a type that represents a reference to a task run result
type ResultRef struct {
PipelineTask string `json:"pipelineTask"`
Result string `json:"result"`
ResultsIndex int `json:"resultsIndex"`
}

const (
Expand All @@ -35,28 +37,33 @@ const (
// ResultResultPart Constant used to define the "results" part of a pipeline result reference
ResultResultPart = "results"
// TODO(#2462) use one regex across all substitutions
variableSubstitutionFormat = `\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*\)`
// variableSubstitutionFormat matches format like $result.resultname, $result.resultname[int] and $result.resultname[*]
variableSubstitutionFormat = `\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9])*\*?\])?\)`
// arrayIndexing will match all `[int]` and `[*]` for parseExpression
arrayIndexing = `\[([0-9])*\*?\]`
// ResultNameFormat Constant used to define the the regex Result.Name should follow
ResultNameFormat = `^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$`
)

var variableSubstitutionRegex = regexp.MustCompile(variableSubstitutionFormat)
var resultNameFormatRegex = regexp.MustCompile(ResultNameFormat)
var arrayIndexingRegex = regexp.MustCompile(arrayIndexing)

// NewResultRefs extracts all ResultReferences from a param or a pipeline result.
// If the ResultReference can be extracted, they are returned. Expressions which are not
// results are ignored.
func NewResultRefs(expressions []string) []*ResultRef {
var resultRefs []*ResultRef
for _, expression := range expressions {
pipelineTask, result, err := parseExpression(expression)
pipelineTask, result, index, err := parseExpression(expression)
// If the expression isn't a result but is some other expression,
// parseExpression will return an error, in which case we just skip that expression,
// since although it's not a result ref, it might be some other kind of reference
if err == nil {
resultRefs = append(resultRefs, &ResultRef{
PipelineTask: pipelineTask,
Result: result,
ResultsIndex: index,
})
}
}
Expand Down Expand Up @@ -122,12 +129,19 @@ func stripVarSubExpression(expression string) string {
return strings.TrimSuffix(strings.TrimPrefix(expression, "$("), ")")
}

func parseExpression(substitutionExpression string) (string, string, error) {
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
Comment on lines +132 to +144
Copy link
Member

Choose a reason for hiding this comment

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

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?

}

// PipelineTaskResultRefs walks all the places a result reference can be used
Expand Down
21 changes: 21 additions & 0 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 @@ -41,6 +41,27 @@ func TestNewResultReference(t *testing.T) {
PipelineTask: "sumTask",
Result: "sumResult",
}},
}, {
name: "refer whole array result",
param: v1beta1.Param{
Name: "param",
Value: *v1beta1.NewArrayOrString("$(tasks.sumTask.results.sumResult[*])"),
},
want: []*v1beta1.ResultRef{{
PipelineTask: "sumTask",
Result: "sumResult",
}},
}, {
name: "refer array indexing result",
param: v1beta1.Param{
Name: "param",
Value: *v1beta1.NewArrayOrString("$(tasks.sumTask.results.sumResult[1])"),
},
want: []*v1beta1.ResultRef{{
PipelineTask: "sumTask",
Result: "sumResult",
ResultsIndex: 1,
}},
}, {
name: "substitution within string",
param: v1beta1.Param{
Expand Down
8 changes: 7 additions & 1 deletion pkg/apis/pipeline/v1beta1/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1524,7 +1524,8 @@
"type": "object",
"required": [
"pipelineTask",
"result"
"result",
"resultsIndex"
],
"properties": {
"pipelineTask": {
Expand All @@ -1534,6 +1535,11 @@
"result": {
"type": "string",
"default": ""
},
"resultsIndex": {
"type": "integer",
"format": "int32",
"default": 0
}
}
},
Expand Down
157 changes: 157 additions & 0 deletions pkg/reconciler/pipelinerun/resources/apply_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,67 @@ func TestApplyTaskResults_MinimalExpression(t *testing.T) {
}},
},
}},
}, {
Yongxuanzhang marked this conversation as resolved.
Show resolved Hide resolved
name: "Test array indexing result substitution on minimal variable substitution expression - 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"][1])`),
}},
},
}},
want: PipelineRunState{{
PipelineTask: &v1beta1.PipelineTask{
Name: "bTask",
TaskRef: &v1beta1.TaskRef{Name: "bTask"},
Params: []v1beta1.Param{{
Name: "bParam",
Value: *v1beta1.NewArrayOrString("arrayResultValueTwo"),
}},
},
}},
}, {
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])`),
}},
},
}},
Comment on lines +634 to +664
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}, {
name: "Test result substitution on minimal variable substitution expression - when expressions",
resolvedResultRefs: ResolvedResultRefs{{
Expand Down Expand Up @@ -633,6 +694,38 @@ func TestApplyTaskResults_MinimalExpression(t *testing.T) {
}},
},
}},
}, {
name: "Test array indexing result substitution on minimal variable substitution expression - when expressions",
resolvedResultRefs: ResolvedResultRefs{{
Value: *v1beta1.NewArrayOrString("arrayResultValueOne", "arrayResultValueTwo"),
ResultReference: v1beta1.ResultRef{
PipelineTask: "aTask",
Result: "aResult",
},
FromTaskRun: "aTaskRun",
}},
targets: PipelineRunState{{
PipelineTask: &v1beta1.PipelineTask{
Name: "bTask",
TaskRef: &v1beta1.TaskRef{Name: "bTask"},
WhenExpressions: []v1beta1.WhenExpression{{
Input: "$(tasks.aTask.results.aResult[1])",
Operator: selection.In,
Values: []string{"$(tasks.aTask.results.aResult[0])"},
}},
},
}},
want: PipelineRunState{{
PipelineTask: &v1beta1.PipelineTask{
Name: "bTask",
TaskRef: &v1beta1.TaskRef{Name: "bTask"},
WhenExpressions: []v1beta1.WhenExpression{{
Input: "arrayResultValueTwo",
Operator: selection.In,
Values: []string{"arrayResultValueOne"},
}},
},
}},
}} {
t.Run(tt.name, func(t *testing.T) {
ApplyTaskResults(tt.targets, tt.resolvedResultRefs)
Expand Down Expand Up @@ -679,6 +772,36 @@ func TestApplyTaskResults_EmbeddedExpression(t *testing.T) {
}},
},
}},
}, {
name: "Test array indexing result substitution on embedded variable substitution expression - params",
Yongxuanzhang marked this conversation as resolved.
Show resolved Hide resolved
resolvedResultRefs: ResolvedResultRefs{{
Value: *v1beta1.NewArrayOrString("arrayResultValueOne", "arrayResultValueTwo"),
ResultReference: v1beta1.ResultRef{
PipelineTask: "aTask",
Result: "aResult",
},
FromTaskRun: "aTaskRun",
}},
targets: PipelineRunState{{
PipelineTask: &v1beta1.PipelineTask{
Name: "bTask",
TaskRef: &v1beta1.TaskRef{Name: "bTask"},
Params: []v1beta1.Param{{
Name: "bParam",
Value: *v1beta1.NewArrayOrString("Result value --> $(tasks.aTask.results.aResult[0])"),
}},
},
}},
want: PipelineRunState{{
PipelineTask: &v1beta1.PipelineTask{
Name: "bTask",
TaskRef: &v1beta1.TaskRef{Name: "bTask"},
Params: []v1beta1.Param{{
Name: "bParam",
Value: *v1beta1.NewArrayOrString("Result value --> arrayResultValueOne"),
}},
},
}},
}, {
name: "Test result substitution on embedded variable substitution expression - when expressions",
resolvedResultRefs: ResolvedResultRefs{{
Expand Down Expand Up @@ -713,6 +836,40 @@ func TestApplyTaskResults_EmbeddedExpression(t *testing.T) {
}},
},
}},
}, {
name: "Test array indexing result substitution on embedded variable substitution expression - when expressions",
resolvedResultRefs: ResolvedResultRefs{{
Value: *v1beta1.NewArrayOrString("arrayResultValueOne", "arrayResultValueTwo"),
ResultReference: v1beta1.ResultRef{
PipelineTask: "aTask",
Result: "aResult",
},
FromTaskRun: "aTaskRun",
}},
targets: PipelineRunState{{
PipelineTask: &v1beta1.PipelineTask{
Name: "bTask",
TaskRef: &v1beta1.TaskRef{Name: "bTask"},
WhenExpressions: []v1beta1.WhenExpression{
{
Input: "Result value --> $(tasks.aTask.results.aResult[1])",
Operator: selection.In,
Values: []string{"Result value --> $(tasks.aTask.results.aResult[0])"},
},
},
},
}},
want: PipelineRunState{{
PipelineTask: &v1beta1.PipelineTask{
Name: "bTask",
TaskRef: &v1beta1.TaskRef{Name: "bTask"},
WhenExpressions: []v1beta1.WhenExpression{{
Input: "Result value --> arrayResultValueTwo",
Operator: selection.In,
Values: []string{"Result value --> arrayResultValueOne"},
}},
},
}},
}} {
t.Run(tt.name, func(t *testing.T) {
ApplyTaskResults(tt.targets, tt.resolvedResultRefs)
Expand Down
6 changes: 4 additions & 2 deletions pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Yongxuanzhang Yongxuanzhang Jun 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

if err != nil && (t.IsFinalTask(facts) || rprt.Skip(facts).SkippingReason == v1beta1.WhenExpressionsSkip) {
return true
}
}
ApplyTaskResults(PipelineRunState{t}, resolvedResultRefs)
facts.ResetSkippedCache()
Expand Down
Loading