Add typed client to plugin client getter. #3043
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
absoludity
commented
Jun 25, 2021
| } | ||
| typedClient, err := kubernetes.NewForConfig(config) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("unable to create typed client: %w", err) |
There was a problem hiding this comment.
Main change here (other than adding the new client) was just to DRY up the code for creating a client (or clients).
There was a problem hiding this comment.
Great, thanks for adding it. I initially thought of adding a clientGetter for each type, sth like "clientGetter" and "dynamicClientGetter", but returning both clients is better (assuming there are no more clients out there)
| @@ -51,6 +48,9 @@ const ( | |||
| clustersCAFilesPrefix = "/etc/additional-clusters-cafiles" | |||
| ) | |||
|
|
|||
| // KubernetesClientGetter is a function type used by plugins to get a k8s client | |||
| type KubernetesClientGetter func(context.Context) (kubernetes.Interface, dynamic.Interface, error) | |||
There was a problem hiding this comment.
Created this type alias so its easier to update the signature (before I added the typed interface).
| if tc.statusCode == codes.OK { | ||
| if _, ok := client.(dynamic.Interface); !ok { |
There was a problem hiding this comment.
Not sure why I initially used a type conversion here, since if it's non-nil, it's going to be the correct type (or it wouldn't compile).
antgamdia
approved these changes
Jun 25, 2021
Closed
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the change
As per discussion on #3022, we originally tried the typed client for interacting with packaging CRs but hit the issue that we'd need the same version of client-go used by those packaging systems, which leads to dependency hell. So we switched to use a dynamic client for the plugins.
That said, now that certain plugins also need to interact with the core k8s api resources, a typed client(set) would also be useful (and nicer to work with) for interacting with those core resources.
This PR updates so that plugins are provided with both - a type clientset and a dynamic clientset.
Benefits
Plugins don't need to use unstructured objects when interacting with the core k8s resources.
Possible drawbacks
None (assuming the client works IRL, which we'll test in a subsequent PR that uses the typed client for the direct helm plugin).
Applicable issues
Additional information
Antonio, if you want to land this so you can use it in your PR, that's fine, or if you'd rather not depend on it but move forward with your unstructured work-around and switch later, that's fine too :)