TEP-0118: Add validation for Matrix.Include.Params of type string #6219
Conversation
tekton-robot
added
kind/feature
|
The following is the coverage report on the affected files.
|
EmmaMunley
force-pushed
the
ValidateParamsTypesInMatrix
branch
from
ef7461a
to
efae66d
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
EmmaMunley
force-pushed
the
ValidateParamsTypesInMatrix
branch
from
efae66d
to
581b35d
Compare
tekton-robot
added
size/XL
| // MatrixHasInclude return whether matrix has Include | ||
| func (matrix *Matrix) MatrixHasInclude() bool { | ||
| return len(matrix.Include) > 0 | ||
| } | ||
|
|
||
| // MatrixHasParams return whether matrix has Params | ||
| func (matrix *Matrix) MatrixHasParams() bool { | ||
| return len(matrix.Params) > 0 |
There was a problem hiding this comment.
do these functions have to be exported? it seems that they are only used within this package
pkg/apis/pipeline/v1/param_types.go
Outdated
| @@ -314,22 +314,51 @@ func validatePipelineParametersVariablesInTaskParameters(params []Param, prefix | |||
| return errs | |||
| } | |||
|
|
|||
| // validatePipelineParametersVariablesInMatrixParameters validates matrix param value | |||
| // validatePipelineParametersVariablesInMatrixParameters validates all Matrix Params including Matrix.Params and Matrix.Include.Params | |||
There was a problem hiding this comment.
| // validatePipelineParametersVariablesInMatrixParameters validates all Matrix Params including Matrix.Params and Matrix.Include.Params | |
| // validatePipelineParametersVariablesInMatrixParameters validates all params in matrix.params and matrix.include.params |
pkg/apis/pipeline/v1/param_types.go
Outdated
| // ValidateParamsTypesInMatrix validates the type of parameter | ||
| // for Matrix.Params and Matrix.Include.Params | ||
| // Matrix.Params must be of type array. Matrix.Include.Params must be of type string. | ||
| func ValidateParamsTypesInMatrix(matrix *Matrix) (errs *apis.FieldError) { |
There was a problem hiding this comment.
why is this function changed from unexported to exported?
pkg/apis/pipeline/v1/param_types.go
Outdated
| if matrix.MatrixHasInclude() { | ||
| for _, include := range matrix.Include { | ||
| for _, param := range include.Params { | ||
| // Validate Matrix.Include.Params are of type string |
There was a problem hiding this comment.
this comment is not necessary because the code is easy to read
|
|
||
| func TestValidateParamsTypesInMatrix(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| matrix *v1.Matrix | ||
| wantErrs *apis.FieldError | ||
| }{{ | ||
| name: "parameters in matrix are strings", | ||
| matrix: &v1.Matrix{ | ||
| Params: []v1.Param{{ | ||
| Name: "foo", Value: v1.ParamValue{Type: v1.ParamTypeString, StringVal: "foo"}, | ||
| }, { | ||
| Name: "bar", Value: v1.ParamValue{Type: v1.ParamTypeString, StringVal: "bar"}, | ||
| }}}, | ||
| wantErrs: &apis.FieldError{ | ||
| Message: "invalid value: parameters of type array only are allowed in matrix", | ||
| Paths: []string{"matrix[foo]", "matrix[bar]"}, | ||
| }, | ||
| }, { | ||
| name: "parameters in matrix are arrays", | ||
| matrix: &v1.Matrix{ | ||
| Params: []v1.Param{{ | ||
| Name: "foobar", Value: v1.ParamValue{Type: v1.ParamTypeArray, ArrayVal: []string{"foo", "bar"}}, | ||
| }, { | ||
| Name: "barfoo", Value: v1.ParamValue{Type: v1.ParamTypeArray, ArrayVal: []string{"bar", "foo"}}, | ||
| }}}, | ||
| }, { | ||
| name: "parameters in include matrix are strings", | ||
| matrix: &v1.Matrix{ | ||
| Include: []v1.MatrixInclude{ | ||
| {Name: "build-1"}, | ||
| {Params: []v1.Param{ | ||
| {Name: "IMAGE", Value: v1.ParamValue{Type: v1.ParamTypeString, StringVal: "image-1"}}, | ||
| {Name: "DOCKERFILE", Value: v1.ParamValue{Type: v1.ParamTypeString, StringVal: "path/to/Dockerfile1"}}}}, | ||
| }}, | ||
| }, { | ||
| name: "parameters in include matrix are arrays", | ||
| matrix: &v1.Matrix{ | ||
| Include: []v1.MatrixInclude{ | ||
| {Name: "build-1"}, | ||
| {Params: []v1.Param{ | ||
| {Name: "foobar", Value: v1.ParamValue{Type: v1.ParamTypeArray, ArrayVal: []string{"foo", "bar"}}}, | ||
| {Name: "barfoo", Value: v1.ParamValue{Type: v1.ParamTypeArray, ArrayVal: []string{"bar", "foo"}}}}}, | ||
| }}, | ||
| wantErrs: &apis.FieldError{ | ||
| Message: "invalid value: parameters of type string only are allowed in matrix", | ||
| Paths: []string{"matrix[barfoo]", "matrix[foobar]"}, | ||
| }, | ||
| }, | ||
| } | ||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| if d := cmp.Diff(tc.wantErrs.Error(), v1.ValidateParamsTypesInMatrix(tc.matrix).Error()); d != "" { | ||
| t.Errorf("ValidateParamsTypesInMatrix() errors diff %s", diff.PrintWantGot(d)) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
we have a test for matrix validations in
pipeline/pkg/apis/pipeline/v1/pipeline_types_test.go
Lines 592 to 702 in d2f075a
adds duplication, please add new test cases to the existing validations test
EmmaMunley
force-pushed
the
ValidateParamsTypesInMatrix
branch
from
581b35d
to
7a88a35
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Thanks @EmmaMunley!
- Update the title that it's doing all validations, not just checking the type
- Update the commit message with some context about what
matrix.includeis, before explaining the validations formatrix.include - Improve the test coverage
- Add relevant documentation
- I'd prefer if the different validations were in separate PRs because this is now XL (but feel free to have all validations in one PR if you prefer)
…ferences to pipeline parameter variables, and combination count This commit validates that: 1) Parameters in Matrix.Include are of type string 2) The parameter only exist in either matrix.Params or pipelineTask.Params 3) The matrix params and include params does not exceed the max combination count Note: Other forms of validation and implementation logic will be added in subsequent commits.
EmmaMunley
force-pushed
the
ValidateParamsTypesInMatrix
branch
from
7a88a35
to
7ecf8a4
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
size/XXL
|
/hold Will break up into smaller PRs |
tekton-robot
added
the
do-not-merge/hold
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
@EmmaMunley: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
tekton-robot
added
the
needs-rebase
|
@EmmaMunley: PR needs rebase. 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. |
|
Closing this out. This PR has been broken up into the following PRs that have been opened: TEP-0118: Added Matrix.Include field in preview mode #6188 |
Changes
This commit validates that:
Note: Other forms of validation and implementation logic will be added in subsequent commits.
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes