Skip to content
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

Update controller-runtime from 0.14 to 0.15 #6465

Closed

Conversation

bshephar
Copy link

This change updates the version of controller-runtime to v0.15.0 along with the necessary method changes to satisfy the interface changes that come with v0.15.0. This is similar to what has been done in Kubebuilder here:
https://github.com/kubernetes-sigs/kubebuilder/pull/3394/files

The specific interface changes in this version are defined in this controller-runtime commit:
kubernetes-sigs/controller-runtime@8770b4d#diff-0b6c79b3483fe5ad476ea26192f2d8bfe05a44688209721b626dc793204df28eR42

Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bshephar Thanks for the PR. Updating controller-runtime in this specific case comes with bumping k8s too. The process usually involves the following steps:

  1. Updating c-r to the latest version in SDK binary, involving a bump in all its dependent libraries (operator-lib, o-f/api, k8s deps etc).
  2. Bumping all the plugins to include the latest versions of Kubebuilder and C-R.
  3. Bumping envtest to use the latest version of k8s.

Doing (1) and (2) together is usually easier. But since that still involves bumping c-r and k8s deps in all the other plugins, we could proceed with (1), but also make sure to complete (2) and rest to fully support the latest versions.

go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@bshephar bshephar force-pushed the controller-runtime branch 2 times, most recently from d5efe19 to 9ec0d43 Compare June 14, 2023 00:12
@bshephar
Copy link
Author

Looks like we also need to update helm-operator-plugins now to this version of kubebuilder since it removes the v2-alpha directory and instead uses v2:

❯ go vet ./...
../../../go/pkg/mod/github.com/operator-framework/helm-operator-plugins@v0.0.12-0.20230413193425-4632388adc61/pkg/plugins/hybrid/v1alpha/scaffolds/init.go:35:2: no required module provides package sigs.k8s.io/kubebuilder/v3/pkg/plugins/common/kustomize/v2-alpha; to add it:
	go get sigs.k8s.io/kubebuilder/v3/pkg/plugins/common/kustomize/v2-alpha

https://github.com/kubernetes-sigs/kubebuilder/tree/master/pkg/plugins/common/kustomize

@bshephar
Copy link
Author

Looks like we also need to update helm-operator-plugins now to this version of kubebuilder since it removes the v2-alpha directory and instead uses v2:

❯ go vet ./...
../../../go/pkg/mod/github.com/operator-framework/helm-operator-plugins@v0.0.12-0.20230413193425-4632388adc61/pkg/plugins/hybrid/v1alpha/scaffolds/init.go:35:2: no required module provides package sigs.k8s.io/kubebuilder/v3/pkg/plugins/common/kustomize/v2-alpha; to add it:
	go get sigs.k8s.io/kubebuilder/v3/pkg/plugins/common/kustomize/v2-alpha

https://github.com/kubernetes-sigs/kubebuilder/tree/master/pkg/plugins/common/kustomize

Ok, so updating things in helm-operator-plugins gets us to these last 3:

❯ go vet ./...
# k8s.io/client-go/applyconfigurations/meta/v1
../../../go/pkg/mod/k8s.io/client-go@v0.27.2/applyconfigurations/meta/v1/unstructured.go:64:38: cannot use doc (variable of type *"github.com/google/gnostic/openapiv2".Document) as *"github.com/google/gnostic-models/openapiv2".Document value in argument to proto.NewOpenAPIData
# sigs.k8s.io/kustomize/kyaml/openapi
../../../go/pkg/mod/sigs.k8s.io/kustomize/kyaml@v0.14.1/openapi/openapi.go:683:33: cannot use doc (variable of type *"github.com/google/gnostic/openapiv2".Document) as *"github.com/google/gnostic-models/openapiv2".Document value in argument to swagger.FromGnostic
# k8s.io/kubectl/pkg/util/openapi
../../../go/pkg/mod/k8s.io/kubectl@v0.27.2/pkg/util/openapi/openapi.go:52:38: cannot use doc (variable of type *"github.com/google/gnostic/openapiv2".Document) as *"github.com/google/gnostic-models/openapiv2".Document value in argument to proto.NewOpenAPIData

Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bshephar I tried these locally. This change requires helm-operator-plugins (operator-framework/helm-operator-plugins#216) and java-plugins (https://github.com/operator-framework/java-operator-sdk) to be bumped first.

However, meanwhile just to test if openapi bump is causing problems, I removed both the plugin dependencies, bumped KB to v3.11.0 (https://github.com/kubernetes-sigs/kubebuilder/releases/tag/v3.11.0) and tested locally. Looks like go mod tidy works well, there are just some breaking changes which need to be addressed in the codebase. Considering this, I believe the error you are facing is because of both the plugins haven't been bumped their k8s dependencies yet. Could we merge both of them to proceed further? Thanks!

This change updates the version of controller-runtime to v0.15.0 along
with the necessary method changes to satisfy the interface changes that
come with v0.15.0. This is similar to what has been done in Kubebuilder
here:
https://github.com/kubernetes-sigs/kubebuilder/pull/3394/files

The specific interface changes in this version are defined in this
controller-runtime commit:
kubernetes-sigs/controller-runtime@8770b4d#diff-0b6c79b3483fe5ad476ea26192f2d8bfe05a44688209721b626dc793204df28eR42

Signed-off-by: Brendan Shephard <bshephar@redhat.com>
Signed-off-by: Brendan Shephard <bshephar@redhat.com>
This change bumps the controller-runtime version to the commit hash that
includes controller-runtime v0.15

Signed-off-by: Brendan Shephard <bshephar@redhat.com>
This change bumps the kubebuilder version to the commit hash
that includes the controller-runtime v0.15 change.

Signed-off-by: Brendan Shephard <bshephar@redhat.com>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2023
@openshift-merge-robot
Copy link

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

This change updates helm-operator-plugins and other dependencies
associated with the CR v0.15.0 and k8s.io 0.27.2

Signed-off-by: Brendan Shephard <bshephar@redhat.com>
@varshaprasad96
Copy link
Member

@bshephar Can we merge the work done here in this PR (#6514). If you're fine, we can collaborate on this and I can cherry pick some of the commits from your branch and add it to the other PR. This is because the other one includes changes in KB and other operators that need to fixed along with this. Also, we are blocked on one of the controller-runtime's PR to include logging to the event handler.

@bshephar
Copy link
Author

@bshephar Can we merge the work done here in this PR (#6514). If you're fine, we can collaborate on this and I can cherry pick some of the commits from your branch and add it to the other PR. This is because the other one includes changes in KB and other operators that need to fixed along with this. Also, we are blocked on one of the controller-runtime's PR to include logging to the event handler.

Hey! Yeah, we sure can. I'm happy with that, I'll close this one.

@bshephar bshephar closed this Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants