-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: migrate velero controller from kubebuilder v2 to v3 #4382
feat: migrate velero controller from kubebuilder v2 to v3 #4382
Conversation
@@ -226,11 +225,6 @@ func AllCRDs(perferredAPIVersion string) *unstructured.UnstructuredList { | |||
resources.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Version: "v1", Kind: "List"}) | |||
|
|||
switch perferredAPIVersion { | |||
case "v1beta1": |
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.
Per my understanding after moving to kubebuilder v3, all CRDs of velero will be v1, so the preferredAPIVersion
is unnecessary?
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.
In release 1.8, there is only one possible version, v1, but preferredAPIVersion is passed from velero client command line parameter "--crds-version".
I think it may be useful for future potential multiple versions need.
In my opinion, keeping the logic there and adding validation to only support v1 may be better.
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.
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.
Yes, this flag was only in place to allow users to explicitly set which version to use if needed. Given that we always use v1 now, you can remove this flag.
There is also some code in the uninstall path to remove v1beta1 CRDS (https://github.com/vmware-tanzu/velero/blob/main/pkg/cmd/cli/uninstall/uninstall.go#L139-L150) but I think we may want to keep that for now to handle any v1beta1 CRDs that a user may have installed. We can probably remove that in 1.9.
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. I think the agreement is to remove the CRD version selection parameter in install command, and keep the v1beta1 check in uninstall command until 1.9 release.
If then, I will open another PR to the remove the CRD version selection parameter.
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.
LGTM - thanks @blackpiglet! I only have one comment on the changelog but everything else looks good 👍
changelogs/unreleased/4382-jxun
Outdated
@@ -0,0 +1 @@ | |||
feat: migrate velero controller from kubebuilder v2 to v3 |
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 changelog should include details to state that v1beta1 CRDs are no longer supported and that we only support Kubernetes 1.16 and later.
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.
Got it. Thanks.
@@ -226,11 +225,6 @@ func AllCRDs(perferredAPIVersion string) *unstructured.UnstructuredList { | |||
resources.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Version: "v1", Kind: "List"}) | |||
|
|||
switch perferredAPIVersion { | |||
case "v1beta1": |
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.
Yes, this flag was only in place to allow users to explicitly set which version to use if needed. Given that we always use v1 now, you can remove this flag.
There is also some code in the uninstall path to remove v1beta1 CRDS (https://github.com/vmware-tanzu/velero/blob/main/pkg/cmd/cli/uninstall/uninstall.go#L139-L150) but I think we may want to keep that for now to handle any v1beta1 CRDs that a user may have installed. We can probably remove that in 1.9.
#4389 is opened to trace --crds-version removing. |
Could you squash to commits? |
1. remove config/crd/v1beta1 2. remove PROJECT file 3. update controller-gen and kubebuilder version 4. generate client and CRD file 5. add changelog and remove v1beta1 CRD generated code. 6. add kubebuilder test bundle setup command. 7. due to apiextensions.k8s.io/v1beta1 is not supported, only k8s after v1.16 is supported, so remove v1.15 check. 8. add CRD and k8s suppored version update in changelog. Signed-off-by: Xun Jiang <jxun@vmware.com>
8485daf
to
303d3dc
Compare
Sure. Commits are squashed. |
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.
LGTM - thanks!
Signed-off-by: Xun Jiang jxun@vmware.com
Thank you for contributing to Velero!
Please add a summary of your change
Does your change fix a particular issue?
Fixes #(issue)
Please indicate you've done the following:
/kind changelog-not-required
.site/content/docs/main
.