Merge pull request #51684 from tcharding/lint-plugins-env

Automatic merge from submit-queue (batch tested with PRs 51031, 51705, 51888, 51727, 51684). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>..

kubectl: Fix golint warnings for plugins/*

**What this PR does / why we need it**:

We currently have an entry in `hack/.golint_failures` for `pkg/kubectl/plugins`. If we lint the files in `plugins/` then we can safely remove this entry.
 
`golint` emits the following warnings (only one of each type shown)
```
exported method EnvList.Slice should have comment or be unexported
error var IncompletePluginError should have name of the form ErrFoo
func name will be used as plugins.PluginsEnvVarPluginLoader by other packages, and that stutters; consider calling this EnvVarPluginLoader
```

This PR clears all `golint` warnings from `kubectl/plugins/`.

1. Remove entry from `hack/.golint_failures`
2. Add missing documentation to exported types and functions.
3. Rename error variables to `ErrFoo`.
4. Rename `PluginsEnvVarPluginLoader` to `KubectlPluginsPathPluginLoader` (mirrors env var name).
5. Rename `XDGDataPluginLoader` to `XDGDataDirsPluginLoader` (to make uniform change above).

**Special notes for your reviewer**:

Felt a bit dirty doing this cleanup because the code in `plugins/` is particularly clean and well written already.

**Release note**:

```release-note
NONE
```
/sig cli
/kind cleanup

hack/.golint_failures

@@ -217,7 +217,6 @@ pkg/kubectl/cmd/util/editor
 pkg/kubectl/cmd/util/jsonmerge
 pkg/kubectl/cmd/util/sanity
 pkg/kubectl/metricsutil
-pkg/kubectl/plugins
 pkg/kubectl/resource
 pkg/kubectl/testing
 pkg/kubectl/util

pkg/kubectl/cmd/util/factory_builder.go

@@ -156,10 +156,10 @@ func (f *ring2Factory) NewBuilder() *resource.Builder {
 // system directory structure spec for the given platform.
 func (f *ring2Factory) PluginLoader() plugins.PluginLoader {
 	if len(os.Getenv("KUBECTL_PLUGINS_PATH")) > 0 {
-		return plugins.PluginsEnvVarPluginLoader()
+		return plugins.KubectlPluginsPathPluginLoader()
 	}
 	return plugins.TolerantMultiPluginLoader{
-		plugins.XDGDataPluginLoader(),
+		plugins.XDGDataDirsPluginLoader(),
 		plugins.UserDirPluginLoader(),
 	}
 }

pkg/kubectl/plugins/env.go

@@ -24,19 +24,21 @@ import (
 	"github.com/spf13/pflag"
 )
 
-// Env represents an environment variable with its name and value
+// Env represents an environment variable with its name and value.
 type Env struct {
 	N string
 	V string
 }
 
+// String returns "name=value" string.
 func (e Env) String() string {
 	return fmt.Sprintf("%s=%s", e.N, e.V)
 }
 
-// EnvList is a list of Env
+// EnvList is a list of Env.
 type EnvList []Env
 
+// Slice returns a slice of "name=value" strings.
 func (e EnvList) Slice() []string {
 	envs := []string{}
 	for _, env := range e {
@@ -45,6 +47,7 @@ func (e EnvList) Slice() []string {
 	return envs
 }
 
+// Merge converts "name=value" strings into Env values and merges them into e.
 func (e EnvList) Merge(s ...string) EnvList {
 	newList := e
 	newList = append(newList, fromSlice(s)...)
@@ -53,12 +56,14 @@ func (e EnvList) Merge(s ...string) EnvList {
 
 // EnvProvider provides the environment in which the plugin will run.
 type EnvProvider interface {
+	// Env returns the env list.
 	Env() (EnvList, error)
 }
 
-// MultiEnvProvider is an EnvProvider for multiple env providers, returns on first error.
+// MultiEnvProvider satisfies the EnvProvider interface for multiple env providers.
 type MultiEnvProvider []EnvProvider
 
+// Env returns the combined env list of multiple env providers, returns on first error.
 func (p MultiEnvProvider) Env() (EnvList, error) {
 	env := EnvList{}
 	for _, provider := range p {
@@ -71,9 +76,10 @@ func (p MultiEnvProvider) Env() (EnvList, error) {
 	return env, nil
 }
 
-// PluginCallerEnvProvider provides env with the path to the caller binary (usually full path to 'kubectl').
+// PluginCallerEnvProvider satisfies the EnvProvider interface.
 type PluginCallerEnvProvider struct{}
 
+// Env returns env with the path to the caller binary (usually full path to 'kubectl').
 func (p *PluginCallerEnvProvider) Env() (EnvList, error) {
 	caller, err := os.Executable()
 	if err != nil {
@@ -84,11 +90,12 @@ func (p *PluginCallerEnvProvider) Env() (EnvList, error) {
 	}, nil
 }
 
-// PluginDescriptorEnvProvider provides env vars with information about the running plugin.
+// PluginDescriptorEnvProvider satisfies the EnvProvider interface.
 type PluginDescriptorEnvProvider struct {
 	Plugin *Plugin
 }
 
+// Env returns env with information about the running plugin.
 func (p *PluginDescriptorEnvProvider) Env() (EnvList, error) {
 	if p.Plugin == nil {
 		return []Env{}, fmt.Errorf("plugin not present to extract env")
@@ -104,19 +111,24 @@ func (p *PluginDescriptorEnvProvider) Env() (EnvList, error) {
 	return env, nil
 }
 
-// OSEnvProvider provides current environment from the operating system.
+// OSEnvProvider satisfies the EnvProvider interface.
 type OSEnvProvider struct{}
 
+// Env returns the current environment from the operating system.
 func (p *OSEnvProvider) Env() (EnvList, error) {
 	return fromSlice(os.Environ()), nil
 }
 
+// EmptyEnvProvider satisfies the EnvProvider interface.
 type EmptyEnvProvider struct{}
 
+// Env returns an empty environment.
 func (p *EmptyEnvProvider) Env() (EnvList, error) {
 	return EnvList{}, nil
 }
 
+// FlagToEnvName converts a flag string into a UNIX like environment variable name.
+//  e.g --some-flag => "PREFIX_SOME_FLAG"
 func FlagToEnvName(flagName, prefix string) string {
 	envName := strings.TrimPrefix(flagName, "--")
 	envName = strings.ToUpper(envName)
@@ -125,6 +137,8 @@ func FlagToEnvName(flagName, prefix string) string {
 	return envName
 }
 
+// FlagToEnv converts a flag and its value into an Env.
+//  e.g --some-flag some-value => Env{N: "PREFIX_SOME_FLAG", V="SOME_VALUE"}
 func FlagToEnv(flag *pflag.Flag, prefix string) Env {
 	envName := FlagToEnvName(flag.Name, prefix)
 	return Env{envName, flag.Value.String()}

pkg/kubectl/plugins/loader.go

@@ -28,10 +28,12 @@ import (
 	"k8s.io/client-go/tools/clientcmd"
 )
 
+// PluginDescriptorFilename is the default file name for plugin descriptions.
 const PluginDescriptorFilename = "plugin.yaml"
 
 // PluginLoader is capable of loading a list of plugin descriptions.
 type PluginLoader interface {
+	// Load loads the plugin descriptions.
 	Load() (Plugins, error)
 }
 
@@ -107,7 +109,7 @@ func (l *DirectoryPluginLoader) Load() (Plugins, error) {
 	return list, err
 }
 
-// UserDirPluginLoader is a PluginLoader that loads plugins from the
+// UserDirPluginLoader returns a PluginLoader that loads plugins from the
 // "plugins" directory under the user's kubeconfig dir (usually "~/.kube/plugins/").
 func UserDirPluginLoader() PluginLoader {
 	dir := filepath.Join(clientcmd.RecommendedConfigDir, "plugins")
@@ -116,7 +118,7 @@ func UserDirPluginLoader() PluginLoader {
 	}
 }
 
-// PathFromEnvVarPluginLoader is a PluginLoader that loads plugins from one or more
+// PathFromEnvVarPluginLoader returns a PluginLoader that loads plugins from one or more
 // directories specified by the provided env var name. In case the env var is not
 // set, the PluginLoader just loads nothing. A list of subdirectories can be provided,
 // which will be appended to each path specified by the env var.
@@ -135,17 +137,17 @@ func PathFromEnvVarPluginLoader(envVarName string, subdirs ...string) PluginLoad
 	return loader
 }
 
-// PluginsEnvVarPluginLoader is a PluginLoader that loads plugins from one or more
+// KubectlPluginsPathPluginLoader returns a PluginLoader that loads plugins from one or more
 // directories specified by the KUBECTL_PLUGINS_PATH env var.
-func PluginsEnvVarPluginLoader() PluginLoader {
+func KubectlPluginsPathPluginLoader() PluginLoader {
 	return PathFromEnvVarPluginLoader("KUBECTL_PLUGINS_PATH")
 }
 
-// XDGDataPluginLoader is a PluginLoader that loads plugins from one or more
+// XDGDataDirsPluginLoader returns a PluginLoader that loads plugins from one or more
 // directories specified by the XDG system directory structure spec in the
 // XDG_DATA_DIRS env var, plus the "kubectl/plugins/" suffix. According to the
 // spec, if XDG_DATA_DIRS is not set it defaults to "/usr/local/share:/usr/share".
-func XDGDataPluginLoader() PluginLoader {
+func XDGDataDirsPluginLoader() PluginLoader {
 	envVarName := "XDG_DATA_DIRS"
 	if len(os.Getenv(envVarName)) > 0 {
 		return PathFromEnvVarPluginLoader(envVarName, "kubectl", "plugins")
@@ -164,6 +166,7 @@ func XDGDataPluginLoader() PluginLoader {
 // a successful loading means every encapsulated loader was able to load without errors.
 type MultiPluginLoader []PluginLoader
 
+// Load calls Load() for each of the encapsulated Loaders.
 func (l MultiPluginLoader) Load() (Plugins, error) {
 	plugins := Plugins{}
 	for _, loader := range l {
@@ -180,6 +183,7 @@ func (l MultiPluginLoader) Load() (Plugins, error) {
 // but is tolerant to errors while loading from them.
 type TolerantMultiPluginLoader []PluginLoader
 
+// Load calls Load() for each of the encapsulated Loaders.
 func (l TolerantMultiPluginLoader) Load() (Plugins, error) {
 	plugins := Plugins{}
 	for _, loader := range l {
@@ -191,9 +195,10 @@ func (l TolerantMultiPluginLoader) Load() (Plugins, error) {
 	return plugins, nil
 }
 
-// DummyPluginLoader loads nothing.
+// DummyPluginLoader is a noop PluginLoader.
 type DummyPluginLoader struct{}
 
+// Load loads nothing.
 func (l *DummyPluginLoader) Load() (Plugins, error) {
 	return Plugins{}, nil
 }

pkg/kubectl/plugins/loader_test.go

@@ -109,7 +109,7 @@ func TestUnexistentDirectoryPluginLoader(t *testing.T) {
 	}
 }
 
-func TestPluginsEnvVarPluginLoader(t *testing.T) {
+func TestKubectlPluginsPathPluginLoader(t *testing.T) {
 	tmp, err := setupValidPlugins(1, 0)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -120,7 +120,7 @@ func TestPluginsEnvVarPluginLoader(t *testing.T) {
 	os.Setenv(env, tmp)
 	defer os.Unsetenv(env)
 
-	loader := PluginsEnvVarPluginLoader()
+	loader := KubectlPluginsPathPluginLoader()
 
 	plugins, err := loader.Load()
 	if err != nil {

pkg/kubectl/plugins/plugins.go

@@ -23,11 +23,16 @@ import (
 )
 
 var (
-	IncompletePluginError     = fmt.Errorf("incomplete plugin descriptor: name, shortDesc and command fields are required")
-	InvalidPluginNameError    = fmt.Errorf("plugin name can't contain spaces")
-	IncompleteFlagError       = fmt.Errorf("incomplete flag descriptor: name and desc fields are required")
-	InvalidFlagNameError      = fmt.Errorf("flag name can't contain spaces")
-	InvalidFlagShorthandError = fmt.Errorf("flag shorthand must be only one letter")
+	// ErrIncompletePlugin indicates plugin is incomplete.
+	ErrIncompletePlugin = fmt.Errorf("incomplete plugin descriptor: name, shortDesc and command fields are required")
+	// ErrInvalidPluginName indicates plugin name is invalid.
+	ErrInvalidPluginName = fmt.Errorf("plugin name can't contain spaces")
+	// ErrIncompleteFlag indicates flag is incomplete.
+	ErrIncompleteFlag = fmt.Errorf("incomplete flag descriptor: name and desc fields are required")
+	// ErrInvalidFlagName indicates flag name is invalid.
+	ErrInvalidFlagName = fmt.Errorf("flag name can't contain spaces")
+	// ErrInvalidFlagShorthand indicates flag shorthand is invalid.
+	ErrInvalidFlagShorthand = fmt.Errorf("flag shorthand must be only one letter")
 )
 
 // Plugin is the representation of a CLI extension (plugin).
@@ -37,7 +42,7 @@ type Plugin struct {
 	Context RunningContext `json:"-"`
 }
 
-// PluginDescription holds everything needed to register a
+// Description holds everything needed to register a
 // plugin as a command. Usually comes from a descriptor file.
 type Description struct {
 	Name      string  `json:"name"`
@@ -49,18 +54,19 @@ type Description struct {
 	Tree      Plugins `json:"tree,omitempty"`
 }
 
-// PluginSource holds the location of a given plugin in the filesystem.
+// Source holds the location of a given plugin in the filesystem.
 type Source struct {
 	Dir            string `json:"-"`
 	DescriptorName string `json:"-"`
 }
 
+// Validate validates plugin data.
 func (p Plugin) Validate() error {
 	if len(p.Name) == 0 || len(p.ShortDesc) == 0 || (len(p.Command) == 0 && len(p.Tree) == 0) {
-		return IncompletePluginError
+		return ErrIncompletePlugin
 	}
 	if strings.Index(p.Name, " ") > -1 {
-		return InvalidPluginNameError
+		return ErrInvalidPluginName
 	}
 	for _, flag := range p.Flags {
 		if err := flag.Validate(); err != nil {
@@ -75,6 +81,7 @@ func (p Plugin) Validate() error {
 	return nil
 }
 
+// IsValid returns true if plugin data is valid.
 func (p Plugin) IsValid() bool {
 	return p.Validate() == nil
 }
@@ -90,24 +97,27 @@ type Flag struct {
 	DefValue  string `json:"defValue,omitempty"`
 }
 
+// Validate validates flag data.
 func (f Flag) Validate() error {
 	if len(f.Name) == 0 || len(f.Desc) == 0 {
-		return IncompleteFlagError
+		return ErrIncompleteFlag
 	}
 	if strings.Index(f.Name, " ") > -1 {
-		return InvalidFlagNameError
+		return ErrInvalidFlagName
 	}
 	return f.ValidateShorthand()
 }
 
+// ValidateShorthand validates flag shorthand data.
 func (f Flag) ValidateShorthand() error {
 	length := len(f.Shorthand)
 	if length == 0 || (length == 1 && unicode.IsLetter(rune(f.Shorthand[0]))) {
 		return nil
 	}
-	return InvalidFlagShorthandError
+	return ErrInvalidFlagShorthand
 }
 
+// Shorthanded returns true if flag shorthand data is valid.
 func (f Flag) Shorthanded() bool {
 	return f.ValidateShorthand() == nil
 }

pkg/kubectl/plugins/plugins_test.go

@@ -39,11 +39,11 @@ func TestPlugin(t *testing.T) {
 					ShortDesc: "The test",
 				},
 			},
-			expectedErr: IncompletePluginError,
+			expectedErr: ErrIncompletePlugin,
 		},
 		{
 			plugin:      &Plugin{},
-			expectedErr: IncompletePluginError,
+			expectedErr: ErrIncompletePlugin,
 		},
 		{
 			plugin: &Plugin{
@@ -53,7 +53,7 @@ func TestPlugin(t *testing.T) {
 					Command:   "echo 1",
 				},
 			},
-			expectedErr: InvalidPluginNameError,
+			expectedErr: ErrInvalidPluginName,
 		},
 		{
 			plugin: &Plugin{
@@ -68,7 +68,7 @@ func TestPlugin(t *testing.T) {
 					},
 				},
 			},
-			expectedErr: IncompleteFlagError,
+			expectedErr: ErrIncompleteFlag,
 		},
 		{
 			plugin: &Plugin{
@@ -84,7 +84,7 @@ func TestPlugin(t *testing.T) {
 					},
 				},
 			},
-			expectedErr: InvalidFlagNameError,
+			expectedErr: ErrInvalidFlagName,
 		},
 		{
 			plugin: &Plugin{
@@ -101,7 +101,7 @@ func TestPlugin(t *testing.T) {
 					},
 				},
 			},
-			expectedErr: InvalidFlagShorthandError,
+			expectedErr: ErrInvalidFlagShorthand,
 		},
 		{
 			plugin: &Plugin{
@@ -118,7 +118,7 @@ func TestPlugin(t *testing.T) {
 					},
 				},
 			},
-			expectedErr: InvalidFlagShorthandError,
+			expectedErr: ErrInvalidFlagShorthand,
 		},
 		{
 			plugin: &Plugin{