From 66f6d283e0d40c638071c1fdfa733be8110601e2 Mon Sep 17 00:00:00 2001 From: tcharding Date: Thu, 31 Aug 2017 15:30:27 +1000 Subject: [PATCH 1/5] Remove plugins entry from hack/.golint_failures --- hack/.golint_failures | 1 - 1 file changed, 1 deletion(-) diff --git a/hack/.golint_failures b/hack/.golint_failures index 3098d5667b3..a4b8f8e5376 100644 --- a/hack/.golint_failures +++ b/hack/.golint_failures @@ -218,7 +218,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 From a3627bdac81693db2d38b54bcc84bd88367c5a3e Mon Sep 17 00:00:00 2001 From: tcharding Date: Thu, 31 Aug 2017 15:30:46 +1000 Subject: [PATCH 2/5] Fix documentation golint warnings Fix golint warnings for load.go Fix golint warnings for plugins.go --- pkg/kubectl/plugins/env.go | 26 ++++++++++++++++++++------ pkg/kubectl/plugins/loader.go | 15 ++++++++++----- pkg/kubectl/plugins/plugins.go | 9 +++++++-- 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/pkg/kubectl/plugins/env.go b/pkg/kubectl/plugins/env.go index f7becb52b19..7845b5fe99b 100644 --- a/pkg/kubectl/plugins/env.go +++ b/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()} diff --git a/pkg/kubectl/plugins/loader.go b/pkg/kubectl/plugins/loader.go index 74899e1d5ec..aef346e0488 100644 --- a/pkg/kubectl/plugins/loader.go +++ b/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,13 +137,13 @@ func PathFromEnvVarPluginLoader(envVarName string, subdirs ...string) PluginLoad return loader } -// PluginsEnvVarPluginLoader is a PluginLoader that loads plugins from one or more +// PluginsEnvVarPluginLoader returns a PluginLoader that loads plugins from one or more // directories specified by the KUBECTL_PLUGINS_PATH env var. func PluginsEnvVarPluginLoader() PluginLoader { return PathFromEnvVarPluginLoader("KUBECTL_PLUGINS_PATH") } -// XDGDataPluginLoader is a PluginLoader that loads plugins from one or more +// XDGDataPluginLoader 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". @@ -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 } diff --git a/pkg/kubectl/plugins/plugins.go b/pkg/kubectl/plugins/plugins.go index e0bea1a3b46..6badf63d14d 100644 --- a/pkg/kubectl/plugins/plugins.go +++ b/pkg/kubectl/plugins/plugins.go @@ -37,7 +37,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,12 +49,13 @@ 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 @@ -75,6 +76,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,6 +92,7 @@ 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 @@ -100,6 +103,7 @@ func (f Flag) Validate() error { 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]))) { @@ -108,6 +112,7 @@ func (f Flag) ValidateShorthand() error { return InvalidFlagShorthandError } +// Shorthanded returns true if flag shorthand data is valid. func (f Flag) Shorthanded() bool { return f.ValidateShorthand() == nil } From 86f1d74a6931310bbfd784bef88fa89d70497b93 Mon Sep 17 00:00:00 2001 From: tcharding Date: Thu, 31 Aug 2017 15:59:52 +1000 Subject: [PATCH 3/5] Change error variable identifiers to ErrFoo --- pkg/kubectl/plugins/plugins.go | 25 +++++++++++++++---------- pkg/kubectl/plugins/plugins_test.go | 14 +++++++------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/pkg/kubectl/plugins/plugins.go b/pkg/kubectl/plugins/plugins.go index 6badf63d14d..f2cc17847b3 100644 --- a/pkg/kubectl/plugins/plugins.go +++ b/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). @@ -58,10 +63,10 @@ type Source struct { // 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 { @@ -95,10 +100,10 @@ type Flag struct { // 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() } @@ -109,7 +114,7 @@ func (f Flag) ValidateShorthand() error { 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. diff --git a/pkg/kubectl/plugins/plugins_test.go b/pkg/kubectl/plugins/plugins_test.go index 1bbe44a5cd5..b5a43573ee3 100644 --- a/pkg/kubectl/plugins/plugins_test.go +++ b/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{ From 91eaa8a30874c3efa0307b7b10d1ef5e5eafe59d Mon Sep 17 00:00:00 2001 From: tcharding Date: Thu, 31 Aug 2017 16:36:17 +1000 Subject: [PATCH 4/5] Rename PluginsEnvVarPluginLoader to stop stutter --- pkg/kubectl/cmd/util/factory_builder.go | 2 +- pkg/kubectl/plugins/loader.go | 4 ++-- pkg/kubectl/plugins/loader_test.go | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/kubectl/cmd/util/factory_builder.go b/pkg/kubectl/cmd/util/factory_builder.go index e3a270fee30..c42df2699c4 100644 --- a/pkg/kubectl/cmd/util/factory_builder.go +++ b/pkg/kubectl/cmd/util/factory_builder.go @@ -180,7 +180,7 @@ func (f *ring2Factory) NewUnstructuredBuilder(allowRemoteCalls bool) (*resource. // 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(), diff --git a/pkg/kubectl/plugins/loader.go b/pkg/kubectl/plugins/loader.go index aef346e0488..def8ae6e5ea 100644 --- a/pkg/kubectl/plugins/loader.go +++ b/pkg/kubectl/plugins/loader.go @@ -137,9 +137,9 @@ func PathFromEnvVarPluginLoader(envVarName string, subdirs ...string) PluginLoad return loader } -// PluginsEnvVarPluginLoader returns 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") } diff --git a/pkg/kubectl/plugins/loader_test.go b/pkg/kubectl/plugins/loader_test.go index b89ee671bfa..34f6b1c6abf 100644 --- a/pkg/kubectl/plugins/loader_test.go +++ b/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 { From 0d464cac2f8fddf34a862d484999ed31afbaea84 Mon Sep 17 00:00:00 2001 From: tcharding Date: Thu, 31 Aug 2017 16:37:39 +1000 Subject: [PATCH 5/5] Rename XDGDataPluginLoader to be uniform --- pkg/kubectl/cmd/util/factory_builder.go | 2 +- pkg/kubectl/plugins/loader.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/kubectl/cmd/util/factory_builder.go b/pkg/kubectl/cmd/util/factory_builder.go index c42df2699c4..d12a4afe52b 100644 --- a/pkg/kubectl/cmd/util/factory_builder.go +++ b/pkg/kubectl/cmd/util/factory_builder.go @@ -183,7 +183,7 @@ func (f *ring2Factory) PluginLoader() plugins.PluginLoader { return plugins.KubectlPluginsPathPluginLoader() } return plugins.TolerantMultiPluginLoader{ - plugins.XDGDataPluginLoader(), + plugins.XDGDataDirsPluginLoader(), plugins.UserDirPluginLoader(), } } diff --git a/pkg/kubectl/plugins/loader.go b/pkg/kubectl/plugins/loader.go index def8ae6e5ea..6cbaffc34de 100644 --- a/pkg/kubectl/plugins/loader.go +++ b/pkg/kubectl/plugins/loader.go @@ -143,11 +143,11 @@ func KubectlPluginsPathPluginLoader() PluginLoader { return PathFromEnvVarPluginLoader("KUBECTL_PLUGINS_PATH") } -// XDGDataPluginLoader returns 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")