✨ Support api-approved annotation for CRD with k8s group#691
Conversation
4b63d42 to
e8aea0c
Compare
|
/retitle ✨ Support api-approved annotation for CRD with k8s group |
| must(markers.MakeDefinition("kubebuilder:deprecatedversion", markers.DescribesType, DeprecatedVersion{})). | ||
| WithHelp(DeprecatedVersion{}.Help()), | ||
|
|
||
| must(markers.MakeDefinition("kubebuilder:apiapprovedk8sgroup", markers.DescribesType, APIApprovedKubernetesGroup{})). |
There was a problem hiding this comment.
Emm, I supposed it to be short name of the annotation key api-approved.kubernetes.io. Do you think k8sgroupapprovedapi would be better?
| type APIApprovedKubernetesGroup struct { | ||
| // URL is the value of api-approved.kubernetes.io annotation on CRD, | ||
| // it should be a valid URL which contains Scheme and Host and | ||
| // should not start with "unapproved". | ||
| URL string `marker:"URL"` | ||
| } |
There was a problem hiding this comment.
Any chance we could make this a more generic Annotation struct with Key and Value fields that we parse out of the marker?
I can imagine a few other use cases, for example adding annotation cert-manager.io/inject-ca-from-secret to the CRD when it needs a CA injected for a conversion webhook configuration.
There was a problem hiding this comment.
Thanks, it makes sense. I will update this later.
ad92c40 to
fde1d19
Compare
|
@joelanford @alvaroaleman I have made it more generic for additional annotations or labels. PTAL :) |
| // +kubebuilder:subresource:status | ||
| // +kubebuilder:resource:singular=mycronjob | ||
| // +kubebuilder:storageversion | ||
| // +kubebuilder:additionalmetadata:annotations="api-approved.kubernetes.io=https://github.com/kubernetes-sigs/controller-tools";"cert-manager.io/inject-ca-from-secret=cert-manager/cert-manager-webhook-ca" |
There was a problem hiding this comment.
| // +kubebuilder:additionalmetadata:annotations="api-approved.kubernetes.io=https://github.com/kubernetes-sigs/controller-tools";"cert-manager.io/inject-ca-from-secret=cert-manager/cert-manager-webhook-ca" | |
| // +kubebuilder:metadata:annotations="api-approved.kubernetes.io=https://github.com/kubernetes-sigs/controller-tools";"cert-manager.io/inject-ca-from-secret=cert-manager/cert-manager-webhook-ca" |
Do we need the additional?
Why not only annotations?
So, if set we add annotations informed
If not set, we add only the default annotation
There was a problem hiding this comment.
Emm.. okey, I will update it to // +kubebuilder:metadata:annotations=...
| WithHelp(DeprecatedVersion{}.Help()), | ||
|
|
||
| must(markers.MakeDefinition("kubebuilder:additionalmetadata", markers.DescribesType, AdditionalMetadata{})). | ||
| WithHelp(AdditionalMetadata{}.Help()), |
There was a problem hiding this comment.
`Are we not adding annotations only?
What about the name here be kubebuilder:annotations
And we Annotations{}
There was a problem hiding this comment.
Ps is possible to add annotations outside of the metadata?
Do we need to use this term here or is that redundant?
There was a problem hiding this comment.
Now it supports to add annotations and labels
// +kubebuilder:metadata:annotations=...
// +kubebuilder:metadata:labels=...
Not sure if there will be some other fields in metadata have to be added there. So we'd better put all of them in kubebuilder:metadata.
fde1d19 to
b370e9b
Compare
camilamacedo86
left a comment
There was a problem hiding this comment.
That seems great for me 🥇
/lgtm
|
/ok-to-test |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, camilamacedo86, FillZpp The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Support api-approved annotation for CRD with k8s group
fixes #656