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

Misleading/redundant usage of AddToScheme #1014

Closed
twz123 opened this issue Feb 10, 2023 · 1 comment · Fixed by #1202
Closed

Misleading/redundant usage of AddToScheme #1014

twz123 opened this issue Feb 10, 2023 · 1 comment · Fixed by #1202

Comments

@twz123
Copy link
Contributor

twz123 commented Feb 10, 2023

Bug Description

Looking at those files

  • cmd/collect/cli/run.go
  • cmd/troubleshoot/cli/run.go
  • pkg/collect/load.go
  • pkg/supportbundle/load.go

they use this code pattern:

import (
	"github.com/replicatedhq/troubleshoot/pkg/client/troubleshootclientset/scheme"
	troubleshootclientsetscheme "github.com/replicatedhq/troubleshoot/pkg/client/troubleshootclientset/scheme"
)

func someFunc(...) {
	// ...
	troubleshootclientsetscheme.AddToScheme(scheme.Scheme)
	// ...
}

This seems to be:

  • misleading, since one would expect scheme to be an import of k8s.io/client-go/kubernetes/scheme.
  • redundant, as the code as is is actually also part of the init function of the troubleshootclientset's scheme.

Expected Behavior

Depending on the desired behavior, either one of the following:

  1. remove those lines, as they are redundant
  2. fix the import, so that the troubleshoot scheme gets added to client-go: import "k8s.io/client-go/kubernetes/scheme"
  3. fix the import usage, so that the lines look like troubleshootclientsetscheme.AddToScheme(troubleshootclientsetscheme.Scheme)

I think removal should be fine, since as it's right now, they're not doing anything. But maybe there are bugs that may be fixed by option 2.

Additional context

AFAICT, the prefilght command actually uses client-go's import, so maybe it's required there.

@xavpaice
Copy link
Member

#933 will tidy a bunch of this up, but I'll leave the issue open as this is valuable information we need to check is dealt with when we update the CLI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants