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

bump k8s dependencies #98

Merged

Conversation

varshaprasad96
Copy link
Member

This commit bumps:

  1. k8s dependencies to 1.21+
  2. Controller runtime to 0.9.2
  3. Kubebuilder to the latest version at the time of this commit

This is to make the plugin compatible with the recent bump of dependencies in Operator SDK.

@fabianvf
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jul 30, 2021
@coveralls
Copy link

coveralls commented Aug 2, 2021

Pull Request Test Coverage Report for Build 1117514105

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 89.693%

Totals Coverage Status
Change from base Build 1102982089: -0.04%
Covered Lines: 1375
Relevant Lines: 1533

💛 - Coveralls

This commit bumps:
1. k8s dependencies to 1.21+
2. Controller runtime to 0.9.2
3. Kubebuilder to the latest version at the time of this commit

Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>
Copy link
Member

@fabianvf fabianvf left a comment

Choose a reason for hiding this comment

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

/lgtm

/cc @joelanford the apidiff breakage is due to the c-r bump, do we need to find a way to maintain that or are we good dropping it?

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2021
Comment on lines 124 to 136
NewClient: func(cache cache.Cache, config *rest.Config, options client.Options, uncachedObjects ...client.Object) (client.Client, error) {
// Create the client for Write operation
c, err := client.New(config, options)
if err != nil {
return nil, err
}
return client.NewDelegatingClient(client.NewDelegatingClientInput{
CacheReader: cache,
Client: c,
UncachedObjects: uncachedObjects,
CacheUnstructured: true,
})
},
Copy link
Member

@joelanford joelanford Aug 6, 2021

Choose a reason for hiding this comment

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

To make this re-usable and to de-clutter this file, what do you think about keeping pkg/manager/delegatingclient.go around and defining a function there that returns a function with the NewClientFunc signature.

It may even be possible (though maybe not necessary) to keep the existing code around (maybe deprecate it) and have this new function just wrap the existing code.

Something like:

func NewCachingClientFunc() cluster.NewClientFunc {
	return func(cache cache.Cache, config *rest.Config, options client.Options, uncachedObjects ...client.Object) (client.Client, error) {
		return NewCachingClientBuilder().WithUncached(uncachedObjects...).Build(cache, config, options)
	}
}

This approach would let you keep the tests for this around in an easier-to-maintain way as well.

Copy link
Member Author

@varshaprasad96 varshaprasad96 Aug 9, 2021

Choose a reason for hiding this comment

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

@joelanford the previous implementation returned manager.ClientBuilder which has been removed from controller-runtime, so we can't keep the existing code.

On creating a wrapper after removing existing code, do you mean having this ?

func NewCachingClientFunc() cluster.NewClientFunc {
	return func(cache cache.Cache, config *rest.Config, options client.Options, uncachedObjects ...client.Object) (client.Client, error) {
		c, err := client.New(config, options)
		if err != nil {
			return nil, err
		}

		return client.NewDelegatingClient(client.NewDelegatingClientInput{
			CacheReader:       cache,
			Client:            c,
			UncachedObjects:   uncachedObjects,
			CacheUnstructured: true,
		})
	}
}

Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2021
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2021
@varshaprasad96 varshaprasad96 merged commit 240cc44 into operator-framework:main Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants