Merge pull request #31218 from gjenkins8/gjenkins/plugn-integration/rm_legacy_command

Remove legacy Command/Hooks from v1 Subprocess (#23)

internal/plugin/installer/local_installer_test.go

@@ -86,7 +86,7 @@ func TestLocalInstallerTarball(t *testing.T) {
 		Body string
 		Mode int64
 	}{
-		{"test-plugin/plugin.yaml", "name: test-plugin\nversion: 1.0.0\nusage: test\ndescription: test\ncommand: echo", 0644},
+		{"test-plugin/plugin.yaml", "name: test-plugin\napiVersion: v1\ntype: cli/v1\nruntime: subprocess\nversion: 1.0.0\nconfig:\n  shortHelp: test\n  longHelp: test\nruntimeConfig:\n  platformCommand:\n  - command: echo", 0644},
 		{"test-plugin/bin/test-plugin", "#!/bin/bash\necho test", 0755},
 	}
 

internal/plugin/loader_test.go

@@ -80,7 +80,7 @@ func TestLoadDir(t *testing.T) {
 				IgnoreFlags: true,
 			},
 			RuntimeConfig: &RuntimeConfigSubprocess{
-				PlatformCommands: []PlatformCommand{
+				PlatformCommand: []PlatformCommand{
 					{OperatingSystem: "linux", Architecture: "", Command: "sh", Args: []string{"-c", "${HELM_PLUGIN_DIR}/hello.sh"}},
 					{OperatingSystem: "windows", Architecture: "", Command: "pwsh", Args: []string{"-c", "${HELM_PLUGIN_DIR}/hello.ps1"}},
 				},
@@ -90,6 +90,7 @@ func TestLoadDir(t *testing.T) {
 						{OperatingSystem: "windows", Architecture: "", Command: "pwsh", Args: []string{"-c", "echo \"installing...\""}},
 					},
 				},
+				expandHookArgs: apiVersion == "legacy",
 			},
 		}
 	}
@@ -150,8 +151,8 @@ func TestLoadDirGetter(t *testing.T) {
 		RuntimeConfig: &RuntimeConfigSubprocess{
 			ProtocolCommands: []SubprocessProtocolCommand{
 				{
-					Protocols: []string{"myprotocol", "myprotocols"},
-					Command:   "echo getter",
+					Protocols:       []string{"myprotocol", "myprotocols"},
+					PlatformCommand: []PlatformCommand{{Command: "echo getter"}},
 				},
 			},
 		},
@@ -174,7 +175,7 @@ func TestPostRenderer(t *testing.T) {
 		Runtime:    "subprocess",
 		Config:     &ConfigPostrenderer{},
 		RuntimeConfig: &RuntimeConfigSubprocess{
-			PlatformCommands: []PlatformCommand{
+			PlatformCommand: []PlatformCommand{
 				{
 					Command: "${HELM_PLUGIN_DIR}/sed-test.sh",
 				},

internal/plugin/metadata.go

@@ -144,15 +144,32 @@ func buildLegacyRuntimeConfig(m MetadataLegacy) RuntimeConfig {
 		protocolCommands =
 			make([]SubprocessProtocolCommand, 0, len(m.Downloaders))
 		for _, d := range m.Downloaders {
-			protocolCommands = append(protocolCommands, SubprocessProtocolCommand(d))
+			protocolCommands = append(protocolCommands, SubprocessProtocolCommand{
+				Protocols:       d.Protocols,
+				PlatformCommand: []PlatformCommand{{Command: d.Command}},
+			})
+		}
+	}
+
+	platformCommand := m.PlatformCommand
+	if len(platformCommand) == 0 && len(m.Command) > 0 {
+		platformCommand = []PlatformCommand{{Command: m.Command}}
+	}
+
+	platformHooks := m.PlatformHooks
+	expandHookArgs := true
+	if len(platformHooks) == 0 && len(m.Hooks) > 0 {
+		platformHooks = make(PlatformHooks, len(m.Hooks))
+		for hookName, hookCommand := range m.Hooks {
+			platformHooks[hookName] = []PlatformCommand{{Command: "sh", Args: []string{"-c", hookCommand}}}
+			expandHookArgs = false
 		}
 	}
 	return &RuntimeConfigSubprocess{
-		PlatformCommands: m.PlatformCommands,
-		Command:          m.Command,
-		PlatformHooks:    m.PlatformHooks,
-		Hooks:            m.Hooks,
+		PlatformCommand:  platformCommand,
+		PlatformHooks:    platformHooks,
 		ProtocolCommands: protocolCommands,
+		expandHookArgs:   expandHookArgs,
 	}
 }
 

internal/plugin/metadata_legacy.go

@@ -45,8 +45,8 @@ type MetadataLegacy struct {
 	// Description is a long description shown in places like `helm help`
 	Description string `yaml:"description"`
 
-	// PlatformCommands is the plugin command, with a platform selector and support for args.
-	PlatformCommands []PlatformCommand `yaml:"platformCommand"`
+	// PlatformCommand is the plugin command, with a platform selector and support for args.
+	PlatformCommand []PlatformCommand `yaml:"platformCommand"`
 
 	// Command is the plugin command, as a single string.
 	// DEPRECATED: Use PlatformCommand instead. Removed in subprocess/v1 plugins.
@@ -73,7 +73,7 @@ func (m *MetadataLegacy) Validate() error {
 	}
 	m.Usage = sanitizeString(m.Usage)
 
-	if len(m.PlatformCommands) > 0 && len(m.Command) > 0 {
+	if len(m.PlatformCommand) > 0 && len(m.Command) > 0 {
 		return fmt.Errorf("both platformCommand and command are set")
 	}
 

internal/plugin/metadata_test.go

@@ -25,44 +25,25 @@ func TestValidatePluginData(t *testing.T) {
 	// A mock plugin with no commands
 	mockNoCommand := mockSubprocessCLIPlugin(t, "foo")
 	mockNoCommand.metadata.RuntimeConfig = &RuntimeConfigSubprocess{
-		PlatformCommands: []PlatformCommand{},
-		PlatformHooks:    map[string][]PlatformCommand{},
+		PlatformCommand: []PlatformCommand{},
+		PlatformHooks:   map[string][]PlatformCommand{},
 	}
 
 	// A mock plugin with legacy commands
 	mockLegacyCommand := mockSubprocessCLIPlugin(t, "foo")
 	mockLegacyCommand.metadata.RuntimeConfig = &RuntimeConfigSubprocess{
-		PlatformCommands: []PlatformCommand{},
-		Command:          "echo \"mock plugin\"",
-		PlatformHooks:    map[string][]PlatformCommand{},
-		Hooks: map[string]string{
-			Install: "echo installing...",
-		},
-	}
-
-	// A mock plugin with a command also set
-	mockWithCommand := mockSubprocessCLIPlugin(t, "foo")
-	mockWithCommand.metadata.RuntimeConfig = &RuntimeConfigSubprocess{
-		PlatformCommands: []PlatformCommand{
-			{OperatingSystem: "linux", Architecture: "", Command: "sh", Args: []string{"-c", "echo \"mock plugin\""}},
-		},
-		Command: "echo \"mock plugin\"",
-	}
-
-	// A mock plugin with a hooks also set
-	mockWithHooks := mockSubprocessCLIPlugin(t, "foo")
-	mockWithHooks.metadata.RuntimeConfig = &RuntimeConfigSubprocess{
-		PlatformCommands: []PlatformCommand{
-			{OperatingSystem: "linux", Architecture: "", Command: "sh", Args: []string{"-c", "echo \"mock plugin\""}},
+		PlatformCommand: []PlatformCommand{
+			{
+				Command: "echo \"mock plugin\"",
+			},
 		},
 		PlatformHooks: map[string][]PlatformCommand{
 			Install: {
-				{OperatingSystem: "linux", Architecture: "", Command: "sh", Args: []string{"-c", "echo \"installing...\""}},
+				PlatformCommand{
+					Command: "echo installing...",
+				},
 			},
 		},
-		Hooks: map[string]string{
-			Install: "echo installing...",
-		},
 	}
 
 	for i, item := range []struct {
@@ -78,8 +59,6 @@ func TestValidatePluginData(t *testing.T) {
 		{false, mockSubprocessCLIPlugin(t, "foo\nbar"), "invalid name"},  // Test newline
 		{true, mockNoCommand, ""},     // Test no command metadata works
 		{true, mockLegacyCommand, ""}, // Test legacy command metadata works
-		{false, mockWithCommand, "runtime config validation failed: both platformCommand and command are set"}, // Test platformCommand and command both set fails
-		{false, mockWithHooks, "runtime config validation failed: both platformHooks and hooks are set"},       // Test platformHooks and hooks both set fails
 	} {
 		err := item.plug.Metadata().Validate()
 		if item.pass && err != nil {

internal/plugin/plugin_test.go

@@ -23,7 +23,7 @@ func mockSubprocessCLIPlugin(t *testing.T, pluginName string) *SubprocessPluginR
 	t.Helper()
 
 	rc := RuntimeConfigSubprocess{
-		PlatformCommands: []PlatformCommand{
+		PlatformCommand: []PlatformCommand{
 			{OperatingSystem: "linux", Architecture: "", Command: "sh", Args: []string{"-c", "echo \"mock plugin\""}},
 			{OperatingSystem: "windows", Architecture: "", Command: "pwsh", Args: []string{"-c", "echo \"mock plugin\""}},
 		},

internal/plugin/runtime_subprocess.go

@@ -33,28 +33,25 @@ import (
 type SubprocessProtocolCommand struct {
 	// Protocols are the list of schemes from the charts URL.
 	Protocols []string `yaml:"protocols"`
-	// Command is the executable path with which the plugin performs
-	// the actual download for the corresponding Protocols
-	Command string `yaml:"command"`
+	// PlatformCommand is the platform based command which the plugin performs
+	// to download for the corresponding getter Protocols.
+	PlatformCommand []PlatformCommand `yaml:"platformCommand"`
 }
 
 // RuntimeConfigSubprocess implements RuntimeConfig for RuntimeSubprocess
 type RuntimeConfigSubprocess struct {
 	// PlatformCommand is a list containing a plugin command, with a platform selector and support for args.
-	PlatformCommands []PlatformCommand `yaml:"platformCommand"`
-	// Command is the plugin command, as a single string.
-	// DEPRECATED: Use PlatformCommand instead. Remove in Helm 4.
-	Command string `yaml:"command"`
+	PlatformCommand []PlatformCommand `yaml:"platformCommand"`
 	// PlatformHooks are commands that will run on plugin events, with a platform selector and support for args.
 	PlatformHooks PlatformHooks `yaml:"platformHooks"`
-	// Hooks are commands that will run on plugin events, as a single string.
-	// DEPRECATED: Use PlatformHooks instead. Remove in Helm 4.
-	Hooks Hooks `yaml:"hooks"`
-	// ProtocolCommands field is used if the plugin supply downloader mechanism
-	// for special protocols.
-	// (This is a compatibility hangover from the old plugin downloader mechanism, which was extended to support multiple
-	// protocols in a given plugin)
+	// ProtocolCommands allows the plugin to specify protocol specific commands
+	//
+	// Obsolete/deprecated: This is a compatibility hangover from the old plugin downloader mechanism, which was extended
+	// to support multiple protocols in a given plugin. The command supplied in PlatformCommand should implement protocol
+	// specific logic by inspecting the download URL
 	ProtocolCommands []SubprocessProtocolCommand `yaml:"protocolCommands,omitempty"`
+
+	expandHookArgs bool
 }
 
 var _ RuntimeConfig = (*RuntimeConfigSubprocess)(nil)
@@ -62,12 +59,6 @@ var _ RuntimeConfig = (*RuntimeConfigSubprocess)(nil)
 func (r *RuntimeConfigSubprocess) GetType() string { return "subprocess" }
 
 func (r *RuntimeConfigSubprocess) Validate() error {
-	if len(r.PlatformCommands) > 0 && len(r.Command) > 0 {
-		return fmt.Errorf("both platformCommand and command are set")
-	}
-	if len(r.PlatformHooks) > 0 && len(r.Hooks) > 0 {
-		return fmt.Errorf("both platformHooks and hooks are set")
-	}
 	return nil
 }
 
@@ -138,25 +129,13 @@ func (r *SubprocessPluginRuntime) InvokeWithEnv(main string, argv []string, env
 }
 
 func (r *SubprocessPluginRuntime) InvokeHook(event string) error {
-	// Get hook commands for the event
-	var cmds []PlatformCommand
-	expandArgs := true
+	cmds := r.RuntimeConfig.PlatformHooks[event]
 
-	cmds = r.RuntimeConfig.PlatformHooks[event]
-	if len(cmds) == 0 && len(r.RuntimeConfig.Hooks) > 0 {
-		cmd := r.RuntimeConfig.Hooks[event]
-		if len(cmd) > 0 {
-			cmds = []PlatformCommand{{Command: "sh", Args: []string{"-c", cmd}}}
-			expandArgs = false
-		}
-	}
-
-	// If no hook commands are defined, just return successfully
 	if len(cmds) == 0 {
 		return nil
 	}
 
-	main, argv, err := PrepareCommands(cmds, expandArgs, []string{})
+	main, argv, err := PrepareCommands(cmds, r.RuntimeConfig.expandHookArgs, []string{})
 	if err != nil {
 		return err
 	}
@@ -200,10 +179,7 @@ func (r *SubprocessPluginRuntime) runCLI(input *Input) (*Output, error) {
 
 	extraArgs := input.Message.(schema.InputMessageCLIV1).ExtraArgs
 
-	cmds := r.RuntimeConfig.PlatformCommands
-	if len(cmds) == 0 && len(r.RuntimeConfig.Command) > 0 {
-		cmds = []PlatformCommand{{Command: r.RuntimeConfig.Command}}
-	}
+	cmds := r.RuntimeConfig.PlatformCommand
 
 	command, args, err := PrepareCommands(cmds, true, extraArgs)
 	if err != nil {
@@ -232,11 +208,7 @@ func (r *SubprocessPluginRuntime) runPostrenderer(input *Input) (*Output, error)
 	// Setup plugin environment
 	SetupPluginEnv(settings, r.metadata.Name, r.pluginDir)
 
-	cmds := r.RuntimeConfig.PlatformCommands
-	if len(cmds) == 0 && len(r.RuntimeConfig.Command) > 0 {
-		cmds = []PlatformCommand{{Command: r.RuntimeConfig.Command}}
-	}
-
+	cmds := r.RuntimeConfig.PlatformCommand
 	command, args, err := PrepareCommands(cmds, true, extraArgs)
 	if err != nil {
 		return nil, fmt.Errorf("failed to prepare plugin command: %w", err)

internal/plugin/runtime_subprocess_getter.go

@@ -22,7 +22,6 @@ import (
 	"os/exec"
 	"path/filepath"
 	"slices"
-	"strings"
 
 	"helm.sh/helm/v4/internal/plugin/schema"
 )
@@ -55,9 +54,12 @@ func (r *SubprocessPluginRuntime) runGetter(input *Input) (*Output, error) {
 		return nil, fmt.Errorf("no downloader found for protocol %q", msg.Protocol)
 	}
 
-	commands := strings.Split(d.Command, " ")
-	args := append(
-		commands[1:],
+	command, args, err := PrepareCommands(d.PlatformCommand, false, []string{})
+	if err != nil {
+		return nil, fmt.Errorf("failed to prepare commands for protocol %q: %w", msg.Protocol, err)
+	}
+	args = append(
+		args,
 		msg.Options.CertFile,
 		msg.Options.KeyFile,
 		msg.Options.CAFile,
@@ -73,7 +75,7 @@ func (r *SubprocessPluginRuntime) runGetter(input *Input) (*Output, error) {
 	// TODO should we pass along input.Stdout?
 	buf := bytes.Buffer{} // subprocess getters are expected to write content to stdout
 
-	pluginCommand := filepath.Join(r.pluginDir, commands[0])
+	pluginCommand := filepath.Join(r.pluginDir, command)
 	prog := exec.Command(
 		pluginCommand,
 		args...)

internal/plugin/subprocess_commands_test.go

@@ -27,14 +27,14 @@ func TestPrepareCommand(t *testing.T) {
 	cmdMain := "sh"
 	cmdArgs := []string{"-c", "echo \"test\""}
 
-	platformCommands := []PlatformCommand{
+	platformCommand := []PlatformCommand{
 		{OperatingSystem: "no-os", Architecture: "no-arch", Command: "pwsh", Args: []string{"-c", "echo \"error\""}},
 		{OperatingSystem: runtime.GOOS, Architecture: "no-arch", Command: "pwsh", Args: []string{"-c", "echo \"error\""}},
 		{OperatingSystem: runtime.GOOS, Architecture: "", Command: "pwsh", Args: []string{"-c", "echo \"error\""}},
 		{OperatingSystem: runtime.GOOS, Architecture: runtime.GOARCH, Command: cmdMain, Args: cmdArgs},
 	}
 
-	cmd, args, err := PrepareCommands(platformCommands, true, []string{})
+	cmd, args, err := PrepareCommands(platformCommand, true, []string{})
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -50,7 +50,7 @@ func TestPrepareCommandExtraArgs(t *testing.T) {
 
 	cmdMain := "sh"
 	cmdArgs := []string{"-c", "echo \"test\""}
-	platformCommands := []PlatformCommand{
+	platformCommand := []PlatformCommand{
 		{OperatingSystem: "no-os", Architecture: "no-arch", Command: "pwsh", Args: []string{"-c", "echo \"error\""}},
 		{OperatingSystem: runtime.GOOS, Architecture: runtime.GOARCH, Command: cmdMain, Args: cmdArgs},
 		{OperatingSystem: runtime.GOOS, Architecture: "no-arch", Command: "pwsh", Args: []string{"-c", "echo \"error\""}},
@@ -91,7 +91,7 @@ func TestPrepareCommandExtraArgs(t *testing.T) {
 			if tc.ignoreFlags {
 				testExtraArgs = []string{}
 			}
-			cmd, args, err := PrepareCommands(platformCommands, true, testExtraArgs)
+			cmd, args, err := PrepareCommands(platformCommand, true, testExtraArgs)
 			if err != nil {
 				t.Fatal(err)
 			}

internal/plugin/testdata/plugdir/bad/duplicate-entries-v1/plugin.yaml

@@ -9,8 +9,11 @@ config:
     description
   ignoreFlags: true
 runtimeConfig:
-  command: "echo hello"
-  hooks:
-    install: "echo installing..."
-  hooks:
-    install: "echo installing something different"
+  platformCommand:
+  - command: "echo hello"
+  platformHooks:
+    install:
+    - command: "echo installing..."
+  platformHooks:
+    install:
+    - command: "echo installing something different"

internal/plugin/testdata/plugdir/good/getter/plugin.yaml

@@ -10,7 +10,8 @@ config:
     - "myprotocols"
 runtimeConfig:
   protocolCommands:
-    - command: "echo getter"
+    - platformCommand:
+      - command: "echo getter"
       protocols:
         - "myprotocol"
         - "myprotocols"

pkg/cmd/plugin_package_test.go

@@ -34,7 +34,7 @@ config:
   shortHelp: A test plugin
   longHelp: A test plugin for testing purposes
 runtimeConfig:
-  platformCommands:
+  platformCommand:
     - os: linux
       command: echo
       args: ["test"]`

pkg/cmd/plugin_test.go

@@ -122,7 +122,7 @@ func TestLoadCLIPlugins(t *testing.T) {
 
 	require.Len(t, plugins, len(tests), "Expected %d plugins, got %d", len(tests), len(plugins))
 
-	for i := 0; i < len(plugins); i++ {
+	for i := range plugins {
 		out.Reset()
 		tt := tests[i]
 		pp := plugins[i]

pkg/cmd/testdata/helm home with space/helm/plugins/fullenv/plugin.yaml

@@ -1,10 +1,12 @@
+---
+apiVersion: v1
 name: fullenv
 type: cli/v1
-apiVersion: v1
 runtime: subprocess
 config:
   shortHelp: "show env vars"
   longHelp: "show all env vars"
   ignoreFlags: false
 runtimeConfig:
-  command: "$HELM_PLUGIN_DIR/fullenv.sh"
+  platformCommand:
+  - command: "$HELM_PLUGIN_DIR/fullenv.sh"

pkg/cmd/testdata/helmhome/helm/plugins/args/plugin.yaml

@@ -7,4 +7,5 @@ config:
   longHelp: "This echos args"
   ignoreFlags: false
 runtimeConfig:
-  command: "$HELM_PLUGIN_DIR/args.sh"
+  platformCommand:
+  - command: "$HELM_PLUGIN_DIR/args.sh"

pkg/cmd/testdata/helmhome/helm/plugins/echo/plugin.yaml

@@ -7,4 +7,5 @@ config:
   longHelp: "This echos stuff"
   ignoreFlags: false
 runtimeConfig:
-  command: "echo hello"
+  platformCommand:
+  - command: "echo hello"

pkg/cmd/testdata/helmhome/helm/plugins/env/plugin.yaml

@@ -1,10 +1,12 @@
+---
+apiVersion: v1
 name: env
 type: cli/v1
-apiVersion: v1
 runtime: subprocess
 config:
   shortHelp: "env stuff"
   longHelp: "show the env"
   ignoreFlags: false
 runtimeConfig:
-  command: "echo $HELM_PLUGIN_NAME"
+  platformCommand:
+  - command: "echo $HELM_PLUGIN_NAME"

pkg/cmd/testdata/helmhome/helm/plugins/exitwith/plugin.yaml

@@ -1,10 +1,12 @@
+---
+apiVersion: v1
 name: exitwith
 type: cli/v1
-apiVersion: v1
 runtime: subprocess
 config:
   shortHelp: "exitwith code"
   longHelp: "This exits with the specified exit code"
   ignoreFlags: false
 runtimeConfig:
-  command: "$HELM_PLUGIN_DIR/exitwith.sh"
+  platformCommand:
+  - command: "$HELM_PLUGIN_DIR/exitwith.sh"

pkg/cmd/testdata/helmhome/helm/plugins/fullenv/plugin.yaml

@@ -1,10 +1,12 @@
+---
+apiVersion: v1
 name: fullenv
 type: cli/v1
-apiVersion: v1
 runtime: subprocess
 config:
   shortHelp: "show env vars"
   longHelp: "show all env vars"
   ignoreFlags: false
 runtimeConfig:
-  command: "$HELM_PLUGIN_DIR/fullenv.sh"
+  platformCommand:
+  - command: "$HELM_PLUGIN_DIR/fullenv.sh"

pkg/cmd/testdata/helmhome/helm/plugins/postrenderer-v1/plugin.yaml

@@ -1,8 +1,13 @@
+---
+apiVersion: v1
 name: "postrenderer-v1"
 version: "1.2.3"
 type: postrenderer/v1
-apiVersion: v1
 runtime: subprocess
+config:
+  shortHelp: "echo test"
+  longHelp: "This echos test"
+  ignoreFlags: false
 runtimeConfig:
   platformCommand:
-    - command: "${HELM_PLUGIN_DIR}/sed-test.sh"
+  - command: "${HELM_PLUGIN_DIR}/sed-test.sh"

pkg/cmd/testdata/testplugin/plugin.yaml

@@ -0,0 +1,12 @@
+---
+apiVersion: v1
+name: testplugin
+type: cli/v1
+runtime: subprocess
+config:
+  shortHelp: "echo test"
+  longHelp: "This echos test"
+  ignoreFlags: false
+runtimeConfig:
+  platformCommand:
+  - command: "echo test"

pkg/getter/plugingetter_test.go

@@ -112,7 +112,7 @@ func (t *testPlugin) Metadata() plugin.Metadata {
 		Runtime:    "subprocess",
 		Config:     &plugin.ConfigCLI{},
 		RuntimeConfig: &plugin.RuntimeConfigSubprocess{
-			PlatformCommands: []plugin.PlatformCommand{
+			PlatformCommand: []plugin.PlatformCommand{
 				{
 					Command: "echo fake-plugin",
 				},

pkg/postrenderer/testdata/plugins/postrenderer-v1/plugin.yaml

@@ -5,4 +5,4 @@ apiVersion: v1
 runtime: subprocess
 runtimeConfig:
   platformCommand:
-    - command: "${HELM_PLUGIN_DIR}/sed-test.sh"
+  - command: "${HELM_PLUGIN_DIR}/sed-test.sh"