-
Notifications
You must be signed in to change notification settings - Fork 399
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
✨ Add deprecated marker for versions #541
✨ Add deprecated marker for versions #541
Conversation
/assign @pwittrock |
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.
generally looks good, minor feedback
pkg/crd/markers/crd.go
Outdated
// DeprecatedVersion marks this version as deprecated. | ||
type DeprecatedVersion struct { | ||
// Warning message to be shown on the deprecated version | ||
Warning string |
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.
is this optional, or is it required in the actual CRD. If optional, you can make it a pointer or do something like
type DeprecatedVersion struct {
Warning string `marker:",optional"`
}
So that the equivalent field is also optional, meaning either of the following would work:
// +kubebuilder:deprecated
type SomeKind struct {}
// { deprecated: true }
// +kubebuilder:deprecated:warning="blah"
type SomeKind struct{}
// { deprecated: true, deprecationWarning: "blah" }
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's optional, but the CRD for CRD also takes a pointer for the deprecation message, so I added the marker optional and also made it a pointer
pkg/crd/markers/crd.go
Outdated
@@ -48,6 +48,9 @@ var CRDMarkers = []*definitionWithHelp{ | |||
|
|||
must(markers.MakeDefinition("kubebuilder:unservedversion", markers.DescribesType, UnservedVersion{})). | |||
WithHelp(UnservedVersion{}.Help()), | |||
|
|||
must(markers.MakeDefinition("kubebuilder:deprecated", markers.DescribesType, DeprecatedVersion{})). |
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.
should this be deprecatedversion
to match with unservedversion
and in case we ever get field-level deprecation, or do we think we can just use the same marker for field-level deprecation if we ever get it?
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 makes sense to keep them consistent, I changed it to deprecatedversion
bf636ca
to
8d87d10
Compare
This adds an integration test entry for the unservedversion and deprecatedversion markers.
8d87d10
to
bf2f926
Compare
pushed a test & squashed /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DirectXMan12, dvaldivia 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 |
thanks, and sorry for the lag! |
@DirectXMan12 no problem, any time |
This PR introduces the marker
deprecated
which would mark a CRD version as deprecated in the generated output, it supports a deprecation warning messageExample usage:
the result is that the version that has this marker would include the following fields