From c04e18e45253f08d127a37c8328e7084e486c7cc Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Wed, 12 Nov 2025 21:23:01 +0100 Subject: [PATCH 1/9] fix: improve plugin name validation error messages Signed-off-by: Benoit Tigeot --- internal/plugin/metadata_legacy.go | 5 ++- internal/plugin/metadata_legacy_test.go | 55 +++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 internal/plugin/metadata_legacy_test.go diff --git a/internal/plugin/metadata_legacy.go b/internal/plugin/metadata_legacy.go index a7b245dc0..8f74d44cb 100644 --- a/internal/plugin/metadata_legacy.go +++ b/internal/plugin/metadata_legacy.go @@ -69,7 +69,10 @@ type MetadataLegacy struct { func (m *MetadataLegacy) Validate() error { if !validPluginName.MatchString(m.Name) { - return fmt.Errorf("invalid plugin name") + if m.Name == "" { + return fmt.Errorf("plugin name is empty or missing: ensure plugin.yaml contains 'name:' field (lowercase)") + } + return fmt.Errorf("invalid plugin name %q: plugin names can only contain ASCII characters a-z, A-Z, 0-9, _ and -", m.Name) } m.Usage = sanitizeString(m.Usage) diff --git a/internal/plugin/metadata_legacy_test.go b/internal/plugin/metadata_legacy_test.go new file mode 100644 index 000000000..caabcc12a --- /dev/null +++ b/internal/plugin/metadata_legacy_test.go @@ -0,0 +1,55 @@ +/* +Copyright The Helm Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package plugin + +import ( + "strings" + "testing" +) + +func TestMetadataLegacyValidate_EmptyName(t *testing.T) { + metadata := MetadataLegacy{ + Name: "", + Version: "1.0.0", + } + + err := metadata.Validate() + if err == nil { + t.Fatal("expected error for empty plugin name") + } + + expectedMsg := "plugin name is empty or missing" + if !strings.Contains(err.Error(), expectedMsg) { + t.Errorf("expected error to contain %q, got %q", expectedMsg, err.Error()) + } +} + +func TestMetadataLegacyValidate_InvalidName(t *testing.T) { + metadata := MetadataLegacy{ + Name: "invalid name", + Version: "1.0.0", + } + + err := metadata.Validate() + if err == nil { + t.Fatal("expected error for invalid plugin name") + } + + expectedMsg := "plugin names can only contain ASCII characters" + if !strings.Contains(err.Error(), expectedMsg) { + t.Errorf("expected error to contain %q, got %q", expectedMsg, err.Error()) + } +} From f4b139a82c4f5f3b38f5cb7b90de3c2e93d62dc3 Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Fri, 14 Nov 2025 15:17:44 +0100 Subject: [PATCH 2/9] Make validation error similar and explicit for both metadatas Signed-off-by: Benoit Tigeot --- internal/plugin/metadata_legacy.go | 8 ++++---- internal/plugin/metadata_v1.go | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/internal/plugin/metadata_legacy.go b/internal/plugin/metadata_legacy.go index 8f74d44cb..562a0287e 100644 --- a/internal/plugin/metadata_legacy.go +++ b/internal/plugin/metadata_legacy.go @@ -68,11 +68,11 @@ type MetadataLegacy struct { } func (m *MetadataLegacy) Validate() error { + if m.Name == "" { + return fmt.Errorf("missing plugin name") + } if !validPluginName.MatchString(m.Name) { - if m.Name == "" { - return fmt.Errorf("plugin name is empty or missing: ensure plugin.yaml contains 'name:' field (lowercase)") - } - return fmt.Errorf("invalid plugin name %q: plugin names can only contain ASCII characters a-z, A-Z, 0-9, _ and -", m.Name) + return fmt.Errorf("invalid plugin name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name) } m.Usage = sanitizeString(m.Usage) diff --git a/internal/plugin/metadata_v1.go b/internal/plugin/metadata_v1.go index 81dbc2e20..5e83bd352 100644 --- a/internal/plugin/metadata_v1.go +++ b/internal/plugin/metadata_v1.go @@ -47,8 +47,11 @@ type MetadataV1 struct { } func (m *MetadataV1) Validate() error { + if m.Name == "" { + return fmt.Errorf("missing plugin `name`") + } if !validPluginName.MatchString(m.Name) { - return fmt.Errorf("invalid plugin `name`") + return fmt.Errorf("invalid plugin `name` %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name) } if m.APIVersion != "v1" { From cf077ceb2758a63aa5f87410204d11c5f2ad3a7b Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Fri, 14 Nov 2025 15:33:05 +0100 Subject: [PATCH 3/9] fix: focus only on plugin name but give more info about what we get And improve test Follow Scott comment: > here I would have a list of valid and invalid names based on pattern, with a check and intended outcome for each one so that we comprehensively cover the rules. https://github.com/helm/helm/pull/31491/files#r2524820312 Signed-off-by: Benoit Tigeot --- internal/plugin/metadata_legacy.go | 3 -- internal/plugin/metadata_legacy_test.go | 63 ++++++++++++++----------- internal/plugin/metadata_v1.go | 5 +- 3 files changed, 37 insertions(+), 34 deletions(-) diff --git a/internal/plugin/metadata_legacy.go b/internal/plugin/metadata_legacy.go index 562a0287e..3cd1a50cd 100644 --- a/internal/plugin/metadata_legacy.go +++ b/internal/plugin/metadata_legacy.go @@ -68,9 +68,6 @@ type MetadataLegacy struct { } func (m *MetadataLegacy) Validate() error { - if m.Name == "" { - return fmt.Errorf("missing plugin name") - } if !validPluginName.MatchString(m.Name) { return fmt.Errorf("invalid plugin name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name) } diff --git a/internal/plugin/metadata_legacy_test.go b/internal/plugin/metadata_legacy_test.go index caabcc12a..1ee23739c 100644 --- a/internal/plugin/metadata_legacy_test.go +++ b/internal/plugin/metadata_legacy_test.go @@ -16,40 +16,49 @@ limitations under the License. package plugin import ( - "strings" "testing" ) -func TestMetadataLegacyValidate_EmptyName(t *testing.T) { - metadata := MetadataLegacy{ - Name: "", - Version: "1.0.0", - } - - err := metadata.Validate() - if err == nil { - t.Fatal("expected error for empty plugin name") - } +func TestMetadataLegacyValidate_PluginName(t *testing.T) { + tests := []struct { + name string + pluginName string + shouldPass bool + }{ + // Valid names + {"lowercase", "myplugin", true}, + {"uppercase", "MYPLUGIN", true}, + {"mixed case", "MyPlugin", true}, + {"with digits", "plugin123", true}, + {"with hyphen", "my-plugin", true}, + {"with underscore", "my_plugin", true}, + {"mixed chars", "my-awesome_plugin_123", true}, - expectedMsg := "plugin name is empty or missing" - if !strings.Contains(err.Error(), expectedMsg) { - t.Errorf("expected error to contain %q, got %q", expectedMsg, err.Error()) + // Invalid names + {"empty", "", false}, + {"space", "my plugin", false}, + {"colon", "Name:", false}, + {"period", "my.plugin", false}, + {"slash", "my/plugin", false}, + {"dollar", "$plugin", false}, + {"unicode", "plügîn", false}, } -} -func TestMetadataLegacyValidate_InvalidName(t *testing.T) { - metadata := MetadataLegacy{ - Name: "invalid name", - Version: "1.0.0", - } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + metadata := MetadataLegacy{ + Name: tt.pluginName, + Version: "1.0.0", + } - err := metadata.Validate() - if err == nil { - t.Fatal("expected error for invalid plugin name") - } + err := metadata.Validate() - expectedMsg := "plugin names can only contain ASCII characters" - if !strings.Contains(err.Error(), expectedMsg) { - t.Errorf("expected error to contain %q, got %q", expectedMsg, err.Error()) + if tt.shouldPass && err != nil { + t.Errorf("expected %q to be valid, got error: %v", tt.pluginName, err) + } + if !tt.shouldPass && err == nil { + t.Errorf("expected %q to be invalid, but passed", tt.pluginName) + } + }) } } diff --git a/internal/plugin/metadata_v1.go b/internal/plugin/metadata_v1.go index 5e83bd352..81dbc2e20 100644 --- a/internal/plugin/metadata_v1.go +++ b/internal/plugin/metadata_v1.go @@ -47,11 +47,8 @@ type MetadataV1 struct { } func (m *MetadataV1) Validate() error { - if m.Name == "" { - return fmt.Errorf("missing plugin `name`") - } if !validPluginName.MatchString(m.Name) { - return fmt.Errorf("invalid plugin `name` %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name) + return fmt.Errorf("invalid plugin `name`") } if m.APIVersion != "v1" { From 9e1e3d21c59820b39a05d950bf27027ba7ef1662 Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Fri, 14 Nov 2025 15:36:21 +0100 Subject: [PATCH 4/9] fix: Make invalid name error message more similar and move tests Follow Scott comment\ https://github.com/helm/helm/pull/31491/files#r2524933784 Signed-off-by: Benoit Tigeot --- internal/plugin/metadata.go | 2 +- internal/plugin/metadata_legacy.go | 2 +- internal/plugin/metadata_legacy_test.go | 19 ++----------- internal/plugin/metadata_test.go | 4 +-- internal/plugin/plugin_test.go | 38 +++++++++++++++++++++++++ 5 files changed, 44 insertions(+), 21 deletions(-) diff --git a/internal/plugin/metadata.go b/internal/plugin/metadata.go index 111c0599f..15a954037 100644 --- a/internal/plugin/metadata.go +++ b/internal/plugin/metadata.go @@ -54,7 +54,7 @@ func (m Metadata) Validate() error { var errs []error if !validPluginName.MatchString(m.Name) { - errs = append(errs, fmt.Errorf("invalid name")) + errs = append(errs, fmt.Errorf("invalid name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name)) } if m.APIVersion == "" { diff --git a/internal/plugin/metadata_legacy.go b/internal/plugin/metadata_legacy.go index 3cd1a50cd..1b80e6196 100644 --- a/internal/plugin/metadata_legacy.go +++ b/internal/plugin/metadata_legacy.go @@ -69,7 +69,7 @@ type MetadataLegacy struct { func (m *MetadataLegacy) Validate() error { if !validPluginName.MatchString(m.Name) { - return fmt.Errorf("invalid plugin name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name) + return fmt.Errorf("invalid name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name) } m.Usage = sanitizeString(m.Usage) diff --git a/internal/plugin/metadata_legacy_test.go b/internal/plugin/metadata_legacy_test.go index 1ee23739c..79d57211c 100644 --- a/internal/plugin/metadata_legacy_test.go +++ b/internal/plugin/metadata_legacy_test.go @@ -25,23 +25,8 @@ func TestMetadataLegacyValidate_PluginName(t *testing.T) { pluginName string shouldPass bool }{ - // Valid names - {"lowercase", "myplugin", true}, - {"uppercase", "MYPLUGIN", true}, - {"mixed case", "MyPlugin", true}, - {"with digits", "plugin123", true}, - {"with hyphen", "my-plugin", true}, - {"with underscore", "my_plugin", true}, - {"mixed chars", "my-awesome_plugin_123", true}, - - // Invalid names - {"empty", "", false}, - {"space", "my plugin", false}, - {"colon", "Name:", false}, - {"period", "my.plugin", false}, - {"slash", "my/plugin", false}, - {"dollar", "$plugin", false}, - {"unicode", "plügîn", false}, + {"valid name", "my-plugin", true}, + {"invalid with space", "my plugin", false}, } for _, tt := range tests { diff --git a/internal/plugin/metadata_test.go b/internal/plugin/metadata_test.go index 28bc4cf51..0c699b00f 100644 --- a/internal/plugin/metadata_test.go +++ b/internal/plugin/metadata_test.go @@ -66,8 +66,8 @@ func TestValidatePluginData(t *testing.T) { } else if !item.pass && err == nil { t.Errorf("expected case %d to fail", i) } - if !item.pass && err.Error() != item.errString { - t.Errorf("index [%d]: expected the following error: %s, but got: %s", i, item.errString, err.Error()) + if !item.pass && !strings.Contains(err.Error(), item.errString) { + t.Errorf("index [%d]: expected error to contain: %s, but got: %s", i, item.errString, err.Error()) } } } diff --git a/internal/plugin/plugin_test.go b/internal/plugin/plugin_test.go index b6c2245ff..d911942b9 100644 --- a/internal/plugin/plugin_test.go +++ b/internal/plugin/plugin_test.go @@ -21,6 +21,44 @@ import ( "helm.sh/helm/v4/internal/plugin/schema" ) +func TestValidPluginName(t *testing.T) { + tests := []struct { + name string + pluginName string + shouldPass bool + }{ + // Valid names + {"lowercase", "myplugin", true}, + {"uppercase", "MYPLUGIN", true}, + {"mixed case", "MyPlugin", true}, + {"with digits", "plugin123", true}, + {"with hyphen", "my-plugin", true}, + {"with underscore", "my_plugin", true}, + {"mixed chars", "my-awesome_plugin_123", true}, + + // Invalid names + {"empty", "", false}, + {"space", "my plugin", false}, + {"colon", "plugin:", false}, + {"period", "my.plugin", false}, + {"slash", "my/plugin", false}, + {"dollar", "$plugin", false}, + {"unicode", "plügîn", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + matches := validPluginName.MatchString(tt.pluginName) + if tt.shouldPass && !matches { + t.Errorf("expected %q to match validPluginName regex", tt.pluginName) + } + if !tt.shouldPass && matches { + t.Errorf("expected %q to not match validPluginName regex", tt.pluginName) + } + }) + } +} + func mockSubprocessCLIPlugin(t *testing.T, pluginName string) *SubprocessPluginRuntime { t.Helper() From acf331a0057ee79246f6058fca02313be29506fa Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Fri, 14 Nov 2025 16:10:22 +0100 Subject: [PATCH 5/9] fix: improve plugin name validation err messages early via unmarshalling - Add strict YAML unmarshalling for v1 plugins (d.KnownFields) - Add comprehensive test coverage for validPluginName regex - Maintain backwards compatibility for legacy plugins Signed-off-by: Benoit Tigeot --- internal/plugin/loader.go | 2 ++ internal/plugin/loader_test.go | 58 ++++++++++++++++++++++++++++++ internal/plugin/metadata.go | 2 +- internal/plugin/metadata_legacy.go | 2 +- internal/plugin/metadata_test.go | 10 +++--- 5 files changed, 67 insertions(+), 7 deletions(-) diff --git a/internal/plugin/loader.go b/internal/plugin/loader.go index a58a84126..2f051b99e 100644 --- a/internal/plugin/loader.go +++ b/internal/plugin/loader.go @@ -47,6 +47,7 @@ func loadMetadataLegacy(metadataData []byte) (*Metadata, error) { var ml MetadataLegacy d := yaml.NewDecoder(bytes.NewReader(metadataData)) + // NOTE: No strict unmarshalling for legacy plugins - maintain backwards compatibility if err := d.Decode(&ml); err != nil { return nil, err } @@ -66,6 +67,7 @@ func loadMetadataV1(metadataData []byte) (*Metadata, error) { var mv1 MetadataV1 d := yaml.NewDecoder(bytes.NewReader(metadataData)) + d.KnownFields(true) if err := d.Decode(&mv1); err != nil { return nil, err } diff --git a/internal/plugin/loader_test.go b/internal/plugin/loader_test.go index 47c214910..31fd8df78 100644 --- a/internal/plugin/loader_test.go +++ b/internal/plugin/loader_test.go @@ -268,3 +268,61 @@ func TestFindPlugins(t *testing.T) { }) } } + +func TestLoadMetadataLegacy_CapitalNameField(t *testing.T) { + yamlWithCapitalName := `Name: my-plugin +version: 1.0.0 +usage: test plugin +description: test description +command: echo test` + + _, err := loadMetadataLegacy([]byte(yamlWithCapitalName)) + + // Legacy plugins: No strict unmarshalling (backwards compatibility) + // YAML decoder silently ignores "Name:", then validation catches empty name + require.Error(t, err, "expected error for capital 'Name:' field") + assert.Contains(t, err.Error(), "invalid plugin name \"\": must contain only a-z, A-Z, 0-9, _ and -") + + t.Logf("Legacy error (validation catches empty name): %v", err) + t.Log("NOTE: V1 plugins use strict unmarshalling and would get: yaml: field Name not found") +} + +func TestLoadMetadataLegacy_CorrectField(t *testing.T) { + yamlWithCorrectName := `name: my-plugin +version: 1.0.0 +usage: test plugin +description: test description +command: echo test` + + m, err := loadMetadataLegacy([]byte(yamlWithCorrectName)) + + require.NoError(t, err, "expected success for correct 'name:' field") + assert.Equal(t, "my-plugin", m.Name) +} + +func TestLoadMetadataV1_CapitalNameField(t *testing.T) { + yamlWithCapitalName := `apiVersion: v1 +Name: my-plugin +type: cli/v1 +runtime: subprocess +` + + _, err := loadMetadataV1([]byte(yamlWithCapitalName)) + + require.Error(t, err, "expected error for capital 'Name:' field") + assert.Contains(t, err.Error(), "field Name not found in type plugin.MetadataV1") + + t.Logf("V1 error (strict unmarshalling): %v", err) +} + +func TestLoadMetadataV1_CorrectField(t *testing.T) { + yamlWithCorrectName := `apiVersion: v1 +name: my-plugin +type: cli/v1 +runtime: subprocess +` + m, err := loadMetadataV1([]byte(yamlWithCorrectName)) + + require.NoError(t, err, "expected success for correct 'name:' field") + assert.Equal(t, "my-plugin", m.Name) +} diff --git a/internal/plugin/metadata.go b/internal/plugin/metadata.go index 15a954037..b051c1223 100644 --- a/internal/plugin/metadata.go +++ b/internal/plugin/metadata.go @@ -54,7 +54,7 @@ func (m Metadata) Validate() error { var errs []error if !validPluginName.MatchString(m.Name) { - errs = append(errs, fmt.Errorf("invalid name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name)) + errs = append(errs, fmt.Errorf("invalid plugin name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name)) } if m.APIVersion == "" { diff --git a/internal/plugin/metadata_legacy.go b/internal/plugin/metadata_legacy.go index 1b80e6196..3cd1a50cd 100644 --- a/internal/plugin/metadata_legacy.go +++ b/internal/plugin/metadata_legacy.go @@ -69,7 +69,7 @@ type MetadataLegacy struct { func (m *MetadataLegacy) Validate() error { if !validPluginName.MatchString(m.Name) { - return fmt.Errorf("invalid name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name) + return fmt.Errorf("invalid plugin name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name) } m.Usage = sanitizeString(m.Usage) diff --git a/internal/plugin/metadata_test.go b/internal/plugin/metadata_test.go index 0c699b00f..145ef5101 100644 --- a/internal/plugin/metadata_test.go +++ b/internal/plugin/metadata_test.go @@ -53,10 +53,10 @@ func TestValidatePluginData(t *testing.T) { }{ {true, mockSubprocessCLIPlugin(t, "abcdefghijklmnopqrstuvwxyz0123456789_-ABC"), ""}, {true, mockSubprocessCLIPlugin(t, "foo-bar-FOO-BAR_1234"), ""}, - {false, mockSubprocessCLIPlugin(t, "foo -bar"), "invalid name"}, - {false, mockSubprocessCLIPlugin(t, "$foo -bar"), "invalid name"}, // Test leading chars - {false, mockSubprocessCLIPlugin(t, "foo -bar "), "invalid name"}, // Test trailing chars - {false, mockSubprocessCLIPlugin(t, "foo\nbar"), "invalid name"}, // Test newline + {false, mockSubprocessCLIPlugin(t, "foo -bar"), "invalid plugin name"}, + {false, mockSubprocessCLIPlugin(t, "$foo -bar"), "invalid plugin name"}, // Test leading chars + {false, mockSubprocessCLIPlugin(t, "foo -bar "), "invalid plugin name"}, // Test trailing chars + {false, mockSubprocessCLIPlugin(t, "foo\nbar"), "invalid plugin name"}, // Test newline {true, mockNoCommand, ""}, // Test no command metadata works {true, mockLegacyCommand, ""}, // Test legacy command metadata works } { @@ -92,7 +92,7 @@ func TestMetadataValidateMultipleErrors(t *testing.T) { // Check that all expected errors are present in the joined error expectedErrors := []string{ - "invalid name", + "invalid plugin name", "empty APIVersion", "empty type field", "empty runtime field", From c81a09b89abcda9905e41376585c842870cafc2c Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Sun, 16 Nov 2025 11:35:25 +0100 Subject: [PATCH 6/9] test: refactor TestMetadataLegacyValidate to be more generic Signed-off-by: Benoit Tigeot --- internal/plugin/metadata_legacy_test.go | 123 +++++++++++++++++++----- 1 file changed, 100 insertions(+), 23 deletions(-) diff --git a/internal/plugin/metadata_legacy_test.go b/internal/plugin/metadata_legacy_test.go index 79d57211c..9421e98b5 100644 --- a/internal/plugin/metadata_legacy_test.go +++ b/internal/plugin/metadata_legacy_test.go @@ -17,33 +17,110 @@ package plugin import ( "testing" + + "github.com/stretchr/testify/assert" ) -func TestMetadataLegacyValidate_PluginName(t *testing.T) { - tests := []struct { - name string - pluginName string - shouldPass bool - }{ - {"valid name", "my-plugin", true}, - {"invalid with space", "my plugin", false}, +func TestMetadataLegacyValidate(t *testing.T) { + testsValid := map[string]MetadataLegacy{ + "valid metadata": { + Name: "myplugin", + }, + "valid with command": { + Name: "myplugin", + Command: "echo hello", + }, + "valid with platformCommand": { + Name: "myplugin", + PlatformCommand: []PlatformCommand{ + {OperatingSystem: "linux", Architecture: "amd64", Command: "echo hello"}, + }, + }, + "valid with hooks": { + Name: "myplugin", + Hooks: Hooks{ + "install": "echo install", + }, + }, + "valid with platformHooks": { + Name: "myplugin", + PlatformHooks: PlatformHooks{ + "install": []PlatformCommand{ + {OperatingSystem: "linux", Architecture: "amd64", Command: "echo install"}, + }, + }, + }, + "valid with downloaders": { + Name: "myplugin", + Downloaders: []Downloaders{ + { + Protocols: []string{"myproto"}, + Command: "echo download", + }, + }, + }, + } + + for testName, metadata := range testsValid { + t.Run(testName, func(t *testing.T) { + assert.NoError(t, metadata.Validate()) + }) + } + + testsInvalid := map[string]MetadataLegacy{ + "invalid name": { + Name: "my plugin", // further tested in TestValidPluginName + }, + "both command and platformCommand": { + Name: "myplugin", + Command: "echo hello", + PlatformCommand: []PlatformCommand{ + {OperatingSystem: "linux", Architecture: "amd64", Command: "echo hello"}, + }, + }, + "both hooks and platformHooks": { + Name: "myplugin", + Hooks: Hooks{ + "install": "echo install", + }, + PlatformHooks: PlatformHooks{ + "install": []PlatformCommand{ + {OperatingSystem: "linux", Architecture: "amd64", Command: "echo install"}, + }, + }, + }, + "downloader with empty command": { + Name: "myplugin", + Downloaders: []Downloaders{ + { + Protocols: []string{"myproto"}, + Command: "", + }, + }, + }, + "downloader with no protocols": { + Name: "myplugin", + Downloaders: []Downloaders{ + { + Protocols: []string{}, + Command: "echo download", + }, + }, + }, + "downloader with empty protocol": { + Name: "myplugin", + Downloaders: []Downloaders{ + { + Protocols: []string{""}, + Command: "echo download", + }, + }, + }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - metadata := MetadataLegacy{ - Name: tt.pluginName, - Version: "1.0.0", - } - - err := metadata.Validate() - - if tt.shouldPass && err != nil { - t.Errorf("expected %q to be valid, got error: %v", tt.pluginName, err) - } - if !tt.shouldPass && err == nil { - t.Errorf("expected %q to be invalid, but passed", tt.pluginName) - } + for testName, metadata := range testsInvalid { + t.Run(testName, func(t *testing.T) { + assert.Error(t, metadata.Validate()) }) } } From 9b242dd9ed656438eedaf93e67003231d23dc765 Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Sun, 16 Nov 2025 11:44:49 +0100 Subject: [PATCH 7/9] test: convert tests to table drive tests Signed-off-by: Benoit Tigeot --- internal/plugin/loader_test.go | 115 +++++++++++++++++++++------------ 1 file changed, 75 insertions(+), 40 deletions(-) diff --git a/internal/plugin/loader_test.go b/internal/plugin/loader_test.go index 31fd8df78..e84905248 100644 --- a/internal/plugin/loader_test.go +++ b/internal/plugin/loader_test.go @@ -269,60 +269,95 @@ func TestFindPlugins(t *testing.T) { } } -func TestLoadMetadataLegacy_CapitalNameField(t *testing.T) { - yamlWithCapitalName := `Name: my-plugin +func TestLoadMetadataLegacy(t *testing.T) { + testCases := map[string]struct { + yaml string + expectError bool + errorContains string + expectedName string + logNote string + }{ + "capital name field": { + yaml: `Name: my-plugin version: 1.0.0 usage: test plugin description: test description -command: echo test` - - _, err := loadMetadataLegacy([]byte(yamlWithCapitalName)) - - // Legacy plugins: No strict unmarshalling (backwards compatibility) - // YAML decoder silently ignores "Name:", then validation catches empty name - require.Error(t, err, "expected error for capital 'Name:' field") - assert.Contains(t, err.Error(), "invalid plugin name \"\": must contain only a-z, A-Z, 0-9, _ and -") - - t.Logf("Legacy error (validation catches empty name): %v", err) - t.Log("NOTE: V1 plugins use strict unmarshalling and would get: yaml: field Name not found") -} - -func TestLoadMetadataLegacy_CorrectField(t *testing.T) { - yamlWithCorrectName := `name: my-plugin +command: echo test`, + expectError: true, + errorContains: `invalid plugin name "": must contain only a-z, A-Z, 0-9, _ and -`, + // Legacy plugins: No strict unmarshalling (backwards compatibility) + // YAML decoder silently ignores "Name:", then validation catches empty name + logNote: "NOTE: V1 plugins use strict unmarshalling and would get: yaml: field Name not found", + }, + "correct name field": { + yaml: `name: my-plugin version: 1.0.0 usage: test plugin description: test description -command: echo test` - - m, err := loadMetadataLegacy([]byte(yamlWithCorrectName)) +command: echo test`, + expectError: false, + expectedName: "my-plugin", + }, + } - require.NoError(t, err, "expected success for correct 'name:' field") - assert.Equal(t, "my-plugin", m.Name) + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + m, err := loadMetadataLegacy([]byte(tc.yaml)) + + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.errorContains) + t.Logf("Legacy error (validation catches empty name): %v", err) + if tc.logNote != "" { + t.Log(tc.logNote) + } + } else { + require.NoError(t, err) + assert.Equal(t, tc.expectedName, m.Name) + } + }) + } } -func TestLoadMetadataV1_CapitalNameField(t *testing.T) { - yamlWithCapitalName := `apiVersion: v1 +func TestLoadMetadataV1(t *testing.T) { + testCases := map[string]struct { + yaml string + expectError bool + errorContains string + expectedName string + }{ + "capital name field": { + yaml: `apiVersion: v1 Name: my-plugin type: cli/v1 runtime: subprocess -` - - _, err := loadMetadataV1([]byte(yamlWithCapitalName)) - - require.Error(t, err, "expected error for capital 'Name:' field") - assert.Contains(t, err.Error(), "field Name not found in type plugin.MetadataV1") - - t.Logf("V1 error (strict unmarshalling): %v", err) -} - -func TestLoadMetadataV1_CorrectField(t *testing.T) { - yamlWithCorrectName := `apiVersion: v1 +`, + expectError: true, + errorContains: "field Name not found in type plugin.MetadataV1", + }, + "correct name field": { + yaml: `apiVersion: v1 name: my-plugin type: cli/v1 runtime: subprocess -` - m, err := loadMetadataV1([]byte(yamlWithCorrectName)) +`, + expectError: false, + expectedName: "my-plugin", + }, + } - require.NoError(t, err, "expected success for correct 'name:' field") - assert.Equal(t, "my-plugin", m.Name) + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + m, err := loadMetadataV1([]byte(tc.yaml)) + + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.errorContains) + t.Logf("V1 error (strict unmarshalling): %v", err) + } else { + require.NoError(t, err) + assert.Equal(t, tc.expectedName, m.Name) + } + }) + } } From b296cbef6c8ad43de4f921c680c8df77fdd1cc84 Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Sun, 16 Nov 2025 11:49:08 +0100 Subject: [PATCH 8/9] test: split tests between valid and invalid Signed-off-by: Benoit Tigeot --- internal/plugin/plugin_test.go | 58 +++++++++++++++++----------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/internal/plugin/plugin_test.go b/internal/plugin/plugin_test.go index d911942b9..d71cec2e9 100644 --- a/internal/plugin/plugin_test.go +++ b/internal/plugin/plugin_test.go @@ -22,38 +22,38 @@ import ( ) func TestValidPluginName(t *testing.T) { - tests := []struct { - name string - pluginName string - shouldPass bool - }{ - // Valid names - {"lowercase", "myplugin", true}, - {"uppercase", "MYPLUGIN", true}, - {"mixed case", "MyPlugin", true}, - {"with digits", "plugin123", true}, - {"with hyphen", "my-plugin", true}, - {"with underscore", "my_plugin", true}, - {"mixed chars", "my-awesome_plugin_123", true}, - - // Invalid names - {"empty", "", false}, - {"space", "my plugin", false}, - {"colon", "plugin:", false}, - {"period", "my.plugin", false}, - {"slash", "my/plugin", false}, - {"dollar", "$plugin", false}, - {"unicode", "plügîn", false}, + validNames := map[string]string{ + "lowercase": "myplugin", + "uppercase": "MYPLUGIN", + "mixed case": "MyPlugin", + "with digits": "plugin123", + "with hyphen": "my-plugin", + "with underscore": "my_plugin", + "mixed chars": "my-awesome_plugin_123", } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - matches := validPluginName.MatchString(tt.pluginName) - if tt.shouldPass && !matches { - t.Errorf("expected %q to match validPluginName regex", tt.pluginName) + for name, pluginName := range validNames { + t.Run("valid/"+name, func(t *testing.T) { + if !validPluginName.MatchString(pluginName) { + t.Errorf("expected %q to match validPluginName regex", pluginName) } - if !tt.shouldPass && matches { - t.Errorf("expected %q to not match validPluginName regex", tt.pluginName) + }) + } + + invalidNames := map[string]string{ + "empty": "", + "space": "my plugin", + "colon": "plugin:", + "period": "my.plugin", + "slash": "my/plugin", + "dollar": "$plugin", + "unicode": "plügîn", + } + + for name, pluginName := range invalidNames { + t.Run("invalid/"+name, func(t *testing.T) { + if validPluginName.MatchString(pluginName) { + t.Errorf("expected %q to not match validPluginName regex", pluginName) } }) } From 71591ee63e5351522cd833ec4980428a91f634ea Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Sun, 16 Nov 2025 12:08:30 +0100 Subject: [PATCH 9/9] style: linting Signed-off-by: Benoit Tigeot --- internal/plugin/plugin_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/plugin/plugin_test.go b/internal/plugin/plugin_test.go index d71cec2e9..ae0b343f3 100644 --- a/internal/plugin/plugin_test.go +++ b/internal/plugin/plugin_test.go @@ -23,13 +23,13 @@ import ( func TestValidPluginName(t *testing.T) { validNames := map[string]string{ - "lowercase": "myplugin", - "uppercase": "MYPLUGIN", - "mixed case": "MyPlugin", - "with digits": "plugin123", - "with hyphen": "my-plugin", + "lowercase": "myplugin", + "uppercase": "MYPLUGIN", + "mixed case": "MyPlugin", + "with digits": "plugin123", + "with hyphen": "my-plugin", "with underscore": "my_plugin", - "mixed chars": "my-awesome_plugin_123", + "mixed chars": "my-awesome_plugin_123", } for name, pluginName := range validNames {