From 6efb12f8c577e879e8e4347b3e782b1f7f2f1752 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Apr 2026 07:43:51 +0000 Subject: [PATCH] Address second round of reviewer feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - exec.go: rename UpdatePlugin parameter path→repo for clarity - exec.go: fix uninstallPlugin to only emit INFO log when err == nil - exec_test.go: add Test_helmSecretsRequiresSplitInstall table-driven tests covering v4.6.9, v4.7.0, v4.8.0, v4.10.0, pre-release, invalid and empty - exec_test.go: add Test_UpdatePlugin_Helm4SecretsUsesUninstallReinstall verifying that Helm 4 + secrets uses uninstall+reinstall (not plugin update) Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/cbd3f8c9-ec7d-4500-b168-cb1c2f7c87bc Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> --- pkg/helmexec/exec.go | 8 ++-- pkg/helmexec/exec_test.go | 97 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 3 deletions(-) diff --git a/pkg/helmexec/exec.go b/pkg/helmexec/exec.go index 17330daf..ee8cdad1 100644 --- a/pkg/helmexec/exec.go +++ b/pkg/helmexec/exec.go @@ -923,11 +923,13 @@ func helmSecretsRequiresSplitInstall(version string) bool { func (helm *execer) uninstallPlugin(name string) error { helm.logger.Infof("Uninstalling helm plugin %v", name) out, err := helm.exec([]string{"plugin", "uninstall", name}, map[string]string{}, nil) - helm.info(out) + if err == nil { + helm.info(out) + } return err } -func (helm *execer) UpdatePlugin(name, path, version string) error { +func (helm *execer) UpdatePlugin(name, repo, version string) error { helm.logger.Infof("Updating helm plugin %v", name) // Special handling for helm-secrets 4.7.0+ with Helm 4 which uses split plugin architecture @@ -951,7 +953,7 @@ func (helm *execer) UpdatePlugin(name, path, version string) error { if uninstallErr := helm.uninstallPlugin(name); uninstallErr != nil { helm.logger.Warnf("Failed to uninstall helm plugin %v: %v", name, uninstallErr) } - if reinstallErr := helm.AddPlugin(name, path, version); reinstallErr != nil { + if reinstallErr := helm.AddPlugin(name, repo, version); reinstallErr != nil { return fmt.Errorf("helm plugin update failed (%w) and reinstall also failed: %w", updateErr, reinstallErr) } return nil diff --git a/pkg/helmexec/exec_test.go b/pkg/helmexec/exec_test.go index c468c8f5..9644c21a 100644 --- a/pkg/helmexec/exec_test.go +++ b/pkg/helmexec/exec_test.go @@ -1547,3 +1547,100 @@ func TestParseHelmVersion(t *testing.T) { }) } } + +func Test_helmSecretsRequiresSplitInstall(t *testing.T) { + tests := []struct { + name string + version string + want bool + }{ + {name: "below threshold", version: "v4.6.9", want: false}, + {name: "exactly at threshold", version: "v4.7.0", want: true}, + {name: "above threshold single digit minor", version: "v4.8.0", want: true}, + {name: "above threshold double digit minor (v4.10.0+)", version: "v4.10.0", want: true}, + {name: "pre-release below threshold", version: "v4.7.0-beta.1", want: false}, + {name: "invalid version string", version: "not-a-version", want: false}, + {name: "empty string", version: "", want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := helmSecretsRequiresSplitInstall(tt.version) + assert.Equal(t, tt.want, got) + }) + } +} + +type funcRunner struct { + execute func(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) +} + +func (r *funcRunner) ExecuteStdIn(cmd string, args []string, env map[string]string, stdin io.Reader) ([]byte, error) { + return r.execute(cmd, args, env, false) +} + +func (r *funcRunner) Execute(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) { + return r.execute(cmd, args, env, enableLiveOutput) +} + +func Test_UpdatePlugin_Helm4SecretsUsesUninstallReinstall(t *testing.T) { + var calledArgs [][]string + runner := &funcRunner{ + execute: func(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) { + calledArgs = append(calledArgs, append([]string(nil), args...)) + return []byte{}, nil + }, + } + + var buffer bytes.Buffer + logger := NewLogger(&buffer, "debug") + helm := &execer{ + helmBinary: "helm", + version: semver.MustParse("4.0.0"), + logger: logger, + kubeconfig: "config", + kubeContext: "dev", + runner: runner, + } + + err := helm.UpdatePlugin("secrets", "https://github.com/jkroepke/helm-secrets", "v4.7.0") + require.NoError(t, err) + + // Verify that "plugin update" was NOT called (the Helm 4 secrets path should skip it). + for _, args := range calledArgs { + for i, a := range args { + if a == "plugin" && i+1 < len(args) && args[i+1] == "update" { + t.Errorf("expected 'plugin update' to not be called for Helm 4 secrets, but it was: %v", args) + } + } + } + + // Verify that uninstall was called for all three split plugins. + checkUninstall := func(name string) { + for _, args := range calledArgs { + for i, a := range args { + if a == "plugin" && i+2 < len(args) && args[i+1] == "uninstall" && args[i+2] == name { + return + } + } + } + t.Errorf("expected 'plugin uninstall %s' to be called", name) + } + checkUninstall("secrets") + checkUninstall("secrets-getter") + checkUninstall("secrets-post-renderer") + + // Verify that install was called for all three split plugin tarballs. + checkInstall := func(urlSubstring string) { + for _, args := range calledArgs { + for i, a := range args { + if a == "plugin" && i+2 < len(args) && args[i+1] == "install" && strings.Contains(args[i+2], urlSubstring) { + return + } + } + } + t.Errorf("expected 'plugin install' for %q to be called", urlSubstring) + } + checkInstall("secrets-4.7.0.tgz") + checkInstall("secrets-getter-4.7.0.tgz") + checkInstall("secrets-post-renderer-4.7.0.tgz") +}