-
Notifications
You must be signed in to change notification settings - Fork 634
Use OpenAPI to validate ImageVolumeSource.Reference #4308
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
Conversation
This version adds Title markers and accepts "optionalOldSelf" of Kubernetes 1.30 on XValidation markers. See: https://github.com/kubernetes-sigs/controller-tools/releases/v0.18.0
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.
🤔 v0.18 requires k8s.io/.. v0.33, but it wasn't bundled in the k8s.io updates last month: #4248
I don't know why!
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.
📝 This may very well be "too clever." I'm curious what others think.
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.
Setting a title field on our CRD to add some validation rules and then removing the title field -- is that the part you're thinking might be too clever?
Seems reasonable to me, with the caveat that having the CRD be assembled from kubebuilder annotations and then post-process functions might make it a little harder to know easily where some element is coming from. Maybe that's a documentation issue, e.g., when we add the title in the struct, we could link to or reference this jq script at this point?
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.
Setting a
titlefield on our CRD to add some validation rules and then removing thetitlefield -- is that the part you're thinking might be too clever?
Yeah. Smuggling information through a marker that does something else.
Seems reasonable to me, with the caveat that having the CRD be assembled from kubebuilder annotations and then post-process functions might make it a little harder to know easily where some element is coming from. Maybe that's a documentation issue, e.g., when we add the title in the struct, we could link to or reference this jq script at this point?
A comment on the marker is better than none. I'll add one.
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.
A comment on the marker is better than none. I'll add one.
Done.
909426e to
c71315f
Compare
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.
I'm curious to hear your thinking behind this change
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.
This information is minimally interesting during development, but AFAIK has no use after that. As an annotation, it is deployed with the CRD but serves no purpose.
My impression has been that the generator is using metadata.annotations to push "stuff" through code that can only handle K8s objects.
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.
And we can't just capture the whole line as a single capture group because you want to exclude the quotes, yeah?
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.
🤔 It doesn't seem like I need the second capture... The line should end just after the quoted string, right? 🦆
I'll check this tomorrow.
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.
And we can't just capture the whole line as a single capture group because you want to exclude the quotes, yeah?
Right. This finds lines starting with '# and removes a pair of '. This is the transformation:
-'# controller-gen.kubebuilder.io/version': v0.18.0 +# controller-gen.kubebuilder.io/version: v0.18.0
🤔 It doesn't seem like I need the second capture...
I forgot that the quotes are around only the key of the flow mapping. The second capture is for the remainder of the line, if any.
OpenAPI validation does not count toward the CRD XValidation CEL budget.
c71315f to
9b7ac1b
Compare
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.
👍
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
We cannot add validation markers inside upstream types, so we have to use CEL validation from outside the type.
What is the new behavior (if this is a feature change)?
We can use the
+kubebuilder:titlemarker to add identifiers to CRD schemas, and manipulate those aftercontroller-genruns.