Add actual validation to the task validation webhook skeleton. #117
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlorenc 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
the
approved
knative-prow-robot
added
the
size/L
dlorenc
force-pushed
the
moretaskvalidation
branch
3 times, most recently
from
0a66647 to
c3ccab0
Compare
|
Got this working and tested in my cluster: Changing the type in build_task.yaml to "what" from "git" gives: Error from server (InternalError): error when creating "examples/build_task.yaml": Internal error occurred: admission webhook "webhook.pipeline.knative.dev" denied the request: mutation failed: invalid value "what": taskspec.Outputs.Sources.builtImage.TypeI'm not in love with the duplicate error, but I copied the validation from build: https://github.com/knative/build/blob/c5d66f1ca208fa2ea3408eb46f0328e4cd390bd1/pkg/apis/build/v1alpha1/build_template_validation.go#L50 That looks like: Error from server (InternalError): error when creating "examples/build_task.yaml": Internal error occurred: admission webhook "webhook.pipeline.knative.dev" denied the request: mutation failed: expected exactly one, got both: taskspec.Outputs.Sources.Name |
| }, | ||
| }, | ||
| }, | ||
| wantErr: true, |
There was a problem hiding this comment.
what do you think about having a separate set of tests for the failure/success conditions vs. having a boolean flag to indicate failure/success?
i find it easier to read tests that are written that way and then the logic in the test itself doesn't have to account for 2 fairly different cases
There was a problem hiding this comment.
Lookin good! Do you have any more validation planned? Looking at the Types, Clusters is a possibility, tho we'll probably be changing that significantly in #68 anyway
| fields: fields{ | ||
| Outputs: &Outputs{ | ||
| Sources: []Source{validSource}, | ||
| }, |
There was a problem hiding this comment.
i'm surprised we can create Tasks without buildSpecs - I guess we're assuming that will be caught by the serialization/deserialization?
There was a problem hiding this comment.
Yeah not sure, I'll play around with it.
dlorenc
force-pushed
the
moretaskvalidation
branch
from
c3ccab0 to
e61315a
Compare
Yeah, there's a lot more we can validate here. We'll keep going on this. |
|
lookin good! /lgtm |
|
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. |
|
ruh roh |
|
/me backs away slowly |
This PR adds the following validation: - Task inputs and outputs must have a valid Type - Task inputs and outputs must not have a duplicated name.
dlorenc
force-pushed
the
moretaskvalidation
branch
from
e7785b6 to
bda8a34
Compare
|
Ahh, looks like everything got renamed. Rebased and renamed stuff. |
This PR adds the following validation:
Ref #29