From 5926ec83dd4760d02f316652a801f0812af39d87 Mon Sep 17 00:00:00 2001 From: George Jenkins Date: Fri, 22 Aug 2025 15:06:09 -0700 Subject: [PATCH] Remove SetupPluginEnv Signed-off-by: George Jenkins --- cmd/helm/helm.go | 8 +- cmd/helm/helm_test.go | 20 +-- internal/plugin/error.go | 4 +- internal/plugin/plugin_test.go | 2 + internal/plugin/runtime.go | 9 ++ internal/plugin/runtime_extismv1.go | 2 +- internal/plugin/runtime_subprocess.go | 117 ++++++++++-------- internal/plugin/runtime_subprocess_getter.go | 34 ++--- internal/plugin/runtime_subprocess_test.go | 92 ++++++++------ internal/plugin/runtime_test.go | 37 ++++++ internal/plugin/schema/postrenderer.go | 5 +- internal/plugin/subprocess_commands.go | 6 +- internal/plugin/subprocess_commands_test.go | 31 +++-- pkg/cmd/load_plugins.go | 16 +-- pkg/cmd/plugin.go | 1 - pkg/cmd/plugin_test.go | 86 +++++++------ pkg/cmd/root.go | 5 + .../helmhome/helm/plugins/env/plugin-name.sh | 3 + .../helmhome/helm/plugins/env/plugin.yaml | 2 +- .../helmhome/helm/plugins/fullenv/fullenv.sh | 12 +- pkg/postrenderer/postrenderer.go | 1 - pkg/postrenderer/postrenderer_test.go | 8 -- 22 files changed, 296 insertions(+), 205 deletions(-) create mode 100755 pkg/cmd/testdata/helmhome/helm/plugins/env/plugin-name.sh diff --git a/cmd/helm/helm.go b/cmd/helm/helm.go index 05e7e7ba2..66d342500 100644 --- a/cmd/helm/helm.go +++ b/cmd/helm/helm.go @@ -41,11 +41,9 @@ func main() { } if err := cmd.Execute(); err != nil { - switch e := err.(type) { - case helmcmd.PluginError: - os.Exit(e.Code) - default: - os.Exit(1) + if cerr, ok := err.(helmcmd.CommandError); ok { + os.Exit(cerr.ExitCode) } + os.Exit(1) } } diff --git a/cmd/helm/helm_test.go b/cmd/helm/helm_test.go index 5431daad0..0458e8037 100644 --- a/cmd/helm/helm_test.go +++ b/cmd/helm/helm_test.go @@ -22,11 +22,13 @@ import ( "os/exec" "runtime" "testing" + + "github.com/stretchr/testify/assert" ) -func TestPluginExitCode(t *testing.T) { +func TestCliPluginExitCode(t *testing.T) { if os.Getenv("RUN_MAIN_FOR_TESTING") == "1" { - os.Args = []string{"helm", "exitwith", "2"} + os.Args = []string{"helm", "exitwith", "43"} // We DO call helm's main() here. So this looks like a normal `helm` process. main() @@ -43,7 +45,7 @@ func TestPluginExitCode(t *testing.T) { // So that the second run is able to run main() and this first run can verify the exit status returned by that. // // This technique originates from https://talks.golang.org/2014/testing.slide#23. - cmd := exec.Command(os.Args[0], "-test.run=TestPluginExitCode") + cmd := exec.Command(os.Args[0], "-test.run=TestCliPluginExitCode") cmd.Env = append( os.Environ(), "RUN_MAIN_FOR_TESTING=1", @@ -57,23 +59,21 @@ func TestPluginExitCode(t *testing.T) { cmd.Stdout = stdout cmd.Stderr = stderr err := cmd.Run() - exiterr, ok := err.(*exec.ExitError) + exiterr, ok := err.(*exec.ExitError) if !ok { - t.Fatalf("Unexpected error returned by os.Exit: %T", err) + t.Fatalf("Unexpected error type returned by os.Exit: %T", err) } - if stdout.String() != "" { - t.Errorf("Expected no write to stdout: Got %q", stdout.String()) - } + assert.Empty(t, stdout.String()) expectedStderr := "Error: plugin \"exitwith\" exited with error\n" if stderr.String() != expectedStderr { t.Errorf("Expected %q written to stderr: Got %q", expectedStderr, stderr.String()) } - if exiterr.ExitCode() != 2 { - t.Errorf("Expected exit code 2: Got %d", exiterr.ExitCode()) + if exiterr.ExitCode() != 43 { + t.Errorf("Expected exit code 43: Got %d", exiterr.ExitCode()) } } } diff --git a/internal/plugin/error.go b/internal/plugin/error.go index 5ace680cb..212460cea 100644 --- a/internal/plugin/error.go +++ b/internal/plugin/error.go @@ -19,8 +19,8 @@ package plugin // - subprocess plugin: child process exit code // - extism plugin: wasm function return code type InvokeExecError struct { - Err error // Underlying error - Code int // Exeit code from plugin code execution + ExitCode int // Exit code from plugin code execution + Err error // Underlying error } // Error implements the error interface diff --git a/internal/plugin/plugin_test.go b/internal/plugin/plugin_test.go index bddabd136..a4de8e52a 100644 --- a/internal/plugin/plugin_test.go +++ b/internal/plugin/plugin_test.go @@ -24,11 +24,13 @@ func mockSubprocessCLIPlugin(t *testing.T, pluginName string) *SubprocessPluginR rc := RuntimeConfigSubprocess{ PlatformCommand: []PlatformCommand{ + {OperatingSystem: "darwin", Architecture: "", Command: "sh", Args: []string{"-c", "echo \"mock plugin\""}}, {OperatingSystem: "linux", Architecture: "", Command: "sh", Args: []string{"-c", "echo \"mock plugin\""}}, {OperatingSystem: "windows", Architecture: "", Command: "pwsh", Args: []string{"-c", "echo \"mock plugin\""}}, }, PlatformHooks: map[string][]PlatformCommand{ Install: { + {OperatingSystem: "darwin", Architecture: "", Command: "sh", Args: []string{"-c", "echo \"installing...\""}}, {OperatingSystem: "linux", Architecture: "", Command: "sh", Args: []string{"-c", "echo \"installing...\""}}, {OperatingSystem: "windows", Architecture: "", Command: "pwsh", Args: []string{"-c", "echo \"installing...\""}}, }, diff --git a/internal/plugin/runtime.go b/internal/plugin/runtime.go index a9c01a380..b2ff0b7ca 100644 --- a/internal/plugin/runtime.go +++ b/internal/plugin/runtime.go @@ -16,6 +16,7 @@ limitations under the License. package plugin import ( + "fmt" "strings" "go.yaml.in/yaml/v3" @@ -73,3 +74,11 @@ func parseEnv(env []string) map[string]string { } return result } + +func formatEnv(env map[string]string) []string { + result := make([]string, 0, len(env)) + for key, value := range env { + result = append(result, fmt.Sprintf("%s=%s", key, value)) + } + return result +} diff --git a/internal/plugin/runtime_extismv1.go b/internal/plugin/runtime_extismv1.go index c0122d08f..b5cc79a6f 100644 --- a/internal/plugin/runtime_extismv1.go +++ b/internal/plugin/runtime_extismv1.go @@ -196,7 +196,7 @@ func (p *ExtismV1PluginRuntime) Invoke(ctx context.Context, input *Input) (*Outp if exitCode != 0 { return nil, &InvokeExecError{ - Code: int(exitCode), + ExitCode: int(exitCode), } } diff --git a/internal/plugin/runtime_subprocess.go b/internal/plugin/runtime_subprocess.go index a1a698679..5e6676a00 100644 --- a/internal/plugin/runtime_subprocess.go +++ b/internal/plugin/runtime_subprocess.go @@ -21,12 +21,12 @@ import ( "fmt" "io" "log/slog" + "maps" "os" "os/exec" - "syscall" + "slices" "helm.sh/helm/v4/internal/plugin/schema" - "helm.sh/helm/v4/pkg/cli" ) // SubprocessProtocolCommand maps a given protocol to the getter command used to retrieve artifacts for that protcol @@ -62,7 +62,9 @@ func (r *RuntimeConfigSubprocess) Validate() error { return nil } -type RuntimeSubprocess struct{} +type RuntimeSubprocess struct { + EnvVars map[string]string +} var _ Runtime = (*RuntimeSubprocess)(nil) @@ -72,6 +74,7 @@ func (r *RuntimeSubprocess) CreatePlugin(pluginDir string, metadata *Metadata) ( metadata: *metadata, pluginDir: pluginDir, RuntimeConfig: *(metadata.RuntimeConfig.(*RuntimeConfigSubprocess)), + EnvVars: maps.Clone(r.EnvVars), }, nil } @@ -80,6 +83,7 @@ type SubprocessPluginRuntime struct { metadata Metadata pluginDir string RuntimeConfig RuntimeConfigSubprocess + EnvVars map[string]string } var _ Plugin = (*SubprocessPluginRuntime)(nil) @@ -109,22 +113,22 @@ func (r *SubprocessPluginRuntime) Invoke(_ context.Context, input *Input) (*Outp // This method allows execution with different command/args than the plugin's default func (r *SubprocessPluginRuntime) InvokeWithEnv(main string, argv []string, env []string, stdin io.Reader, stdout, stderr io.Writer) error { mainCmdExp := os.ExpandEnv(main) - prog := exec.Command(mainCmdExp, argv...) - prog.Env = env - prog.Stdin = stdin - prog.Stdout = stdout - prog.Stderr = stderr + cmd := exec.Command(mainCmdExp, argv...) + cmd.Env = slices.Clone(os.Environ()) + cmd.Env = append( + cmd.Env, + fmt.Sprintf("HELM_PLUGIN_NAME=%s", r.metadata.Name), + fmt.Sprintf("HELM_PLUGIN_DIR=%s", r.pluginDir)) + cmd.Env = append(cmd.Env, env...) + + cmd.Stdin = stdin + cmd.Stdout = stdout + cmd.Stderr = stderr - if err := prog.Run(); err != nil { - if eerr, ok := err.(*exec.ExitError); ok { - os.Stderr.Write(eerr.Stderr) - status := eerr.Sys().(syscall.WaitStatus) - return &InvokeExecError{ - Err: fmt.Errorf("plugin %q exited with error", r.metadata.Name), - Code: status.ExitStatus(), - } - } + if err := executeCmd(cmd, r.metadata.Name); err != nil { + return err } + return nil } @@ -135,15 +139,23 @@ func (r *SubprocessPluginRuntime) InvokeHook(event string) error { return nil } - main, argv, err := PrepareCommands(cmds, r.RuntimeConfig.expandHookArgs, []string{}) + env := parseEnv(os.Environ()) + maps.Insert(env, maps.All(r.EnvVars)) + env["HELM_PLUGIN_NAME"] = r.metadata.Name + env["HELM_PLUGIN_DIR"] = r.pluginDir + + main, argv, err := PrepareCommands(cmds, r.RuntimeConfig.expandHookArgs, []string{}, env) if err != nil { return err } - prog := exec.Command(main, argv...) - prog.Stdout, prog.Stderr = os.Stdout, os.Stderr + cmd := exec.Command(main, argv...) + cmd.Env = formatEnv(env) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr - if err := prog.Run(); err != nil { + slog.Debug("executing plugin hook command", slog.String("pluginName", r.metadata.Name), slog.String("command", cmd.String())) + if err := cmd.Run(); err != nil { if eerr, ok := err.(*exec.ExitError); ok { os.Stderr.Write(eerr.Stderr) return fmt.Errorf("plugin %s hook for %q exited with error", event, r.metadata.Name) @@ -159,10 +171,15 @@ func (r *SubprocessPluginRuntime) InvokeHook(event string) error { func executeCmd(prog *exec.Cmd, pluginName string) error { if err := prog.Run(); err != nil { if eerr, ok := err.(*exec.ExitError); ok { - os.Stderr.Write(eerr.Stderr) + slog.Debug( + "plugin execution failed", + slog.String("pluginName", pluginName), + slog.String("error", err.Error()), + slog.Int("exitCode", eerr.ExitCode()), + slog.String("stderr", string(bytes.TrimSpace(eerr.Stderr)))) return &InvokeExecError{ - Err: fmt.Errorf("plugin %q exited with error", pluginName), - Code: eerr.ExitCode(), + Err: fmt.Errorf("plugin %q exited with error", pluginName), + ExitCode: eerr.ExitCode(), } } @@ -181,14 +198,27 @@ func (r *SubprocessPluginRuntime) runCLI(input *Input) (*Output, error) { cmds := r.RuntimeConfig.PlatformCommand - command, args, err := PrepareCommands(cmds, true, extraArgs) + env := parseEnv(os.Environ()) + maps.Insert(env, maps.All(r.EnvVars)) + maps.Insert(env, maps.All(parseEnv(input.Env))) + env["HELM_PLUGIN_NAME"] = r.metadata.Name + env["HELM_PLUGIN_DIR"] = r.pluginDir + + command, args, err := PrepareCommands(cmds, true, extraArgs, env) if err != nil { return nil, fmt.Errorf("failed to prepare plugin command: %w", err) } - err2 := r.InvokeWithEnv(command, args, input.Env, input.Stdin, input.Stdout, input.Stderr) - if err2 != nil { - return nil, err2 + cmd := exec.Command(command, args...) + cmd.Env = formatEnv(env) + + cmd.Stdin = input.Stdin + cmd.Stdout = input.Stdout + cmd.Stderr = input.Stderr + + slog.Debug("executing plugin command", slog.String("pluginName", r.metadata.Name), slog.String("command", cmd.String())) + if err := executeCmd(cmd, r.metadata.Name); err != nil { + return nil, err } return &Output{ @@ -201,20 +231,19 @@ func (r *SubprocessPluginRuntime) runPostrenderer(input *Input) (*Output, error) return nil, fmt.Errorf("plugin %q input message does not implement InputMessagePostRendererV1", r.metadata.Name) } - msg := input.Message.(schema.InputMessagePostRendererV1) - extraArgs := msg.ExtraArgs - settings := msg.Settings - - // Setup plugin environment - SetupPluginEnv(settings, r.metadata.Name, r.pluginDir) + env := parseEnv(os.Environ()) + maps.Insert(env, maps.All(r.EnvVars)) + maps.Insert(env, maps.All(parseEnv(input.Env))) + env["HELM_PLUGIN_NAME"] = r.metadata.Name + env["HELM_PLUGIN_DIR"] = r.pluginDir + msg := input.Message.(schema.InputMessagePostRendererV1) cmds := r.RuntimeConfig.PlatformCommand - command, args, err := PrepareCommands(cmds, true, extraArgs) + command, args, err := PrepareCommands(cmds, true, msg.ExtraArgs, env) if err != nil { return nil, fmt.Errorf("failed to prepare plugin command: %w", err) } - // TODO de-duplicate code here by calling RuntimeSubprocess.invokeWithEnv() cmd := exec.Command( command, args...) @@ -232,12 +261,12 @@ func (r *SubprocessPluginRuntime) runPostrenderer(input *Input) (*Output, error) postRendered := &bytes.Buffer{} stderr := &bytes.Buffer{} - //cmd.Env = pluginExec.env + cmd.Env = formatEnv(env) cmd.Stdout = postRendered cmd.Stderr = stderr + slog.Debug("executing plugin command", slog.String("pluginName", r.metadata.Name), slog.String("command", cmd.String())) if err := executeCmd(cmd, r.metadata.Name); err != nil { - slog.Info("plugin execution failed", slog.String("stderr", stderr.String())) return nil, err } @@ -247,15 +276,3 @@ func (r *SubprocessPluginRuntime) runPostrenderer(input *Input) (*Output, error) }, }, nil } - -// SetupPluginEnv prepares os.Env for plugins. It operates on os.Env because -// the plugin subsystem itself needs access to the environment variables -// created here. -func SetupPluginEnv(settings *cli.EnvSettings, name, base string) { // TODO: remove - env := settings.EnvVars() - env["HELM_PLUGIN_NAME"] = name - env["HELM_PLUGIN_DIR"] = base - for key, val := range env { - os.Setenv(key, val) - } -} diff --git a/internal/plugin/runtime_subprocess_getter.go b/internal/plugin/runtime_subprocess_getter.go index d1884bc93..6a41b149f 100644 --- a/internal/plugin/runtime_subprocess_getter.go +++ b/internal/plugin/runtime_subprocess_getter.go @@ -18,6 +18,8 @@ package plugin import ( "bytes" "fmt" + "log/slog" + "maps" "os" "os/exec" "path/filepath" @@ -54,10 +56,20 @@ func (r *SubprocessPluginRuntime) runGetter(input *Input) (*Output, error) { return nil, fmt.Errorf("no downloader found for protocol %q", msg.Protocol) } - command, args, err := PrepareCommands(d.PlatformCommand, false, []string{}) + env := parseEnv(os.Environ()) + maps.Insert(env, maps.All(r.EnvVars)) + maps.Insert(env, maps.All(parseEnv(input.Env))) + env["HELM_PLUGIN_NAME"] = r.metadata.Name + env["HELM_PLUGIN_DIR"] = r.pluginDir + env["HELM_PLUGIN_USERNAME"] = msg.Options.Username + env["HELM_PLUGIN_PASSWORD"] = msg.Options.Password + env["HELM_PLUGIN_PASS_CREDENTIALS_ALL"] = fmt.Sprintf("%t", msg.Options.PassCredentialsAll) + + command, args, err := PrepareCommands(d.PlatformCommand, false, []string{}, env) if err != nil { return nil, fmt.Errorf("failed to prepare commands for protocol %q: %w", msg.Protocol, err) } + args = append( args, msg.Options.CertFile, @@ -65,24 +77,18 @@ func (r *SubprocessPluginRuntime) runGetter(input *Input) (*Output, error) { msg.Options.CAFile, msg.Href) - // TODO should we append to input.Env too? - env := append( - os.Environ(), - fmt.Sprintf("HELM_PLUGIN_USERNAME=%s", msg.Options.Username), - fmt.Sprintf("HELM_PLUGIN_PASSWORD=%s", msg.Options.Password), - fmt.Sprintf("HELM_PLUGIN_PASS_CREDENTIALS_ALL=%t", msg.Options.PassCredentialsAll)) - - // TODO should we pass along input.Stdout? buf := bytes.Buffer{} // subprocess getters are expected to write content to stdout pluginCommand := filepath.Join(r.pluginDir, command) - prog := exec.Command( + cmd := exec.Command( pluginCommand, args...) - prog.Env = env - prog.Stdout = &buf - prog.Stderr = os.Stderr - if err := executeCmd(prog, r.metadata.Name); err != nil { + cmd.Env = formatEnv(env) + cmd.Stdout = &buf + cmd.Stderr = os.Stderr + + slog.Debug("executing plugin command", slog.String("pluginName", r.metadata.Name), slog.String("command", cmd.String())) + if err := executeCmd(cmd, r.metadata.Name); err != nil { return nil, err } diff --git a/internal/plugin/runtime_subprocess_test.go b/internal/plugin/runtime_subprocess_test.go index 9d932816d..dab372027 100644 --- a/internal/plugin/runtime_subprocess_test.go +++ b/internal/plugin/runtime_subprocess_test.go @@ -16,49 +16,69 @@ limitations under the License. package plugin import ( + "fmt" "os" "path/filepath" "testing" - "helm.sh/helm/v4/pkg/cli" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.yaml.in/yaml/v3" + + "helm.sh/helm/v4/internal/plugin/schema" ) -func TestSetupEnv(t *testing.T) { - name := "pequod" - base := filepath.Join("testdata/helmhome/helm/plugins", name) - - s := cli.New() - s.PluginsDirectory = "testdata/helmhome/helm/plugins" - - SetupPluginEnv(s, name, base) - for _, tt := range []struct { - name, expect string - }{ - {"HELM_PLUGIN_NAME", name}, - {"HELM_PLUGIN_DIR", base}, - } { - if got := os.Getenv(tt.name); got != tt.expect { - t.Errorf("Expected $%s=%q, got %q", tt.name, tt.expect, got) - } +func mockSubprocessCLIPluginErrorExit(t *testing.T, pluginName string, exitCode uint8) *SubprocessPluginRuntime { + t.Helper() + + rc := RuntimeConfigSubprocess{ + PlatformCommand: []PlatformCommand{ + {Command: "sh", Args: []string{"-c", fmt.Sprintf("echo \"mock plugin $@\"; exit %d", exitCode)}}, + }, + } + + pluginDir := t.TempDir() + + md := Metadata{ + Name: pluginName, + Version: "v0.1.2", + Type: "cli/v1", + APIVersion: "v1", + Runtime: "subprocess", + Config: &ConfigCLI{ + Usage: "Mock plugin", + ShortHelp: "Mock plugin", + LongHelp: "Mock plugin for testing", + IgnoreFlags: false, + }, + RuntimeConfig: &rc, } -} -func TestSetupEnvWithSpace(t *testing.T) { - name := "sureshdsk" - base := filepath.Join("testdata/helm home/helm/plugins", name) - - s := cli.New() - s.PluginsDirectory = "testdata/helm home/helm/plugins" - - SetupPluginEnv(s, name, base) - for _, tt := range []struct { - name, expect string - }{ - {"HELM_PLUGIN_NAME", name}, - {"HELM_PLUGIN_DIR", base}, - } { - if got := os.Getenv(tt.name); got != tt.expect { - t.Errorf("Expected $%s=%q, got %q", tt.name, tt.expect, got) - } + data, err := yaml.Marshal(md) + require.NoError(t, err) + os.WriteFile(filepath.Join(pluginDir, "plugin.yaml"), data, 0o644) + + return &SubprocessPluginRuntime{ + metadata: md, + pluginDir: pluginDir, + RuntimeConfig: rc, } } + +func TestSubprocessPluginRuntime(t *testing.T) { + p := mockSubprocessCLIPluginErrorExit(t, "foo", 56) + + output, err := p.Invoke(t.Context(), &Input{ + Message: schema.InputMessageCLIV1{ + ExtraArgs: []string{"arg1", "arg2"}, + //Env: []string{"FOO=bar"}, + }, + }) + + require.Error(t, err) + ieerr, ok := err.(*InvokeExecError) + require.True(t, ok, "expected InvokeExecError, got %T", err) + assert.Equal(t, 56, ieerr.ExitCode) + + assert.Nil(t, output) +} diff --git a/internal/plugin/runtime_test.go b/internal/plugin/runtime_test.go index 8b72648b2..f8fe481c1 100644 --- a/internal/plugin/runtime_test.go +++ b/internal/plugin/runtime_test.go @@ -61,3 +61,40 @@ func TestParseEnv(t *testing.T) { }) } } + +func TestFormatEnv(t *testing.T) { + type testCase struct { + env map[string]string + expected []string + } + + testCases := map[string]testCase{ + "empty": { + env: map[string]string{}, + expected: []string{}, + }, + "single": { + env: map[string]string{"KEY": "value"}, + expected: []string{"KEY=value"}, + }, + "multiple": { + env: map[string]string{"KEY1": "value1", "KEY2": "value2"}, + expected: []string{"KEY1=value1", "KEY2=value2"}, + }, + "empty_key": { + env: map[string]string{"": "value1", "KEY2": "value2"}, + expected: []string{"=value1", "KEY2=value2"}, + }, + "empty_value": { + env: map[string]string{"KEY1": "value1", "KEY2": "", "KEY3": "value3"}, + expected: []string{"KEY1=value1", "KEY2=", "KEY3=value3"}, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + result := formatEnv(tc.env) + assert.ElementsMatch(t, tc.expected, result) + }) + } +} diff --git a/internal/plugin/schema/postrenderer.go b/internal/plugin/schema/postrenderer.go index 0f0c09369..82fd3059f 100644 --- a/internal/plugin/schema/postrenderer.go +++ b/internal/plugin/schema/postrenderer.go @@ -18,16 +18,13 @@ package schema import ( "bytes" - - "helm.sh/helm/v4/pkg/cli" ) // InputMessagePostRendererV1 implements Input.Message type InputMessagePostRendererV1 struct { Manifests *bytes.Buffer `json:"manifests"` // from CLI --post-renderer-args - ExtraArgs []string `json:"extraArgs"` - Settings *cli.EnvSettings `json:"settings"` + ExtraArgs []string `json:"extraArgs"` } type OutputMessagePostRendererV1 struct { diff --git a/internal/plugin/subprocess_commands.go b/internal/plugin/subprocess_commands.go index d979f98e3..e21ec2bab 100644 --- a/internal/plugin/subprocess_commands.go +++ b/internal/plugin/subprocess_commands.go @@ -77,13 +77,15 @@ func getPlatformCommand(cmds []PlatformCommand) ([]string, []string) { // returns the main command and an args array. // // The result is suitable to pass to exec.Command. -func PrepareCommands(cmds []PlatformCommand, expandArgs bool, extraArgs []string) (string, []string, error) { +func PrepareCommands(cmds []PlatformCommand, expandArgs bool, extraArgs []string, env map[string]string) (string, []string, error) { cmdParts, args := getPlatformCommand(cmds) if len(cmdParts) == 0 || cmdParts[0] == "" { return "", nil, fmt.Errorf("no plugin command is applicable") } - main := os.ExpandEnv(cmdParts[0]) + main := os.Expand(cmdParts[0], func(key string) string { + return env[key] + }) baseArgs := []string{} if len(cmdParts) > 1 { for _, cmdPart := range cmdParts[1:] { diff --git a/internal/plugin/subprocess_commands_test.go b/internal/plugin/subprocess_commands_test.go index 16446cdec..c1eba7a55 100644 --- a/internal/plugin/subprocess_commands_test.go +++ b/internal/plugin/subprocess_commands_test.go @@ -34,7 +34,8 @@ func TestPrepareCommand(t *testing.T) { {OperatingSystem: runtime.GOOS, Architecture: runtime.GOARCH, Command: cmdMain, Args: cmdArgs}, } - cmd, args, err := PrepareCommands(platformCommand, true, []string{}) + env := map[string]string{} + cmd, args, err := PrepareCommands(platformCommand, true, []string{}, env) if err != nil { t.Fatal(err) } @@ -91,7 +92,9 @@ func TestPrepareCommandExtraArgs(t *testing.T) { if tc.ignoreFlags { testExtraArgs = []string{} } - cmd, args, err := PrepareCommands(platformCommand, true, testExtraArgs) + + env := map[string]string{} + cmd, args, err := PrepareCommands(platformCommand, true, testExtraArgs, env) if err != nil { t.Fatal(err) } @@ -112,7 +115,8 @@ func TestPrepareCommands(t *testing.T) { {OperatingSystem: runtime.GOOS, Architecture: "", Command: "pwsh", Args: []string{"-c", "echo \"error\""}}, } - cmd, args, err := PrepareCommands(cmds, true, []string{}) + env := map[string]string{} + cmd, args, err := PrepareCommands(cmds, true, []string{}, env) if err != nil { t.Fatal(err) } @@ -138,7 +142,8 @@ func TestPrepareCommandsExtraArgs(t *testing.T) { expectedArgs := append(cmdArgs, extraArgs...) - cmd, args, err := PrepareCommands(cmds, true, extraArgs) + env := map[string]string{} + cmd, args, err := PrepareCommands(cmds, true, extraArgs, env) if err != nil { t.Fatal(err) } @@ -160,7 +165,8 @@ func TestPrepareCommandsNoArch(t *testing.T) { {OperatingSystem: runtime.GOOS, Architecture: "no-arch", Command: "pwsh", Args: []string{"-c", "echo \"error\""}}, } - cmd, args, err := PrepareCommands(cmds, true, []string{}) + env := map[string]string{} + cmd, args, err := PrepareCommands(cmds, true, []string{}, env) if err != nil { t.Fatal(err) } @@ -182,7 +188,8 @@ func TestPrepareCommandsNoOsNoArch(t *testing.T) { {OperatingSystem: runtime.GOOS, Architecture: "no-arch", Command: "pwsh", Args: []string{"-c", "echo \"error\""}}, } - cmd, args, err := PrepareCommands(cmds, true, []string{}) + env := map[string]string{} + cmd, args, err := PrepareCommands(cmds, true, []string{}, env) if err != nil { t.Fatal(err) } @@ -201,7 +208,8 @@ func TestPrepareCommandsNoMatch(t *testing.T) { {OperatingSystem: "no-os", Architecture: runtime.GOARCH, Command: "sh", Args: []string{"-c", "echo \"test\""}}, } - if _, _, err := PrepareCommands(cmds, true, []string{}); err == nil { + env := map[string]string{} + if _, _, err := PrepareCommands(cmds, true, []string{}, env); err == nil { t.Fatalf("Expected error to be returned") } } @@ -209,7 +217,8 @@ func TestPrepareCommandsNoMatch(t *testing.T) { func TestPrepareCommandsNoCommands(t *testing.T) { cmds := []PlatformCommand{} - if _, _, err := PrepareCommands(cmds, true, []string{}); err == nil { + env := map[string]string{} + if _, _, err := PrepareCommands(cmds, true, []string{}, env); err == nil { t.Fatalf("Expected error to be returned") } } @@ -224,7 +233,8 @@ func TestPrepareCommandsExpand(t *testing.T) { expectedArgs := []string{"-c", "echo \"test\""} - cmd, args, err := PrepareCommands(cmds, true, []string{}) + env := map[string]string{} + cmd, args, err := PrepareCommands(cmds, true, []string{}, env) if err != nil { t.Fatal(err) } @@ -244,7 +254,8 @@ func TestPrepareCommandsNoExpand(t *testing.T) { {OperatingSystem: "", Architecture: "", Command: cmdMain, Args: cmdArgs}, } - cmd, args, err := PrepareCommands(cmds, false, []string{}) + env := map[string]string{} + cmd, args, err := PrepareCommands(cmds, false, []string{}, env) if err != nil { t.Fatal(err) } diff --git a/pkg/cmd/load_plugins.go b/pkg/cmd/load_plugins.go index 5057c1033..75cfdc3cf 100644 --- a/pkg/cmd/load_plugins.go +++ b/pkg/cmd/load_plugins.go @@ -46,11 +46,6 @@ const ( pluginDynamicCompletionExecutable = "plugin.complete" ) -type PluginError struct { - error - Code int -} - // loadCLIPlugins loads CLI plugins into the command list. // // This follows a different pattern than the other commands because it has @@ -101,8 +96,6 @@ func loadCLIPlugins(baseCmd *cobra.Command, out io.Writer) { if err != nil { return err } - // Setup plugin environment - plugin.SetupPluginEnv(settings, plug.Metadata().Name, plug.Dir()) // For CLI plugin types runtime, set extra args and settings extraArgs := []string{} @@ -128,12 +121,10 @@ func loadCLIPlugins(baseCmd *cobra.Command, out io.Writer) { Stderr: os.Stderr, } _, err = plug.Invoke(context.Background(), input) - // TODO do we want to keep execErr here? if execErr, ok := err.(*plugin.InvokeExecError); ok { - // TODO can we replace cmd.PluginError with plugin.Error? - return PluginError{ - error: execErr.Err, - Code: execErr.Code, + return CommandError{ + error: execErr.Err, + ExitCode: execErr.ExitCode, } } return err @@ -369,7 +360,6 @@ func pluginDynamicComp(plug plugin.Plugin, cmd *cobra.Command, args []string, to argv = append(argv, u...) argv = append(argv, toComplete) } - plugin.SetupPluginEnv(settings, plug.Metadata().Name, plug.Dir()) cobra.CompDebugln(fmt.Sprintf("calling %s with args %v", main, argv), settings.Debug) buf := new(bytes.Buffer) diff --git a/pkg/cmd/plugin.go b/pkg/cmd/plugin.go index 393e9672c..ba904ef5f 100644 --- a/pkg/cmd/plugin.go +++ b/pkg/cmd/plugin.go @@ -48,7 +48,6 @@ func newPluginCmd(out io.Writer) *cobra.Command { func runHook(p plugin.Plugin, event string) error { pluginHook, ok := p.(plugin.PluginHook) if ok { - plugin.SetupPluginEnv(settings, p.Metadata().Name, p.Dir()) return pluginHook.InvokeHook(event) } diff --git a/pkg/cmd/plugin_test.go b/pkg/cmd/plugin_test.go index 738a64740..f7a418569 100644 --- a/pkg/cmd/plugin_test.go +++ b/pkg/cmd/plugin_test.go @@ -17,6 +17,7 @@ package cmd import ( "bytes" + "fmt" "os" "runtime" "strings" @@ -93,14 +94,14 @@ func TestLoadCLIPlugins(t *testing.T) { ) loadCLIPlugins(&cmd, &out) - envs := strings.Join([]string{ - "fullenv", - "testdata/helmhome/helm/plugins/fullenv", - "testdata/helmhome/helm/plugins", - "testdata/helmhome/helm/repositories.yaml", - "testdata/helmhome/helm/repository", - os.Args[0], - }, "\n") + fullEnvOutput := strings.Join([]string{ + "HELM_PLUGIN_NAME=fullenv", + "HELM_PLUGIN_DIR=testdata/helmhome/helm/plugins/fullenv", + "HELM_PLUGINS=testdata/helmhome/helm/plugins", + "HELM_REPOSITORY_CONFIG=testdata/helmhome/helm/repositories.yaml", + "HELM_REPOSITORY_CACHE=testdata/helmhome/helm/repository", + fmt.Sprintf("HELM_BIN=%s", os.Args[0]), + }, "\n") + "\n" // Test that the YAML file was correctly converted to a command. tests := []struct { @@ -113,47 +114,50 @@ func TestLoadCLIPlugins(t *testing.T) { }{ {"args", "echo args", "This echos args", "-a -b -c\n", []string{"-a", "-b", "-c"}, 0}, {"echo", "echo stuff", "This echos stuff", "hello\n", []string{}, 0}, - {"env", "env stuff", "show the env", "env\n", []string{}, 0}, + {"env", "env stuff", "show the env", "HELM_PLUGIN_NAME=env\n", []string{}, 0}, {"exitwith", "exitwith code", "This exits with the specified exit code", "", []string{"2"}, 2}, - {"fullenv", "show env vars", "show all env vars", envs + "\n", []string{}, 0}, + {"fullenv", "show env vars", "show all env vars", fullEnvOutput, []string{}, 0}, } - plugins := cmd.Commands() + pluginCmds := cmd.Commands() - require.Len(t, plugins, len(tests), "Expected %d plugins, got %d", len(tests), len(plugins)) + require.Len(t, pluginCmds, len(tests), "Expected %d plugins, got %d", len(tests), len(pluginCmds)) - for i := range plugins { + for i := range pluginCmds { out.Reset() tt := tests[i] - pp := plugins[i] - if pp.Use != tt.use { - t.Errorf("%d: Expected Use=%q, got %q", i, tt.use, pp.Use) - } - if pp.Short != tt.short { - t.Errorf("%d: Expected Use=%q, got %q", i, tt.short, pp.Short) - } - if pp.Long != tt.long { - t.Errorf("%d: Expected Use=%q, got %q", i, tt.long, pp.Long) - } + pluginCmd := pluginCmds[i] + t.Run(fmt.Sprintf("%s-%d", pluginCmd.Name(), i), func(t *testing.T) { + out.Reset() + if pluginCmd.Use != tt.use { + t.Errorf("%d: Expected Use=%q, got %q", i, tt.use, pluginCmd.Use) + } + if pluginCmd.Short != tt.short { + t.Errorf("%d: Expected Use=%q, got %q", i, tt.short, pluginCmd.Short) + } + if pluginCmd.Long != tt.long { + t.Errorf("%d: Expected Use=%q, got %q", i, tt.long, pluginCmd.Long) + } - // Currently, plugins assume a Linux subsystem. Skip the execution - // tests until this is fixed - if runtime.GOOS != "windows" { - if err := pp.RunE(pp, tt.args); err != nil { - if tt.code > 0 { - perr, ok := err.(PluginError) - if !ok { - t.Errorf("Expected %s to return pluginError: got %v(%T)", tt.use, err, err) + // Currently, plugins assume a Linux subsystem. Skip the execution + // tests until this is fixed + if runtime.GOOS != "windows" { + if err := pluginCmd.RunE(pluginCmd, tt.args); err != nil { + if tt.code > 0 { + cerr, ok := err.(CommandError) + if !ok { + t.Errorf("Expected %s to return pluginError: got %v(%T)", tt.use, err, err) + } + if cerr.ExitCode != tt.code { + t.Errorf("Expected %s to return %d: got %d", tt.use, tt.code, cerr.ExitCode) + } + } else { + t.Errorf("Error running %s: %+v", tt.use, err) } - if perr.Code != tt.code { - t.Errorf("Expected %s to return %d: got %d", tt.use, tt.code, perr.Code) - } - } else { - t.Errorf("Error running %s: %+v", tt.use, err) } + assert.Equal(t, tt.expect, out.String(), "expected output for %q", tt.use) } - assert.Equal(t, tt.expect, out.String(), "expected output for %s", tt.use) - } + }) } } @@ -214,12 +218,12 @@ func TestLoadPluginsWithSpace(t *testing.T) { if runtime.GOOS != "windows" { if err := pp.RunE(pp, tt.args); err != nil { if tt.code > 0 { - perr, ok := err.(PluginError) + cerr, ok := err.(CommandError) if !ok { t.Errorf("Expected %s to return pluginError: got %v(%T)", tt.use, err, err) } - if perr.Code != tt.code { - t.Errorf("Expected %s to return %d: got %d", tt.use, tt.code, perr.Code) + if cerr.ExitCode != tt.code { + t.Errorf("Expected %s to return %d: got %d", tt.use, tt.code, cerr.ExitCode) } } else { t.Errorf("Error running %s: %+v", tt.use, err) diff --git a/pkg/cmd/root.go b/pkg/cmd/root.go index 836df834d..2b2f7b750 100644 --- a/pkg/cmd/root.go +++ b/pkg/cmd/root.go @@ -460,3 +460,8 @@ func newRegistryClientWithTLS( } return registryClient, nil } + +type CommandError struct { + error + ExitCode int +} diff --git a/pkg/cmd/testdata/helmhome/helm/plugins/env/plugin-name.sh b/pkg/cmd/testdata/helmhome/helm/plugins/env/plugin-name.sh new file mode 100755 index 000000000..9e823ac13 --- /dev/null +++ b/pkg/cmd/testdata/helmhome/helm/plugins/env/plugin-name.sh @@ -0,0 +1,3 @@ +#!/usr/bin/env sh + +echo HELM_PLUGIN_NAME=${HELM_PLUGIN_NAME} diff --git a/pkg/cmd/testdata/helmhome/helm/plugins/env/plugin.yaml b/pkg/cmd/testdata/helmhome/helm/plugins/env/plugin.yaml index fa933af93..78a0a23fb 100644 --- a/pkg/cmd/testdata/helmhome/helm/plugins/env/plugin.yaml +++ b/pkg/cmd/testdata/helmhome/helm/plugins/env/plugin.yaml @@ -9,4 +9,4 @@ config: ignoreFlags: false runtimeConfig: platformCommand: - - command: "echo $HELM_PLUGIN_NAME" + - command: ${HELM_PLUGIN_DIR}/plugin-name.sh diff --git a/pkg/cmd/testdata/helmhome/helm/plugins/fullenv/fullenv.sh b/pkg/cmd/testdata/helmhome/helm/plugins/fullenv/fullenv.sh index 2efad9b3c..cc0c64a6a 100755 --- a/pkg/cmd/testdata/helmhome/helm/plugins/fullenv/fullenv.sh +++ b/pkg/cmd/testdata/helmhome/helm/plugins/fullenv/fullenv.sh @@ -1,7 +1,7 @@ #!/bin/sh -echo $HELM_PLUGIN_NAME -echo $HELM_PLUGIN_DIR -echo $HELM_PLUGINS -echo $HELM_REPOSITORY_CONFIG -echo $HELM_REPOSITORY_CACHE -echo $HELM_BIN +echo HELM_PLUGIN_NAME=${HELM_PLUGIN_NAME} +echo HELM_PLUGIN_DIR=${HELM_PLUGIN_DIR} +echo HELM_PLUGINS=${HELM_PLUGINS} +echo HELM_REPOSITORY_CONFIG=${HELM_REPOSITORY_CONFIG} +echo HELM_REPOSITORY_CACHE=${HELM_REPOSITORY_CACHE} +echo HELM_BIN=${HELM_BIN} diff --git a/pkg/postrenderer/postrenderer.go b/pkg/postrenderer/postrenderer.go index ed6699c32..55e6d3adf 100644 --- a/pkg/postrenderer/postrenderer.go +++ b/pkg/postrenderer/postrenderer.go @@ -65,7 +65,6 @@ func (r *postRendererPlugin) Run(renderedManifests *bytes.Buffer) (*bytes.Buffer Message: schema.InputMessagePostRendererV1{ ExtraArgs: r.args, Manifests: renderedManifests, - Settings: r.settings, }, } output, err := r.plugin.Invoke(context.Background(), input) diff --git a/pkg/postrenderer/postrenderer_test.go b/pkg/postrenderer/postrenderer_test.go index 9addd481d..824a1d179 100644 --- a/pkg/postrenderer/postrenderer_test.go +++ b/pkg/postrenderer/postrenderer_test.go @@ -18,14 +18,12 @@ package postrenderer import ( "bytes" - "path/filepath" "runtime" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "helm.sh/helm/v4/internal/plugin" "helm.sh/helm/v4/pkg/cli" ) @@ -38,8 +36,6 @@ func TestNewPostRenderPluginRunWithNoOutput(t *testing.T) { s := cli.New() s.PluginsDirectory = "testdata/plugins" name := "postrenderer-v1" - base := filepath.Join(s.PluginsDirectory, name) - plugin.SetupPluginEnv(s, name, base) renderer, err := NewPostRendererPlugin(s, name, "") require.NoError(t, err) @@ -57,8 +53,6 @@ func TestNewPostRenderPluginWithOneArgsRun(t *testing.T) { s := cli.New() s.PluginsDirectory = "testdata/plugins" name := "postrenderer-v1" - base := filepath.Join(s.PluginsDirectory, name) - plugin.SetupPluginEnv(s, name, base) renderer, err := NewPostRendererPlugin(s, name, "ARG1") require.NoError(t, err) @@ -77,8 +71,6 @@ func TestNewPostRenderPluginWithTwoArgsRun(t *testing.T) { s := cli.New() s.PluginsDirectory = "testdata/plugins" name := "postrenderer-v1" - base := filepath.Join(s.PluginsDirectory, name) - plugin.SetupPluginEnv(s, name, base) renderer, err := NewPostRendererPlugin(s, name, "ARG1", "ARG2") require.NoError(t, err)