From 6344f1d8e995468f9db5f0e9421dc6f09e26b034 Mon Sep 17 00:00:00 2001 From: Sushil Kumar Date: Wed, 3 May 2017 13:37:47 -0700 Subject: [PATCH 1/5] Errors out in case requested plugin version does not exists Fixes issues/2384 - helm plugin install installs a default version in case requested version is not available --- pkg/plugin/installer/vcs_installer.go | 4 ++- pkg/plugin/installer/vcs_installer_test.go | 35 ++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/pkg/plugin/installer/vcs_installer.go b/pkg/plugin/installer/vcs_installer.go index ce7a93b83..f9f181662 100644 --- a/pkg/plugin/installer/vcs_installer.go +++ b/pkg/plugin/installer/vcs_installer.go @@ -16,6 +16,7 @@ limitations under the License. package installer // import "k8s.io/helm/pkg/plugin/installer" import ( + "fmt" "os" "sort" @@ -112,7 +113,8 @@ func (i *VCSInstaller) solveVersion(repo vcs.Repo) (string, error) { return ver, nil } } - return "", nil + + return "", fmt.Errorf("requested version %q does not exist for plugin %q", i.Version, i.Repo.Remote()) } // setVersion attempts to checkout the version diff --git a/pkg/plugin/installer/vcs_installer_test.go b/pkg/plugin/installer/vcs_installer_test.go index e340bbe5a..53da07ad4 100644 --- a/pkg/plugin/installer/vcs_installer_test.go +++ b/pkg/plugin/installer/vcs_installer_test.go @@ -22,6 +22,8 @@ import ( "k8s.io/helm/pkg/helm/helmpath" + "fmt" + "github.com/Masterminds/vcs" ) @@ -88,3 +90,36 @@ func TestVCSInstaller(t *testing.T) { t.Errorf("expected path '$HELM_HOME/plugins/helm-env', got %q", i.Path()) } } + +func TestVCSInstallerNonExistentVersion(t *testing.T) { + hh, err := ioutil.TempDir("", "helm-home-") + if err != nil { + t.Fatal(err) + } + defer os.Remove(hh) + + home := helmpath.Home(hh) + if err := os.MkdirAll(home.Plugins(), 0755); err != nil { + t.Fatalf("Could not create %s: %s", home.Plugins(), err) + } + + source := "https://github.com/adamreese/helm-env" + version := "0.2.0" + + i, err := NewForSource(source, version, home) + if err != nil { + t.Errorf("unexpected error: %s", err) + } + + // ensure a VCSInstaller was returned + _, ok := i.(*VCSInstaller) + if !ok { + t.Error("expected a VCSInstaller") + } + + if err := Install(i); err == nil { + t.Error("expected error for version does not exists, got none") + } else if err.Error() != fmt.Sprintf("requested version %q does not exist for plugin %q", version, source) { + t.Errorf("expected error for version does not exists, got (%v)", err) + } +} From c84fb11a68b00cb24f75bd4c16f536b93c484172 Mon Sep 17 00:00:00 2001 From: Sushil Kumar Date: Wed, 3 May 2017 16:20:28 -0700 Subject: [PATCH 2/5] Errors out in case requested plugin exists Partially fixes issues/2385 - helm install silently updates the plugin, if it pre-existed --- pkg/plugin/installer/installer.go | 4 ++++ pkg/plugin/installer/local_installer_test.go | 4 ++-- pkg/plugin/installer/vcs_installer_test.go | 15 ++++++++++++--- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/pkg/plugin/installer/installer.go b/pkg/plugin/installer/installer.go index 31ef9ae53..8da6e24e5 100644 --- a/pkg/plugin/installer/installer.go +++ b/pkg/plugin/installer/installer.go @@ -40,6 +40,10 @@ type Installer interface { // Install installs a plugin to $HELM_HOME. func Install(i Installer) error { + if _, pathErr := os.Stat(i.Path()); !os.IsNotExist(pathErr) { + return errors.New("plugin already exists") + } + return i.Install() } diff --git a/pkg/plugin/installer/local_installer_test.go b/pkg/plugin/installer/local_installer_test.go index a2a1b541c..6a7c957d6 100644 --- a/pkg/plugin/installer/local_installer_test.go +++ b/pkg/plugin/installer/local_installer_test.go @@ -31,7 +31,7 @@ func TestLocalInstaller(t *testing.T) { if err != nil { t.Fatal(err) } - defer os.Remove(hh) + defer os.RemoveAll(hh) home := helmpath.Home(hh) if err := os.MkdirAll(home.Plugins(), 0755); err != nil { @@ -43,7 +43,7 @@ func TestLocalInstaller(t *testing.T) { if err != nil { t.Fatal(err) } - defer os.Remove(tdir) + defer os.RemoveAll(tdir) if err := ioutil.WriteFile(filepath.Join(tdir, "plugin.yaml"), []byte{}, 0644); err != nil { t.Fatal(err) } diff --git a/pkg/plugin/installer/vcs_installer_test.go b/pkg/plugin/installer/vcs_installer_test.go index 53da07ad4..31d9278bb 100644 --- a/pkg/plugin/installer/vcs_installer_test.go +++ b/pkg/plugin/installer/vcs_installer_test.go @@ -18,6 +18,7 @@ package installer // import "k8s.io/helm/pkg/plugin/installer" import ( "io/ioutil" "os" + "path/filepath" "testing" "k8s.io/helm/pkg/helm/helmpath" @@ -53,7 +54,7 @@ func TestVCSInstaller(t *testing.T) { if err != nil { t.Fatal(err) } - defer os.Remove(hh) + defer os.RemoveAll(hh) home := helmpath.Home(hh) if err := os.MkdirAll(home.Plugins(), 0755); err != nil { @@ -61,8 +62,9 @@ func TestVCSInstaller(t *testing.T) { } source := "https://github.com/adamreese/helm-env" + testRepoPath, _ := filepath.Abs("../testdata/plugdir/echo") repo := &testRepo{ - local: "../testdata/plugdir/echo", + local: testRepoPath, tags: []string{"0.1.0", "0.1.1"}, } @@ -89,6 +91,13 @@ func TestVCSInstaller(t *testing.T) { if i.Path() != home.Path("plugins", "helm-env") { t.Errorf("expected path '$HELM_HOME/plugins/helm-env', got %q", i.Path()) } + + // Install again to test plugin exists error + if err := Install(i); err == nil { + t.Error("expected error for plugin exists, got none") + } else if err.Error() != "plugin already exists" { + t.Errorf("expected error for plugin exists, got (%v)", err) + } } func TestVCSInstallerNonExistentVersion(t *testing.T) { @@ -96,7 +105,7 @@ func TestVCSInstallerNonExistentVersion(t *testing.T) { if err != nil { t.Fatal(err) } - defer os.Remove(hh) + defer os.RemoveAll(hh) home := helmpath.Home(hh) if err := os.MkdirAll(home.Plugins(), 0755); err != nil { From 1c5aab8e7853e33de550cc75edb7a1e7aa88a8cc Mon Sep 17 00:00:00 2001 From: Sushil Kumar Date: Thu, 4 May 2017 21:36:43 -0700 Subject: [PATCH 3/5] Fixes messages for plugin remove option Fixes issues/2398 - helm plugin remove does not works as expected - [ ] plugin remove option is coded to remove multiple plugins, but instead returns error when more than one plugin is requested to be removed. - [ ] plugin remove does not show any error/message for non-existent plugin. --- cmd/helm/plugin_remove.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/cmd/helm/plugin_remove.go b/cmd/helm/plugin_remove.go index da2041f89..6eec206d5 100644 --- a/cmd/helm/plugin_remove.go +++ b/cmd/helm/plugin_remove.go @@ -16,6 +16,7 @@ limitations under the License. package main import ( + "errors" "fmt" "io" "os" @@ -48,8 +49,8 @@ func newPluginRemoveCmd(out io.Writer) *cobra.Command { } func (pcmd *pluginRemoveCmd) complete(args []string) error { - if err := checkArgsLength(len(args), "plugin"); err != nil { - return err + if len(args) == 0 { + return errors.New("please provide plugin name to remove") } pcmd.names = args pcmd.home = settings.Home @@ -67,9 +68,12 @@ func (pcmd *pluginRemoveCmd) run() error { for _, name := range pcmd.names { if found := findPlugin(plugins, name); found != nil { if err := removePlugin(found, pcmd.home); err != nil { - return err + fmt.Fprintf(pcmd.out, "Failed to remove plugin %s, got error (%v)\n", name, err) + } else { + fmt.Fprintf(pcmd.out, "Removed plugin: %s\n", name) } - fmt.Fprintf(pcmd.out, "Removed plugin: %s\n", name) + } else { + fmt.Fprintf(pcmd.out, "Plugin: %s not found\n", name) } } return nil From 24157e4aedefd2fed72fcb4c2274384588a7ca52 Mon Sep 17 00:00:00 2001 From: Sushil Kumar Date: Sat, 6 May 2017 15:59:20 -0700 Subject: [PATCH 4/5] Updated review comments :) --- pkg/plugin/installer/vcs_installer_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/plugin/installer/vcs_installer_test.go b/pkg/plugin/installer/vcs_installer_test.go index 31d9278bb..081598c7c 100644 --- a/pkg/plugin/installer/vcs_installer_test.go +++ b/pkg/plugin/installer/vcs_installer_test.go @@ -16,6 +16,7 @@ limitations under the License. package installer // import "k8s.io/helm/pkg/plugin/installer" import ( + "fmt" "io/ioutil" "os" "path/filepath" @@ -23,8 +24,6 @@ import ( "k8s.io/helm/pkg/helm/helmpath" - "fmt" - "github.com/Masterminds/vcs" ) From 084bbfa2baa6a76bd7517b36c3416a3d84636438 Mon Sep 17 00:00:00 2001 From: Sushil Kumar Date: Mon, 8 May 2017 21:11:38 -0700 Subject: [PATCH 5/5] Return error exit-code in case of error --- cmd/helm/plugin_remove.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cmd/helm/plugin_remove.go b/cmd/helm/plugin_remove.go index 6eec206d5..e7e522d51 100644 --- a/cmd/helm/plugin_remove.go +++ b/cmd/helm/plugin_remove.go @@ -20,6 +20,7 @@ import ( "fmt" "io" "os" + "strings" "k8s.io/helm/pkg/helm/helmpath" "k8s.io/helm/pkg/plugin" @@ -64,18 +65,21 @@ func (pcmd *pluginRemoveCmd) run() error { if err != nil { return err } - + var errorPlugins []string for _, name := range pcmd.names { if found := findPlugin(plugins, name); found != nil { if err := removePlugin(found, pcmd.home); err != nil { - fmt.Fprintf(pcmd.out, "Failed to remove plugin %s, got error (%v)\n", name, err) + errorPlugins = append(errorPlugins, fmt.Sprintf("Failed to remove plugin %s, got error (%v)", name, err)) } else { fmt.Fprintf(pcmd.out, "Removed plugin: %s\n", name) } } else { - fmt.Fprintf(pcmd.out, "Plugin: %s not found\n", name) + errorPlugins = append(errorPlugins, fmt.Sprintf("Plugin: %s not found", name)) } } + if len(errorPlugins) > 0 { + return fmt.Errorf(strings.Join(errorPlugins, "\n")) + } return nil }