k8sclient.client: refactored to create singleton pattern, remove init #340
Conversation
|
This also passes the run through the readme test. Ran that with master and this change as of tonight. |
pkg/k8sclient/client.go
Outdated
| runBackgroundCacheReset(1 * time.Minute) | ||
| // GetResourceClient returns the resource client using a singleton factory | ||
| func GetResourceClient(apiVersion, kind, namespace string) (dynamic.ResourceInterface, string, error) { | ||
| if singletonFactory == nil { |
There was a problem hiding this comment.
This isn't goroutine safe. If it's possible to be called concurrently, consider using a mutex or https://golang.org/pkg/sync/#Once.Do
pkg/k8sclient/client.go
Outdated
| kubeConfig.ContentConfig = dynamic.ContentConfig() | ||
| clientPool := dynamic.NewClientPool(kubeConfig, restMapper, dynamic.LegacyAPIPathResolverFunc) | ||
|
|
||
| singletonFactory := &resourceClientFactory{ |
pkg/k8sclient/client.go
Outdated
| @@ -77,8 +97,8 @@ func GetResourceClient(apiVersion, kind, namespace string) (dynamic.ResourceInte | |||
| } | |||
|
|
|||
| // GetKubeClient returns the kubernetes client used to create the dynamic client | |||
| func GetKubeClient() kubernetes.Interface { | |||
| return kubeClient | |||
| func (c *resourceClientFactory) GetKubeClient() kubernetes.Interface { | |||
|
@CSdread I'm fine with this intermediate fix for now if it unblocks people to write unit tests for And just for more context on the long term plan for global state: we are currently looking at using the controller-runtime library in the SDK to help us remove the global state and make unit testing easier by letting us inject a fake client. |
|
@hasbro17 that is an awesome plan! I have abstracted out the use of the sdk functions to a class that is also a singleton, that enables me to inject a repository of "queries" to the sdk as a class into my controller. This will enable me to test because i can mock out those methods. |
|
@hasbro17 fixed this up as specified, and re-based. Ready for your approval and merging. Thank you! |
Gopkg.lock
Outdated
| revision = "c12348ce28de40eed0136aa2b644d0ee0650e56c" | ||
| version = "v1.0.1" | ||
| branch = "master" | ||
| name = "github.com/operator-framework/operator-sdk" |
There was a problem hiding this comment.
I think you need to run dep ensure again to update the lock file with the changes from the master.
You're missing the prometheus dependencies that got introduced in #323
https://github.com/operator-framework/operator-sdk/blob/master/Gopkg.lock#L193-L199
Plus I don't know why the SDK is vendoring itself here. This should not be here.
Can you remove your vendor directory and Gopkg.lock file and try dep ensure again.
I've tried your PR locally and it does not need to vendor the SDK.
There was a problem hiding this comment.
not sure either, will do, thanks @hasbro17
|
@hasbro17 fixed |
Gopkg.lock
Outdated
|
|
||
| [[projects]] | ||
| branch = "master" | ||
| name = "github.com/operator-framework/operator-sdk" |
There was a problem hiding this comment.
This still showing up here.
I think it might be better if you can exclude Gopkg.lock from your PR.
Since no new dependencies were introduced by your changes, I don't think Gopkg.lock needs to be updated.
The SDK dependency might be showing up due to some other local state that's not included in this PR.
After this is merged I'll update the lock file myself separately.
|
done @hasbro17 and for future ill destroy my fork and recreate, that way i can stop that |
|
LGTM |
ref: #292
k8sclient.client: refactored to create singleton pattern, remove
initAt this time it is possible to abstract away the api for unit testing, the
initmethod blocks the ability to do the unit tests because it tries to connect to
the cluster. This refactor though not fully the direction you guys are going, is a temporary fix to allow unit testing of downstream code in operators using the sdk. In addition, this does not break the existing API and allows for no other code to change.