Task validation : fix variable validation in case of multiple variables 👼 #472
Conversation
googlebot
added
the
cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
knative-prow-robot
added
approved
vdemeester
force-pushed
the
fix-validation-args
branch
from
8d85eae to
a2384ce
Compare
| @@ -27,6 +27,8 @@ import ( | |||
| "k8s.io/apimachinery/pkg/util/validation" | |||
| ) | |||
|
|
|||
| const substitution = "[_a-zA-Z][_a-zA-Z0-9.-]*" | |||
There was a problem hiding this comment.
I think this is a good "base" for variable name but might be too restrictive though
There was a problem hiding this comment.
seems fine to me, it allows - and _
There was a problem hiding this comment.
nice job @vdemeester 👍
Can you just add in the docs the allowed rules for naming variables in templating so it passes the regex?
| @@ -27,6 +27,8 @@ import ( | |||
| "k8s.io/apimachinery/pkg/util/validation" | |||
| ) | |||
|
|
|||
| const substitution = "[_a-zA-Z][_a-zA-Z0-9.-]*" | |||
There was a problem hiding this comment.
seems fine to me, it allows - and _
|
Nice! I’m gonna pull this over to #471 as well. =) |
…es 👼
There was a problem if a single string contains more than one
variables when validating it. For example, the following Task would
fail validation at creation.
```yaml
apiVersion: pipeline.knative.dev/v1alpha1
kind: Task
metadata:
name: foo
spec:
inputs:
params:
- name: foo
description: foo is FOO
default: banana
- name: bar
description: bar is BAR
default: baz
steps:
- name: run-me
image: busybok
command: ["/bin/sh", "-c"]
args: ["echo I am a ${inputs.params.foo} named ${inputs.params.baz}"]
```
The regular expression used was too naive, it is now fixed. Before
this change, the extracted variable would be:
`inputs param.foo} named ${inputs.params.baz`
… and thus, the validation would fail.
With this fix, it extracts both variables and validate both.
Signed-off-by: Vincent Demeester <[email protected]>
vdemeester
force-pushed
the
fix-validation-args
branch
from
a2384ce to
0b756b8
Compare
|
The following is the coverage report on pkg/.
|
|
/lgtm |
|
nice catch @vdemeester 👊 |
There was a problem if a single string contains more than one
variables when validating it. For example, the following Task would
fail validation at creation.
The regular expression used was too naive, it is now fixed. Before
this change, the extracted variable would be:
inputs param.foo} named ${inputs.params.baz… and thus, the validation would fail.
With this fix, it extracts both variables and validate both.
/cc @bobcatfish @shashwathi
Signed-off-by: Vincent Demeester [email protected]