Repository: helm/helm
File: internal/plugin/metadata_legacy.go (Go)

Code before:
	// 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.
	Command string `yaml:"command"`

	// IgnoreFlags ignores any flags passed in from Helm
	IgnoreFlags bool `yaml:"ignoreFlags"`

	// 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. Removed in subprocess/v1 plugins.
	Hooks Hooks `yaml:"hooks"`

	// Downloaders field is used if the plugin supply downloader mechanism
	// for special protocols.
	Downloaders []Downloaders `yaml:"downloaders"`
}

func (m *MetadataLegacy) Validate() error {
	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)
	}
	m.Usage = sanitizeString(m.Usage)

	if len(m.PlatformCommand) > 0 && len(m.Command) > 0 {
		return fmt.Errorf("both platformCommand and command are set")
	}

	if len(m.PlatformHooks) > 0 && len(m.Hooks) > 0 {
		return fmt.Errorf("both platformHooks and hooks are set")
	}

	// Validate downloader plugins
	for i, downloader := range m.Downloaders {
		if downloader.Command == "" {
			return fmt.Errorf("downloader %d has empty command", i)
		}
		if len(downloader.Protocols) == 0 {
			return fmt.Errorf("downloader %d has no protocols", i)
		}
		for j, protocol := range downloader.Protocols {
			if protocol == "" {
				return fmt.Errorf("downloader %d has empty protocol at index %d", i, j)
			}

Review comment (bug):
Two things about this check - this is the wrong way to address the problem of a missing or incorrect key in `plugin.yaml` (the original issue reported using `Name` instead of `name` key). We should solve this instead by unmarshalling strict. We already do that in config, but we missed doing that in the non-config metadata. This is a bug we should fix.

For context, we added more robust validate for `plugin.yaml` keys and values in `[plugin.MetadataV1.Validate]` but didn't add additional validation from what existed in Helm 3 to `[plugin.MetadataLegacy.Validate]` because the goal is backwards-compatibility for legacy plugins. And because of that we should NOT add any additional validation here.

That said, your PR is not adding additional validation failures, but just splitting the existing check into empty/wrong messages. I do think improving the error message even for the legacy plugins is fine. But we should improve this message if so:

1. no need to mention the case or anything required for the YAML keys: this is documented in the docs site and in the codebase. And once we fix structly unmarshalling, that will surface the issue clearly on it's own.
2. if we're going to give the user the validate rules, let's make the messages consistent in format as we have for empty required fields in `[plugin.MetadataV1.Validate]`. If we're going to give the user info about valid name syntax in the error message when it doesn't match `validPluginName`, we should also do that in the V1 error message and make the legacy plugin message match.

Code after:
	// 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.
	Command string `yaml:"command"`

	// IgnoreFlags ignores any flags passed in from Helm
	IgnoreFlags bool `yaml:"ignoreFlags"`

	// 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. Removed in subprocess/v1 plugins.
	Hooks Hooks `yaml:"hooks"`

	// Downloaders field is used if the plugin supply downloader mechanism
	// for special protocols.
	Downloaders []Downloaders `yaml:"downloaders"`
}

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)
	}
	m.Usage = sanitizeString(m.Usage)

	if len(m.PlatformCommand) > 0 && len(m.Command) > 0 {
		return fmt.Errorf("both platformCommand and command are set")
	}

	if len(m.PlatformHooks) > 0 && len(m.Hooks) > 0 {
		return fmt.Errorf("both platformHooks and hooks are set")
	}

	// Validate downloader plugins
	for i, downloader := range m.Downloaders {
		if downloader.Command == "" {
			return fmt.Errorf("downloader %d has empty command", i)
		}
		if len(downloader.Protocols) == 0 {
			return fmt.Errorf("downloader %d has no protocols", i)
		}
		for j, protocol := range downloader.Protocols {
			if protocol == "" {
				return fmt.Errorf("downloader %d has empty protocol at index %d", i, j)
			}
		}
	}