-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
Fix test cases for validatePipelineParameterVariables function #4901
Conversation
Prior to this commit, many success tests were meaningless because they just use the param names in the format of `$(baz)`, `$(foo-is-baz)`. In validation, without using the format of `$(params.<Name>)`, the two examples above will be treated as just string literal instead of param reference, and thus in fact validation agains using correct params are not executed. That explains why there is no problem even though declared param is actually called `baz`, but using `$(bazzzz)`. In this commit, the usage of param names is corrected to `$(params.<Name>)` and type for ArrayOrString is added so that the param reference is really validated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! I'm curious if the type: ParamTypeString
is needed, since users don't have to declare the param type if it is a string
Yes, that's needed here in the test because it's used in In yaml file, users don't have to specify the param type because the unmarshal operation will infer its type and set it i.e. the following declaration will have
the following declaration will have
the following declaration will have
|
/assign @ywluogg |
Link this to #4723 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lbernick, ywluogg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm Merge won't go through until #4944 has merged. |
/test pull-tekton-pipeline-integration-tests |
/test pull-tekton-pipeline-integration-tests |
/test pull-tekton-pipeline-build-tests |
/retest |
1 similar comment
/retest |
/test pull-tekton-pipeline-alpha-integration-tests |
2 similar comments
/test pull-tekton-pipeline-alpha-integration-tests |
/test pull-tekton-pipeline-alpha-integration-tests |
/kind misc |
Changes
Prior to this commit, many success tests were meaningless because they
just use the param names in the format of
$(baz)
,$(foo-is-baz)
.In validation, without using the format of
$(params.<Name>)
, thetwo examples above will be treated as just string literal instead of param
reference, and thus in fact validation agains using correct params
are not executed. That explains why there is no problem even though
declared param is actually called
baz
, but using$(bazzzz)
.In this commit, the usage of param names is corrected to
$(params.<Name>)
and type for ArrayOrString is added so that the param reference is really
validated.
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