-
Notifications
You must be signed in to change notification settings - Fork 40
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
fix: use non-deprecated CRD api #355
Conversation
@@ -218,7 +218,7 @@ func GetKubernetesLocalConfig() (*rest.Config, error) { | |||
} | |||
|
|||
func CRDExists(kubeClient dynamic.Interface, name string) bool { | |||
CRDSchema := schema.GroupVersionResource{Group: "apiextensions.k8s.io", Version: "v1beta1", Resource: "customresourcedefinitions"} | |||
CRDSchema := schema.GroupVersionResource{Group: "apiextensions.k8s.io", Version: "v1", Resource: "customresourcedefinitions"} |
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.
oh wow, nice catch, didn't see this one when evaluating deprecated APIs
This would probably not be backwards compatible though, would it? i.e. what happens to people who are still running 1.19 once we change this.
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 believe this deprecation happens in 1.22 which is pretty new)
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.
V1 went GA in 1.16 IIRC
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.
Yep, per https://kubernetes.io/blog/2021/07/14/upcoming-changes-in-kubernetes-1-22/ - V1 has been available since 1.16. Deprecation happened in 1.16, removal happens in 1.22.
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.
CRDExists is used by upgrade stratgegy for crd strategy if you want to use some CRD to upgrade.
In this case, if you were using upgrade-manager's RollingUpgrade CRD, but you are still using v1beta1 version of it, you would have a problem right?
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.
Thats fine then, we should just probably add a release note on this being a potentially breaking 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.
You shouldn't - per Kubernetes docs
For example, suppose there are two API versions, v1 and v1beta1, for the same resource. If you originally created an object using the v1beta1 version of its API, you can later read, update, or delete that object using either the v1beta1 or the v1 API version.
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.
Right, so I guess it's a storedVersion for CRD type
Signed-off-by: Jonah Back <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #355 +/- ##
=======================================
Coverage 51.44% 51.44%
=======================================
Files 33 33
Lines 4556 4556
=======================================
Hits 2344 2344
Misses 2065 2065
Partials 147 147
Continue to review full report at Codecov.
|
Signed-off-by: Jonah Back [email protected]