-
Notifications
You must be signed in to change notification settings - Fork 105
bug: Respect provided kubeconfig in helm collector #1833
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
base: main
Are you sure you want to change the base?
bug: Respect provided kubeconfig in helm collector #1833
Conversation
Signed-off-by: Danil-Grigorev <[email protected]>
func (c configGetter) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) { | ||
return nil, nil | ||
} |
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.
I am unclear if this function is used by the helm client. were you able to confirm either way? it may be possible to implement it like so.
func (c configGetter) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) { | |
return nil, nil | |
} | |
func (c configGetter) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) { | |
discoveryClient, err := discovery.NewDiscoveryClientForConfig(c.restConfig) | |
if err != nil { | |
return nil, err | |
} | |
cached := memory.NewMemCacheClient(discoveryClient) | |
return cached, nil | |
} |
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.
I’ll check tomorrow, but from what I observed, only the implemented method was used. Helm operations there simply listed known resources, so it didn’t require discovery. Committed anyway, thanks for pointing that out.
@Danil-Grigorev Making sure you saw the PR feedback above from @emosbaugh |
Co-authored-by: Ethan Mosbaugh <[email protected]>
Description, Motivation and Context
This change updates the Helm collector to properly use provided client configuration.
Previously, the collector used
actionConfig.Init(nil, ...)
, which relied on implicit defaults and did not respect the configured cluster context. This resulted in using serverhttps://localhost:8080
while performing operations on a host withoutKUBECONFIG
or~/.kube/config
. In a scenario, when thesupport-bundle
was used as a library, this resulted in a failed collection forhelm
.To fix this, a new
configGetter
type was introduced that implementsgenericclioptions.RESTClientGetter
. Helm collector now passesc.ClientConfig
through.Checklist
Does this PR introduce a breaking change?