Search and Replace: Validate against openAPI#1228
Search and Replace: Validate against openAPI#1228phanimarupaka wants to merge 1 commit intokptdev:masterfrom
Conversation
phanimarupaka
force-pushed
the
ValidateAgainstOpenAPI
branch
from
9006651 to
e063b0b
Compare
| github.com/cpuguy83/go-md2man/v2 v2.0.0 | ||
| github.com/go-errors/errors v1.0.1 | ||
| github.com/go-openapi/spec v0.19.5 | ||
| github.com/instrumenta/kubeval v0.0.0-20201118090229-529b532b1ea1 |
There was a problem hiding this comment.
Can we depend on a proper release instead of HEAD?
There was a problem hiding this comment.
I followed the instructions here https://www.kubeval.com/go/ and this is what go imported. When I tried their release tag it doesn't work. How is it different from https://github.com/GoogleContainerTools/kpt/blob/master/go.mod#L16.
There was a problem hiding this comment.
It is not different. Just in general we should try to depend on proper releases. But if that doesn't work, then we don't have any choice.
| PutLiteral: r.PutLiteral, | ||
| PutPattern: r.PutPattern, | ||
| PackagePath: pkgPath, | ||
| Validate: r.Validate, |
There was a problem hiding this comment.
Does the validation step need to be part of SearchReplace? It seems like it is basically a step we perform after we have made the change. Could we implement this as a separate step after we have run SearchReplace? It would make the solution more modular and it would prevent adding too much functionality into SearchReplace.
I'm also wondering if this is something that could be supported with a function instead of adding the functionality directly into kpt.
There was a problem hiding this comment.
It seems like it is basically a step we perform after we have made the change => We throw an error and not write the result back if there are errors. So it is a part of Search and Replace operation.
We can support this as a function but this feature would need the validation. By including this option in Search and Replace, users can get immediate feedback when they replace field values.
phanimarupaka
force-pushed
the
ValidateAgainstOpenAPI
branch
from
e063b0b to
a542003
Compare
| } | ||
|
|
||
| if sr.Validate { | ||
| validateResult, err := ValidateNode(object) |
There was a problem hiding this comment.
So this seems to separate the validation of fields from the process of determining the correct type. And the tests seems to all focus on validation errors involving incorrect types. Should we do validation at the same time as we determine the type or are there validation errors we can't catch at that point?
This also seems to do validation on the complete package. Do we want to do that? It seems like maybe this should work for users even if there might be other fields in any of the resources that doesn't validate.
|
|
||
| // Filter parses input node and performs search and replace operation on the node | ||
| func (sr *SearchReplace) Filter(object *yaml.RNode) (*yaml.RNode, error) { | ||
| originalObject, err := deepCopy(object) |
There was a problem hiding this comment.
What should be the level at which we roll back changes if validation fails? It seems like we do in on a per-resource basis here, but it's not obvious that we shouldn't handle this on a per-package or package+subpackages basis.
| } | ||
|
|
||
| // filterAll runs the yaml.Filter against all inputs | ||
| func filterAll(filter yaml.Filter) kio.Filter { |
There was a problem hiding this comment.
Might be worth explaining why we need this and how it is different from the kio.FilterAll function: https://github.com/kubernetes-sigs/kustomize/blob/052a6b4e967b8cba7f9c12623c9dd53fb16e2f7a/kyaml/kio/kio.go#L139
2 participants
@mortent
This PR is to validate the user input against openAPI schema during Search and Replace. This PR uses kubeval library to do so.
Future scope:
Passing in the custom openAPI schema to kubeval library. (Can be done after Natasha's work)
Input type interpretation to validate. kubernetes-sigs/kustomize#2594