ronantakizawa/codereview-bench huggingface dataset:
Repository: helm/helm
File: internal/plugin/plugin_test.go (Go)

Code before:
/*
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 (
	"testing"

	"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 {

Review comment (suggestion):
I think it's better to have this kind of test closer to where `validPluginName` is defined.

Code after:
/*
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 (
	"testing"

	"helm.sh/helm/v4/internal/plugin/schema"
)

func TestValidPluginName(t *testing.T) {
	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 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)
			}
		})
	}

	invalidNames := map[string]string{
		"empty":   "",
		"space":   "my plugin",
		"colon":   "plugin:",
		"period":  "my.plugin",
		"slash":   "my/plugin",
		"dollar":  "$plugin",
Repository: helm/helm
File: internal/plugin/subprocess_commands_test.go (Go)

Code before:

	expectedArgs := []string{"-c", "echo \"testxtesty\""}

	env := parseEnv(os.Environ())
	env["TESTY"] = "testy"

	cmd, args, err := PrepareCommands(cmds, true, []string{}, env)
	if err != nil {
		t.Fatal(err)
	}
	if cmd != cmdMain {
		t.Fatalf("Expected %q, got %q", cmdMain, cmd)
	}
	if !reflect.DeepEqual(args, expectedArgs) {
		t.Fatalf("Expected %v, got %v", expectedArgs, args)
	}
}

func TestPrepareCommandsNoExpand(t *testing.T) {
	t.Setenv("TEST", "test")
	cmdMain := "sh"
	cmdArgs := []string{"-c", "echo \"${TEST}\""}
	cmds := []PlatformCommand{
		{OperatingSystem: "", Architecture: "", Command: cmdMain, Args: cmdArgs},
	}
	env := parseEnv(os.Environ())

	cmd, args, err := PrepareCommands(cmds, false, []string{}, env)
	if err != nil {
		t.Fatal(err)
	}
	if cmd != cmdMain {
		t.Fatalf("Expected %q, got %q", cmdMain, cmd)
	}
	if !reflect.DeepEqual(args, cmdArgs) {
		t.Fatalf("Expected %v, got %v", cmdArgs, args)
	}
}


Review comment (suggestion):
similar to above:

```suggestion
	env := map[string]string {
	    "TEST": "test",
	}
```

Code after:

	env := map[string]string{
		"TESTX": "testx",
		"TESTY": "testy",
	}

	cmd, args, err := PrepareCommands(cmds, true, []string{}, env)
	if err != nil {
		t.Fatal(err)
	}
	if cmd != cmdMain {
		t.Fatalf("Expected %q, got %q", cmdMain, cmd)
	}
	if !reflect.DeepEqual(args, expectedArgs) {
		t.Fatalf("Expected %v, got %v", expectedArgs, args)
	}
}

func TestPrepareCommandsNoExpand(t *testing.T) {
	cmdMain := "sh"
	cmdArgs := []string{"-c", "echo \"${TEST}\""}
	cmds := []PlatformCommand{
		{OperatingSystem: "", Architecture: "", Command: cmdMain, Args: cmdArgs},
	}

	env := map[string]string{
		"TEST": "test",
	}

	cmd, args, err := PrepareCommands(cmds, false, []string{}, env)
	if err != nil {
		t.Fatal(err)
	}
	if cmd != cmdMain {
		t.Fatalf("Expected %q, got %q", cmdMain, cmd)
	}
	if !reflect.DeepEqual(args, cmdArgs) {
		t.Fatalf("Expected %v, got %v", cmdArgs, args)
	}
}
Repository: helm/helm
File: pkg/kube/resource.go (Go)

Code before:

    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 kube // import "helm.sh/helm/v4/pkg/kube"

import "k8s.io/cli-runtime/pkg/resource"

// ResourceList provides convenience methods for comparing collections of Infos.
type ResourceList []*resource.Info

// Append adds an Info to the Result.
func (r *ResourceList) Append(val *resource.Info) {
	*r = append(*r, val)
}

// Visit implements resource.Visitor. The visitor stops if fn returns an error.
func (r ResourceList) Visit(fn resource.VisitorFunc) error {
	for _, i := range r {
		if i.Name == "" {
			continue
		}
		if err := fn(i, nil); err != nil {
			return err
		}
	}
	return nil
}

// Filter returns a new Result with Infos that satisfy the predicate fn.
func (r ResourceList) Filter(fn func(*resource.Info) bool) ResourceList {
	var result ResourceList
	for _, i := range r {
		if fn(i) {
			result.Append(i)
		}
	}
	return result
}

// Get returns the Info from the result that matches the name and kind.
func (r ResourceList) Get(info *resource.Info) *resource.Info {
	for _, i := range r {
		if isMatchingInfo(i, info) {
			return i

Review comment (suggestion):
This function is a generic visitor for any/all scenarios. It should not implement business logic like skipping objects missing a name

Code after:

    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 kube // import "helm.sh/helm/v4/pkg/kube"

import "k8s.io/cli-runtime/pkg/resource"

// ResourceList provides convenience methods for comparing collections of Infos.
type ResourceList []*resource.Info

// Append adds an Info to the Result.
func (r *ResourceList) Append(val *resource.Info) {
	*r = append(*r, val)
}

// Visit implements resource.Visitor. The visitor stops if fn returns an error.
func (r ResourceList) Visit(fn resource.VisitorFunc) error {
	for _, i := range r {
		if err := fn(i, nil); err != nil {
			return err
		}
	}
	return nil
}

// Filter returns a new Result with Infos that satisfy the predicate fn.
func (r ResourceList) Filter(fn func(*resource.Info) bool) ResourceList {
	var result ResourceList
	for _, i := range r {
		if fn(i) {
			result.Append(i)
		}
	}
	return result
}

// Get returns the Info from the result that matches the name and kind.
func (r ResourceList) Get(info *resource.Info) *resource.Info {
	for _, i := range r {
		if isMatchingInfo(i, info) {
			return i
		}
	}
	return nil
Repository: helm/helm
File: pkg/action/validate.go (Go)

Code before:

func mergeAnnotations(obj runtime.Object, annotations map[string]string) error {
	current, err := accessor.Annotations(obj)
	if err != nil {
		return err
	}
	return accessor.SetAnnotations(obj, mergeStrStrMaps(current, annotations))
}

// merge two maps, always taking the value on the right
func mergeStrStrMaps(current, desired map[string]string) map[string]string {
	result := make(map[string]string)
	maps.Copy(result, current)
	maps.Copy(result, desired)
	return result
}

func validateNameAndgenerateName(info *resource.Info) (bool, error) {
	accessor, _ := meta.Accessor(info.Object)
	if info.Name == "" && accessor.GetGenerateName() != "" {
		return true, nil
	} else if info.Name != "" && accessor.GetGenerateName() != "" {
		return true, fmt.Errorf("metadata.name and metadata.generateName cannot both be set")
	}

	return false, nil
}


Review comment (suggestion):
The return above make the else redundant here. Removing makes the logic easier to follow:

```suggestion
	accessor, _ := meta.Accessor(info.Object)
	if info.Name == "" && accessor.GetGenerateName() != "" {
		return true, nil
	}
	
	if info.Name != "" && accessor.GetGenerateName() != "" {
		return true, fmt.Errorf("metadata.name and metadata.generateName cannot both be set")
	}

	return false, nil
```

Code after:

func mergeAnnotations(obj runtime.Object, annotations map[string]string) error {
	current, err := accessor.Annotations(obj)
	if err != nil {
		return err
	}
	return accessor.SetAnnotations(obj, mergeStrStrMaps(current, annotations))
}

// merge two maps, always taking the value on the right
func mergeStrStrMaps(current, desired map[string]string) map[string]string {
	result := make(map[string]string)
	maps.Copy(result, current)
	maps.Copy(result, desired)
	return result
}

// validateNameAndGenerateName validates that an object only has either `Name` or `GenerateName` set (and not both)
// If `GenerateName` is set, true is returned
// If an invalid combination of `Name` and `GenerateName` are set, an error is returned
func validateNameAndGenerateName(info *resource.Info) (bool, error) {
	accessor, err := meta.Accessor(info.Object)
	if err != nil {
		return false, err
	}

	if info.Name == "" && accessor.GetGenerateName() != "" {
		return true, nil
	}

	if info.Name != "" && accessor.GetGenerateName() != "" {
		return true, fmt.Errorf("metadata.name and metadata.generateName cannot both be set")
	}

	return false, nil
}
Repository: helm/helm
File: pkg/action/validate.go (Go)

Code before:
	"k8s.io/apimachinery/pkg/runtime"
	"k8s.io/cli-runtime/pkg/resource"

	"helm.sh/helm/v4/pkg/kube"
)

var accessor = meta.NewAccessor()

const (
	appManagedByLabel              = "app.kubernetes.io/managed-by"
	appManagedByHelm               = "Helm"
	helmReleaseNameAnnotation      = "meta.helm.sh/release-name"
	helmReleaseNamespaceAnnotation = "meta.helm.sh/release-namespace"
)

// requireAdoption returns the subset of resources that already exist in the cluster.
func requireAdoption(resources kube.ResourceList) (kube.ResourceList, error) {
	var requireUpdate kube.ResourceList

	err := resources.Visit(func(info *resource.Info, err error) error {
		if err != nil {
			return err
		}

		skip, err := validateNameAndgenerateName(info)
		if skip {
			return err
		}

		helper := resource.NewHelper(info.Client, info.Mapping)
		_, err = helper.Get(info.Namespace, info.Name)
		if err != nil {
			if apierrors.IsNotFound(err) {
				return nil
			}
			return fmt.Errorf("could not get information about the resource %s: %w", resourceString(info), err)
		}

		requireUpdate.Append(info)
		return nil
	})

	return requireUpdate, err
}

func existingResourceConflict(resources kube.ResourceList, releaseName, releaseNamespace string) (kube.ResourceList, error) {
	var requireUpdate kube.ResourceList

	err := resources.Visit(func(info *resource.Info, err error) error {
		if err != nil {
			return err

Review comment (suggestion):
```suggestion
		isGenerateName, err := validateNameAndgenerateName(info)
		if isGenerateName {
```

and others below..

Code after:
	"k8s.io/apimachinery/pkg/runtime"
	"k8s.io/cli-runtime/pkg/resource"

	"helm.sh/helm/v4/pkg/kube"
)

var accessor = meta.NewAccessor()

const (
	appManagedByLabel              = "app.kubernetes.io/managed-by"
	appManagedByHelm               = "Helm"
	helmReleaseNameAnnotation      = "meta.helm.sh/release-name"
	helmReleaseNamespaceAnnotation = "meta.helm.sh/release-namespace"
)

// requireAdoption returns the subset of resources that already exist in the cluster.
func requireAdoption(resources kube.ResourceList) (kube.ResourceList, error) {
	var requireUpdate kube.ResourceList

	err := resources.Visit(func(info *resource.Info, err error) error {
		if err != nil {
			return err
		}

		isGenerateName, err := validateNameAndGenerateName(info)
		if isGenerateName || err != nil {
			return err
		}

		helper := resource.NewHelper(info.Client, info.Mapping)
		_, err = helper.Get(info.Namespace, info.Name)
		if err != nil {
			if apierrors.IsNotFound(err) {
				return nil
			}
			return fmt.Errorf("could not get information about the resource %s: %w", resourceString(info), err)
		}

		requireUpdate.Append(info)
		return nil
	})

	return requireUpdate, err
}

func existingResourceConflict(resources kube.ResourceList, releaseName, releaseNamespace string) (kube.ResourceList, error) {
	var requireUpdate kube.ResourceList

	err := resources.Visit(func(info *resource.Info, err error) error {
		if err != nil {
			return err
Repository: helm/helm
File: pkg/action/validate.go (Go)

Code before:

func mergeLabels(obj runtime.Object, labels map[string]string) error {
	current, err := accessor.Labels(obj)
	if err != nil {
		return err
	}
	return accessor.SetLabels(obj, mergeStrStrMaps(current, labels))
}

func mergeAnnotations(obj runtime.Object, annotations map[string]string) error {
	current, err := accessor.Annotations(obj)
	if err != nil {
		return err
	}
	return accessor.SetAnnotations(obj, mergeStrStrMaps(current, annotations))
}

// merge two maps, always taking the value on the right
func mergeStrStrMaps(current, desired map[string]string) map[string]string {
	result := make(map[string]string)
	maps.Copy(result, current)
	maps.Copy(result, desired)
	return result
}

func validateNameAndgenerateName(info *resource.Info) (bool, error) {
	accessor, _ := meta.Accessor(info.Object)
	if info.Name == "" && accessor.GetGenerateName() != "" {
		return true, nil
	} else if info.Name != "" && accessor.GetGenerateName() != "" {
		return true, fmt.Errorf("metadata.name and metadata.generateName cannot both be set")
	}

	return false, nil
}


Review comment (suggestion):
```suggestion
// validateNameAndgenerateName validates that an object only has either `Name` or `GenerateName` set (and not both)
// If `GenerateName` is set, true is returned
// If an invalid combination of `Name` and `GenerateName` are set, an error is returned
func validateNameAndgenerateName(info *resource.Info) (bool, error) {
```

Code after:

func mergeLabels(obj runtime.Object, labels map[string]string) error {
	current, err := accessor.Labels(obj)
	if err != nil {
		return err
	}
	return accessor.SetLabels(obj, mergeStrStrMaps(current, labels))
}

func mergeAnnotations(obj runtime.Object, annotations map[string]string) error {
	current, err := accessor.Annotations(obj)
	if err != nil {
		return err
	}
	return accessor.SetAnnotations(obj, mergeStrStrMaps(current, annotations))
}

// merge two maps, always taking the value on the right
func mergeStrStrMaps(current, desired map[string]string) map[string]string {
	result := make(map[string]string)
	maps.Copy(result, current)
	maps.Copy(result, desired)
	return result
}

// validateNameAndGenerateName validates that an object only has either `Name` or `GenerateName` set (and not both)
// If `GenerateName` is set, true is returned
// If an invalid combination of `Name` and `GenerateName` are set, an error is returned
func validateNameAndGenerateName(info *resource.Info) (bool, error) {
	accessor, err := meta.Accessor(info.Object)
	if err != nil {
		return false, err
	}

	if info.Name == "" && accessor.GetGenerateName() != "" {
		return true, nil
	}

	if info.Name != "" && accessor.GetGenerateName() != "" {
		return true, fmt.Errorf("metadata.name and metadata.generateName cannot both be set")
	}

	return false, nil
}
Repository: helm/helm
File: pkg/action/validate.go (Go)

Code before:
func mergeLabels(obj runtime.Object, labels map[string]string) error {
	current, err := accessor.Labels(obj)
	if err != nil {
		return err
	}
	return accessor.SetLabels(obj, mergeStrStrMaps(current, labels))
}

func mergeAnnotations(obj runtime.Object, annotations map[string]string) error {
	current, err := accessor.Annotations(obj)
	if err != nil {
		return err
	}
	return accessor.SetAnnotations(obj, mergeStrStrMaps(current, annotations))
}

// merge two maps, always taking the value on the right
func mergeStrStrMaps(current, desired map[string]string) map[string]string {
	result := make(map[string]string)
	maps.Copy(result, current)
	maps.Copy(result, desired)
	return result
}

func validateNameAndgenerateName(info *resource.Info) (bool, error) {
	accessor, _ := meta.Accessor(info.Object)
	if info.Name == "" && accessor.GetGenerateName() != "" {
		return true, nil
	} else if info.Name != "" && accessor.GetGenerateName() != "" {
		return true, fmt.Errorf("metadata.name and metadata.generateName cannot both be set")
	}

	return false, nil
}


Review comment (suggestion):
The error should be handled here I think? (any specific reason to not handle?)

```suggestion
	accessor, err := meta.Accessor(info.Object)
	if err != nil {
        ...
```

Code after:
func mergeLabels(obj runtime.Object, labels map[string]string) error {
	current, err := accessor.Labels(obj)
	if err != nil {
		return err
	}
	return accessor.SetLabels(obj, mergeStrStrMaps(current, labels))
}

func mergeAnnotations(obj runtime.Object, annotations map[string]string) error {
	current, err := accessor.Annotations(obj)
	if err != nil {
		return err
	}
	return accessor.SetAnnotations(obj, mergeStrStrMaps(current, annotations))
}

// merge two maps, always taking the value on the right
func mergeStrStrMaps(current, desired map[string]string) map[string]string {
	result := make(map[string]string)
	maps.Copy(result, current)
	maps.Copy(result, desired)
	return result
}

// validateNameAndGenerateName validates that an object only has either `Name` or `GenerateName` set (and not both)
// If `GenerateName` is set, true is returned
// If an invalid combination of `Name` and `GenerateName` are set, an error is returned
func validateNameAndGenerateName(info *resource.Info) (bool, error) {
	accessor, err := meta.Accessor(info.Object)
	if err != nil {
		return false, err
	}

	if info.Name == "" && accessor.GetGenerateName() != "" {
		return true, nil
	}

	if info.Name != "" && accessor.GetGenerateName() != "" {
		return true, fmt.Errorf("metadata.name and metadata.generateName cannot both be set")
	}

	return false, nil
}
Repository: helm/helm
File: pkg/action/validate.go (Go)

Code before:
	"k8s.io/apimachinery/pkg/runtime"
	"k8s.io/cli-runtime/pkg/resource"

	"helm.sh/helm/v4/pkg/kube"
)

var accessor = meta.NewAccessor()

const (
	appManagedByLabel              = "app.kubernetes.io/managed-by"
	appManagedByHelm               = "Helm"
	helmReleaseNameAnnotation      = "meta.helm.sh/release-name"
	helmReleaseNamespaceAnnotation = "meta.helm.sh/release-namespace"
)

// requireAdoption returns the subset of resources that already exist in the cluster.
func requireAdoption(resources kube.ResourceList) (kube.ResourceList, error) {
	var requireUpdate kube.ResourceList

	err := resources.Visit(func(info *resource.Info, err error) error {
		if err != nil {
			return err
		}

		isGenerateName, err := validateNameAndgenerateName(info)
		if isGenerateName {
			return err
		}

		helper := resource.NewHelper(info.Client, info.Mapping)
		_, err = helper.Get(info.Namespace, info.Name)
		if err != nil {
			if apierrors.IsNotFound(err) {
				return nil
			}
			return fmt.Errorf("could not get information about the resource %s: %w", resourceString(info), err)
		}

		requireUpdate.Append(info)
		return nil
	})

	return requireUpdate, err
}

func existingResourceConflict(resources kube.ResourceList, releaseName, releaseNamespace string) (kube.ResourceList, error) {
	var requireUpdate kube.ResourceList

	err := resources.Visit(func(info *resource.Info, err error) error {
		if err != nil {
			return err

Review comment (suggestion):
```suggestion
		if isGenerateName || err != nil {
```

Let's explicitly handle the error, rather than relying on `isGenerateName` being true

Code after:
	"k8s.io/apimachinery/pkg/runtime"
	"k8s.io/cli-runtime/pkg/resource"

	"helm.sh/helm/v4/pkg/kube"
)

var accessor = meta.NewAccessor()

const (
	appManagedByLabel              = "app.kubernetes.io/managed-by"
	appManagedByHelm               = "Helm"
	helmReleaseNameAnnotation      = "meta.helm.sh/release-name"
	helmReleaseNamespaceAnnotation = "meta.helm.sh/release-namespace"
)

// requireAdoption returns the subset of resources that already exist in the cluster.
func requireAdoption(resources kube.ResourceList) (kube.ResourceList, error) {
	var requireUpdate kube.ResourceList

	err := resources.Visit(func(info *resource.Info, err error) error {
		if err != nil {
			return err
		}

		isGenerateName, err := validateNameAndGenerateName(info)
		if isGenerateName || err != nil {
			return err
		}

		helper := resource.NewHelper(info.Client, info.Mapping)
		_, err = helper.Get(info.Namespace, info.Name)
		if err != nil {
			if apierrors.IsNotFound(err) {
				return nil
			}
			return fmt.Errorf("could not get information about the resource %s: %w", resourceString(info), err)
		}

		requireUpdate.Append(info)
		return nil
	})

	return requireUpdate, err
}

func existingResourceConflict(resources kube.ResourceList, releaseName, releaseNamespace string) (kube.ResourceList, error) {
	var requireUpdate kube.ResourceList

	err := resources.Visit(func(info *resource.Info, err error) error {
		if err != nil {
			return err