Add CEL validation to validate subcommand
#5326
Conversation
Signed-off-by: ezgidemirel <ezgidemirel91@gmail.com>
There was a problem hiding this comment.
Left a few comments, but we can mostly address them in follow up PRs, functionality-wise LGTM! Thanks @ezgidemirel!
| fmt.Printf("[✓] %s, %s validated successfully\n", r.GroupVersionKind().String(), r.GetAnnotations()[composite.AnnotationKeyCompositionResourceName]) | ||
| } else { | ||
| failure++ | ||
| s := structurals[gvk] // if we have a schema validator, we should also have a structural |
There was a problem hiding this comment.
feels like we could merge the two maps by having a dedicated struct wrapping both the schemaValidator and the then CEL validator
| func getResourceName(r *unstructured.Unstructured) string { | ||
| if r.GetName() != "" { | ||
| return r.GetName() | ||
| } | ||
|
|
||
| // fallback to composition resource name | ||
| return r.GetAnnotations()[composite.AnnotationKeyCompositionResourceName] | ||
| } |
There was a problem hiding this comment.
what do you think about the following logic instead:
- no name, no annotation =>
"" - no name, but annotation =>
"" (annotation) - name, no annotation =>
"name" - name and annotation =>
"name" (annotation)
both name and annotation could be useful to function users as they can now set the name of resources too.
There was a problem hiding this comment.
I prefer keeping it simple and not making the output hard to read. Since there can be only one resource with the same name and GVK, users can check the annotation if they want.
There was a problem hiding this comment.
I don't think it would be hard to read and in a scenario where I'm piping the render output into this there would be no manifests to check, I'd have to rerun the command without piping to see both.
At least I'd quote (%q) the name so that if both the annotation and the name are missing you don get a dangling comma and it's easier to see the name is missing: iam.aws.upbound.io/v1beta1, Kind=User, validated successfully => iam.aws.upbound.io/v1beta1, Kind=User, "" validated successfully.
ezgidemirel
force-pushed
the
validate-cel
branch
2 times, most recently
from
021d3b1
to
45628bb
Compare
Signed-off-by: ezgidemirel <ezgidemirel91@gmail.com>
ezgidemirel
force-pushed
the
validate-cel
branch
from
45628bb
to
7edf5fe
Compare
| errWriteOutput = "cannot write output" | ||
| ) | ||
|
|
||
| func newValidatorsAndStructurals(crds []*extv1.CustomResourceDefinition) (map[runtimeschema.GroupVersionKind][]*validation.SchemaValidator, map[runtimeschema.GroupVersionKind]*schema.Structural, error) { |
There was a problem hiding this comment.
I'd suggest having this return (struct, error) next time someone touches this code. The return values are long and hard to follow.
Description of your changes
This PR adds CEL validation capabilities to the
validatesubcommand.Fixes #5327
I have:
make reviewableto ensure this PR is ready for review.Added or updated e2e tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.