Skip to content

Commit

Permalink
TEP-0118: Add validation for matrix pipeline parameter variables incl…
Browse files Browse the repository at this point in the history 
…ude Matrix.Include.Params

[TEP-0090: Matrix] introduced `Matrix` to the `PipelineTask` specification such that the `PipelineTask` executes a list of `TaskRuns` or `Runs` in parallel with the specified list of inputs for a `Parameter` or with different combinations of the inputs for a set of `Parameters`.

To build on this, Tep-0018 introduced Matrix.Include, which allows passing in a specific combinations of `Parameters` into the `Matrix`.

This commit validates that matrix pipeline parameter variables  including include params that may contain the reference(s) to other params are used appropriately.

Note: This is still in preview mode. Other forms of validation and implementation logic will be added in subsequent commits.
  • Loading branch information
@EmmaMunley
EmmaMunley committed Mar 1, 2023
1 parent 4ee897b commit db65e9f
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 134 deletions.
22 changes: 17 additions & 5 deletions pkg/apis/pipeline/v1/param_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -314,12 +314,24 @@ func validatePipelineParametersVariablesInTaskParameters(params []Param, prefix
return errs
}

// validatePipelineParametersVariablesInMatrixParameters validates matrix param value
// validatePipelineParametersVariablesInMatrixParameters validates all pipeline paramater variables including Matrix.Params and Matrix.Include.Params
// that may contain the reference(s) to other params to make sure those references are used appropriately.
func validatePipelineParametersVariablesInMatrixParameters(matrix []Param, prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
for _, param := range matrix {
for idx, arrayElement := range param.Value.ArrayVal {
errs = errs.Also(validateArrayVariable(arrayElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("value", idx).ViaFieldKey("matrix", param.Name))
func validatePipelineParametersVariablesInMatrixParameters(matrix *Matrix, prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
if matrix != nil {
if matrix.MatrixHasInclude() {
for _, include := range matrix.Include {
for idx, param := range include.Params {
stringElement := param.Value.StringVal
errs = errs.Also(validateStringVariable(stringElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("value", idx).ViaFieldKey("matrix.include.params", param.Name))
}
}
}
if matrix.MatrixHasParams() {
for _, param := range matrix.Params {
for idx, arrayElement := range param.Value.ArrayVal {
errs = errs.Also(validateArrayVariable(arrayElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("value", idx).ViaFieldKey("matrix.params", param.Name))
}
}
}
}
return errs
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1/pipeline_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ func validatePipelineParametersVariables(tasks []PipelineTask, prefix string, pa
for idx, task := range tasks {
errs = errs.Also(validatePipelineParametersVariablesInTaskParameters(task.Params, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
if task.IsMatrixed() {
errs = errs.Also(validatePipelineParametersVariablesInMatrixParameters(task.Matrix.Params, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
errs = errs.Also(validatePipelineParametersVariablesInMatrixParameters(task.Matrix, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
}
errs = errs.Also(task.When.validatePipelineParametersVariables(prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
}
Expand Down
71 changes: 10 additions & 61 deletions pkg/apis/pipeline/v1/pipeline_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1550,7 +1550,7 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) {
}},
expectedError: apis.FieldError{
Message: `non-existent variable in "$(params.does-not-exist)"`,
Paths: []string{"[0].matrix[a-param].value[0]"},
Paths: []string{"[0].matrix.params[a-param].value[0]"},
},
}, {
name: "invalid pipeline task with a matrix parameter combined with missing param from the param declarations",
Expand All @@ -1567,7 +1567,7 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) {
}},
expectedError: apis.FieldError{
Message: `non-existent variable in "$(params.does-not-exist)"`,
Paths: []string{"[0].matrix[a-param].value[2]"},
Paths: []string{"[0].matrix.params[a-param].value[2]"},
},
}, {
name: "invalid pipeline task with two matrix parameters and one of them missing from the param declarations",
Expand All @@ -1586,7 +1586,7 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) {
}},
expectedError: apis.FieldError{
Message: `non-existent variable in "$(params.does-not-exist)"`,
Paths: []string{"[0].matrix[b-param].value[0]"},
Paths: []string{"[0].matrix.params[b-param].value[0]"},
},
}, {
name: "invalid object key in the input of the when expression",
Expand Down Expand Up @@ -1730,7 +1730,7 @@ func TestValidatePipelineParameterVariables_Failure(t *testing.T) {
}},
expectedError: apis.FieldError{
Message: `non-existent variable in "$(params.myObject.non-exist-key)"`,
Paths: []string{"[0].matrix[b-param].value[0]"},
Paths: []string{"[0].matrix.params[b-param].value[0]"},
},
api: "alpha",
}, {
Expand Down Expand Up @@ -2419,9 +2419,6 @@ func TestContextValid(t *testing.T) {
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{StringVal: "$(context.pipeline.name)"},
}},
Matrix: &Matrix{
Params: []Param{{
Name: "a-param-mat", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.name)"}},
Expand All @@ -2432,9 +2429,6 @@ func TestContextValid(t *testing.T) {
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineRun.name)"},
}},
Matrix: &Matrix{
Params: []Param{{
Name: "a-param-mat", Value: ParamValue{ArrayVal: []string{"$(context.pipelineRun.name)"}},
Expand All @@ -2445,9 +2439,6 @@ func TestContextValid(t *testing.T) {
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineRun.namespace)"},
}},
Matrix: &Matrix{
Params: []Param{{
Name: "a-param-mat", Value: ParamValue{ArrayVal: []string{"$(context.pipelineRun.namespace)"}},
Expand All @@ -2458,9 +2449,6 @@ func TestContextValid(t *testing.T) {
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineRun.uid)"},
}},
Matrix: &Matrix{
Params: []Param{{
Name: "a-param-mat", Value: ParamValue{ArrayVal: []string{"$(context.pipelineRun.uid)"}},
Expand All @@ -2471,9 +2459,6 @@ func TestContextValid(t *testing.T) {
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.name)", "and", "$(context.pipelineRun.name)"}},
}},
Matrix: &Matrix{
Params: []Param{{
Name: "a-param-mat", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.name)", "and", "$(context.pipelineRun.name)"}},
Expand All @@ -2484,9 +2469,6 @@ func TestContextValid(t *testing.T) {
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineTask.retries)"},
}},
Matrix: &Matrix{
Params: []Param{{
Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineTask.retries)"},
Expand All @@ -2497,9 +2479,6 @@ func TestContextValid(t *testing.T) {
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{ArrayVal: []string{"$(context.pipelineTask.retries)"}},
}},
Matrix: &Matrix{
Params: []Param{{
Name: "a-param-mat", Value: ParamValue{ArrayVal: []string{"$(context.pipelineTask.retries)"}},
Expand All @@ -2510,9 +2489,6 @@ func TestContextValid(t *testing.T) {
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{StringVal: "$(context.pipeline.name)"},
}},
Matrix: &Matrix{
Include: []MatrixInclude{{
Name: "build-1",
Expand All @@ -2525,9 +2501,6 @@ func TestContextValid(t *testing.T) {
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineTask.retries)"},
}},
Matrix: &Matrix{
Include: []MatrixInclude{{
Name: "build-1",
Expand All @@ -2545,7 +2518,7 @@ func TestContextValid(t *testing.T) {
}
}

func TestContextInvalid(t *testing.T) {
func TestContextInvalidMatrix(t *testing.T) {
tests := []struct {
name string
tasks []PipelineTask
Expand All @@ -2555,80 +2528,56 @@ func TestContextInvalid(t *testing.T) {
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{StringVal: "$(context.pipeline.missing)"},
}},
Matrix: &Matrix{
Params: []Param{{
Name: "a-param-foo", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.missing-foo)"}},
Name: "a-param-foo", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.missing)"}},
}}},
}},
expectedError: *apis.ErrGeneric("").Also(&apis.FieldError{
Message: `non-existent variable in "$(context.pipeline.missing)"`,
Paths: []string{"value"},
}).Also(&apis.FieldError{
Message: `non-existent variable in "$(context.pipeline.missing-foo)"`,
Paths: []string{"value"},
}),
}, {
name: "invalid string context variable for pipelineRun",
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineRun.missing)"},
}},
Matrix: &Matrix{
Params: []Param{{
Name: "a-param-foo", Value: ParamValue{ArrayVal: []string{"$(context.pipelineRun.missing-foo)"}},
Name: "a-param-foo", Value: ParamValue{ArrayVal: []string{"$(context.pipelineRun.missing)"}},
}}},
}},
expectedError: *apis.ErrGeneric("").Also(&apis.FieldError{
Message: `non-existent variable in "$(context.pipelineRun.missing)"`,
Paths: []string{"value"},
}).Also(&apis.FieldError{
Message: `non-existent variable in "$(context.pipelineRun.missing-foo)"`,
Paths: []string{"value"},
}),
}, {
name: "invalid string context variable for pipelineTask",
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{StringVal: "$(context.pipelineTask.missing)"},
}},
Matrix: &Matrix{
Params: []Param{{
Name: "a-param-foo", Value: ParamValue{ArrayVal: []string{"$(context.pipelineTask.missing-foo)"}},
Name: "a-param-foo", Value: ParamValue{ArrayVal: []string{"$(context.pipelineTask.missing)"}},
}}},
}},
expectedError: *apis.ErrGeneric("").Also(&apis.FieldError{
Message: `non-existent variable in "$(context.pipelineTask.missing)"`,
Paths: []string{"value"},
}).Also(&apis.FieldError{
Message: `non-existent variable in "$(context.pipelineTask.missing-foo)"`,
Paths: []string{"value"},
}),
}, {
name: "invalid array context variables for pipeline, pipelineTask and pipelineRun",
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.missing)", "$(context.pipelineTask.missing)", "$(context.pipelineRun.missing)"}},
}},
Matrix: &Matrix{
Params: []Param{{
Name: "a-param", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.missing-foo)", "$(context.pipelineTask.missing-foo)", "$(context.pipelineRun.missing-foo)"}},
Name: "a-param", Value: ParamValue{ArrayVal: []string{"$(context.pipeline.missing)", "$(context.pipelineTask.missing)", "$(context.pipelineRun.missing)"}},
}}},
}},
expectedError: *apis.ErrGeneric(`non-existent variable in "$(context.pipeline.missing)"`, "value").
Also(apis.ErrGeneric(`non-existent variable in "$(context.pipelineRun.missing)"`, "value")).
Also(apis.ErrGeneric(`non-existent variable in "$(context.pipelineTask.missing)"`, "value")).
Also(apis.ErrGeneric(`non-existent variable in "$(context.pipeline.missing-foo)"`, "value")).
Also(apis.ErrGeneric(`non-existent variable in "$(context.pipelineRun.missing-foo)"`, "value")).
Also(apis.ErrGeneric(`non-existent variable in "$(context.pipelineTask.missing-foo)"`, "value")),
Also(apis.ErrGeneric(`non-existent variable in "$(context.pipelineTask.missing)"`, "value")),
}, {
name: "invalid string context variable for pipeline in include matrix",
tasks: []PipelineTask{{
Expand Down
22 changes: 17 additions & 5 deletions pkg/apis/pipeline/v1beta1/param_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -310,12 +310,24 @@ func validatePipelineParametersVariablesInTaskParameters(params []Param, prefix
return errs
}

// validatePipelineParametersVariablesInMatrixParameters validates matrix param value
// validatePipelineParametersVariablesInMatrixParameters validates all pipeline paramater variables including Matrix.Params and Matrix.Include.Params
// that may contain the reference(s) to other params to make sure those references are used appropriately.
func validatePipelineParametersVariablesInMatrixParameters(matrix []Param, prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
for _, param := range matrix {
for idx, arrayElement := range param.Value.ArrayVal {
errs = errs.Also(validateArrayVariable(arrayElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("value", idx).ViaFieldKey("matrix", param.Name))
func validatePipelineParametersVariablesInMatrixParameters(matrix *Matrix, prefix string, paramNames sets.String, arrayParamNames sets.String, objectParamNameKeys map[string][]string) (errs *apis.FieldError) {
if matrix != nil {
if matrix.MatrixHasInclude() {
for _, include := range matrix.Include {
for idx, param := range include.Params {
stringElement := param.Value.StringVal
errs = errs.Also(validateStringVariable(stringElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("value", idx).ViaFieldKey("matrix.include.params", param.Name))
}
}
}
if matrix.MatrixHasParams() {
for _, param := range matrix.Params {
for idx, arrayElement := range param.Value.ArrayVal {
errs = errs.Also(validateArrayVariable(arrayElement, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaFieldIndex("value", idx).ViaFieldKey("matrix.params", param.Name))
}
}
}
}
return errs
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1beta1/pipeline_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ func validatePipelineParametersVariables(tasks []PipelineTask, prefix string, pa
for idx, task := range tasks {
errs = errs.Also(validatePipelineParametersVariablesInTaskParameters(task.Params, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
if task.IsMatrixed() {
errs = errs.Also(validatePipelineParametersVariablesInMatrixParameters(task.Matrix.Params, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
errs = errs.Also(validatePipelineParametersVariablesInMatrixParameters(task.Matrix, prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
}
errs = errs.Also(task.WhenExpressions.validatePipelineParametersVariables(prefix, paramNames, arrayParamNames, objectParamNameKeys).ViaIndex(idx))
}
Expand Down
Loading

0 comments on commit db65e9f

Please sign in to comment.