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
✨ Support api-approved annotation for CRD with k8s group #691
✨ 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 |
pkg/crd/markers/crd.go
Outdated
@@ -51,6 +52,9 @@ var CRDMarkers = []*definitionWithHelp{ | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why thek8sgroup
suffix?
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.
Emm, I supposed it to be short name of the annotation key api-approved.kubernetes.io
. Do you think k8sgroupapprovedapi
would be better?
pkg/crd/markers/crd.go
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :) |
pkg/crd/testdata/cronjob_types.go
Outdated
@@ -499,6 +499,7 @@ type CronJobStatus struct { | |||
// +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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// +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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emm.. okey, I will update it to // +kubebuilder:metadata:annotations=...
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.
Updated
pkg/crd/markers/crd.go
Outdated
@@ -51,6 +52,9 @@ var CRDMarkers = []*definitionWithHelp{ | |||
|
|||
must(markers.MakeDefinition("kubebuilder:deprecatedversion", markers.DescribesType, DeprecatedVersion{})). | |||
WithHelp(DeprecatedVersion{}.Help()), | |||
|
|||
must(markers.MakeDefinition("kubebuilder:additionalmetadata", markers.DescribesType, AdditionalMetadata{})). | |||
WithHelp(AdditionalMetadata{}.Help()), |
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.
`Are we not adding annotations only?
What about the name here be kubebuilder:annotations
And we Annotations{}
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
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
Needs 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