Address second round of reviewer feedback

- 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>
This commit is contained in:
copilot-swe-agent[bot] 2026-04-24 07:43:51 +00:00 committed by GitHub
parent a31ee32e9e
commit 6efb12f8c5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 102 additions and 3 deletions

View File

@ -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

View File

@ -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")
}