TEP-0075: Validate Pipeline object variables in value, matrix and when #4902
Conversation
tekton-robot
added
the
release-note
tekton-robot
added
the
size/L
|
The following is the coverage report on the affected files.
|
chuangw6
force-pushed
the
param-variable-validation
branch
from
099f6ac to
ec33611
Compare
|
The following is the coverage report on the affected files.
|
chuangw6
force-pushed
the
param-variable-validation
branch
from
ec33611 to
1fb40aa
Compare
|
The following is the coverage report on the affected files.
|
chuangw6
force-pushed
the
param-variable-validation
branch
from
1fb40aa to
d667c5c
Compare
|
The following is the coverage report on the affected files.
|
chuangw6
force-pushed
the
param-variable-validation
branch
2 times, most recently
from
d47bf45 to
46f5e8c
Compare
|
The following is the coverage report on the affected files.
|
|
@chuangw6: GitHub didn't allow me to assign the following users: ywluogg. Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this: 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. |
|
/lgtm |
|
/kind misc |
Chatted with @ywluogg offline. |
There was a problem hiding this comment.
Could you please update your commit message to make it more clear what is changing as a result of your PR? e.g. "Add validation for usage of parameter variables in Pipeline parameters, matrixes, and when expressions. Valid usage includes.... Invalid usage includes...."
|
The following is the coverage report on the affected files.
|
chuangw6
force-pushed
the
param-variable-validation
branch
from
5f31d26 to
a630923
Compare
|
The following is the coverage report on the affected files.
|
chuangw6
force-pushed
the
param-variable-validation
branch
from
a630923 to
83bccd0
Compare
|
The following is the coverage report on the affected files.
|
pkg/substitution/substitution.go
Outdated
| parameterSubstitution = `.*?(\[\*\])?` | ||
|
|
||
| // BraceMatchingRegex is a regex for parameter references including dot notation, bracket notation with single and double quotes. | ||
| BraceMatchingRegex = "(\\$(\\(%s(\\.(?P<var1>%s)|\\[\"(?P<var2>%s)\"\\]|\\['(?P<var3>%s)'\\])\\)))" |
There was a problem hiding this comment.
could you avoid exporting this, and instead create an exported helper function?
There was a problem hiding this comment.
I think the purpose of this package is to validate and parse parameters (it's hard to tell haha-- substitution is a bit messy), so the reason I suggest writing a helper function is to think about what validation and parsing can be abstracted away. The functions exported by a package form a sort of API that other packages can use. Imagine if you encountered an API with this getter function-- I think it would be confusing. This is where a package just for params would be really helpful! Since we don't have that, could you try to write a helper function that's consistent with what the substitution package is designed for? I am hoping to avoid the use of regexes in many places in the codebase because they are very hard to understand.
There was a problem hiding this comment.
Thank you @lbernick for the explanation. That makes sense to me! Done.
chuangw6
force-pushed
the
param-variable-validation
branch
from
83bccd0 to
7277716
Compare
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-alpha-integration-tests |
chuangw6
force-pushed
the
param-variable-validation
branch
2 times, most recently
from
deebc5b to
229ee8b
Compare
Besides task steps, Parameter variables can be used as the value of a param, matrix or when expression. Those validations regarding object type are added: - The whole object value can only be used when providing values for other object param. example: ``` params: - name: arg value: $(params.myObject[*]) ``` - Individual object attributes should be used in all other cases (string/array val, matrix and when expression).
chuangw6
force-pushed
the
param-variable-validation
branch
from
229ee8b to
4525854
Compare
|
The following is the coverage report on the affected files.
|
|
/lgtm |
|
/test pull-tekton-pipeline-build-tests |
|
The following is the coverage report on the affected files.
|
|
/test pull-pipeline-kind-k8s-v1-21-e2e |
|
@chuangw6: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
|
The following is the coverage report on the affected files.
|
|
/test pull-pipeline-kind-k8s-v1-21-e2e |
|
@chuangw6: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
Changes
Besides task steps, parameter variables can be used as the value of
another param, matrix or when expression.
for other object param. example:
$(params.myobject.key1).Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
(if there are no user facing changes, use release note "NONE")
Release Notes