ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/retina
File: test/e2e/framework/kubernetes/get-external-crd.go (Go)

Code before:

import (
	"fmt"
	"io"
	"log"
	"net/http"
	"net/url"
	"os"
	"path"
	"path/filepath"
)

func downloadExternalCRDs(chartPath string) error {
	crdUrls := []string{
		"https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/main/example/prometheus-operator-crd/monitoring.coreos.com_servicemonitors.yaml",
	}

	for _, crdUrl := range crdUrls {
		crd, err := fetchYAML(crdUrl)
		if err == nil && crd != nil {
			crdName, err := extractFileName(crdUrl)
			if err == nil && crdName != "" {
				log.Printf("CRD exists %s", crdName)
				log.Printf("File path to be written to %s", filepath.Join(chartPath, "/crds/"+crdName))
				err = saveToFile(filepath.Join(chartPath, "/crds/"+crdName), crd)
				if err != nil {
					return fmt.Errorf("failed to save crd.yaml /crds dir: %w", err)
				}
			}
		}
		if err != nil {
			return fmt.Errorf("failed to fetch crd from %s: %w", crdUrl, err)
		}
	}
	return nil
}

func fetchYAML(url string) ([]byte, error) {
	resp, err := http.Get(url)
	if err != nil {
		return nil, fmt.Errorf("failed to get crd source code: %w", err)
	}
	defer resp.Body.Close()
	return io.ReadAll(resp.Body)
}

func extractFileName(rawURL string) (string, error) {
	parsedURL, err := url.Parse(rawURL)
	if err != nil {
		return "", fmt.Errorf("failed to parse url: %w", err)
	}

Review comment (refactor):
this block should be straight after `crdName, err := extractFileName(crdUrl)` - (relates to previous comments)

Code after:

import (
	"fmt"
	"io"
	"log"
	"net/http"
	"net/url"
	"os"
	"path"
	"path/filepath"
)

func downloadExternalCRDs(chartPath string) error {
	crdUrls := []string{
		"https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/main/example/prometheus-operator-crd/monitoring.coreos.com_servicemonitors.yaml",
	}

	for _, crdUrl := range crdUrls {
		crd, err := fetchYAML(crdUrl)
		if err != nil {
			return err
		}

		crdName, err := extractFileName(crdUrl)
		if err != nil {
			return err
		}

		log.Printf("CRD exists %s", crdName)
		log.Printf("File path to be written to %s", filepath.Join(chartPath, "/crds/"+crdName))
		err = saveToFile(filepath.Join(chartPath, "/crds/"+crdName), crd)
		if err != nil {
			return err
		}
	}
	return nil
}

func fetchYAML(url string) ([]byte, error) {
	resp, err := http.Get(url)
	if err != nil {
		return nil, fmt.Errorf("failed to get crd source code from %s: %w", url, err)
	}
	defer resp.Body.Close()
	return io.ReadAll(resp.Body)
}

func extractFileName(rawURL string) (string, error) {
	parsedURL, err := url.Parse(rawURL)
	if err != nil {
		return "", fmt.Errorf("failed to parse url: %w", err)
Repository: microsoft/retina
File: test/e2e/framework/kubernetes/get-external-crd.go (Go)

Code before:
package kubernetes

import (
	"fmt"
	"io"
	"log"
	"net/http"
	"net/url"
	"os"
	"path"
	"path/filepath"
)

func downloadExternalCRDs(chartPath string) error {
	crdUrls := []string{
		"https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/main/example/prometheus-operator-crd/monitoring.coreos.com_servicemonitors.yaml",
	}

	for _, crdUrl := range crdUrls {
		crd, err := fetchYAML(crdUrl)
		if err != nil {
			return err
		}
		if crd != nil {
			crdName, err := extractFileName(crdUrl)
			if err != nil {
				return err
			}
			if crdName != "" {
				log.Printf("CRD exists %s", crdName)
				log.Printf("File path to be written to %s", filepath.Join(chartPath, "/crds/"+crdName))
				err = saveToFile(filepath.Join(chartPath, "/crds/"+crdName), crd)
				if err != nil {
					return err
				}
			}
		}
	}
	return nil
}

func fetchYAML(url string) ([]byte, error) {
	resp, err := http.Get(url)
	if err != nil {
		return nil, fmt.Errorf("failed to get crd source code from %s: %w", url, err)
	}
	defer resp.Body.Close()
	return io.ReadAll(resp.Body)
}

Review comment (suggestion):
no need for this check as `fetchYAML` returns error or a byte slice, so if we get to this point we have a byte slice since the error would have been caught in the previous block.

Code after:
package kubernetes

import (
	"fmt"
	"io"
	"log"
	"net/http"
	"net/url"
	"os"
	"path"
	"path/filepath"
)

func downloadExternalCRDs(chartPath string) error {
	crdUrls := []string{
		"https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/main/example/prometheus-operator-crd/monitoring.coreos.com_servicemonitors.yaml",
	}

	for _, crdUrl := range crdUrls {
		crd, err := fetchYAML(crdUrl)
		if err != nil {
			return err
		}

		crdName, err := extractFileName(crdUrl)
		if err != nil {
			return err
		}

		log.Printf("CRD exists %s", crdName)
		log.Printf("File path to be written to %s", filepath.Join(chartPath, "/crds/"+crdName))
		err = saveToFile(filepath.Join(chartPath, "/crds/"+crdName), crd)
		if err != nil {
			return err
		}
	}
	return nil
}

func fetchYAML(url string) ([]byte, error) {
	resp, err := http.Get(url)
	if err != nil {
		return nil, fmt.Errorf("failed to get crd source code from %s: %w", url, err)
	}
	defer resp.Body.Close()
	return io.ReadAll(resp.Body)
}

func extractFileName(rawURL string) (string, error) {
Repository: microsoft/retina
File: pkg/capture/file/timestamp.go (Go)

Code before:
package file

import (
	"time"

	"github.com/pkg/errors"
)

type Timestamp struct {
	time.Time
}

const captureFileNameTimestampFormat string = "2006#01#02#15#04#05UTC"

func (timestamp *Timestamp) TimestampToString() string {
	return timestamp.Time.Format(captureFileNameTimestampFormat)
}

func NewTimestamp(timestamp string) (*Timestamp, error) {
	timestampStr, err := time.Parse(captureFileNameTimestampFormat, timestamp)
	if err != nil {
		return nil, errors.Wrap(err, "failed to create new timestamp")
	}
	return &Timestamp{timestampStr}, nil
}


Review comment (suggestion):
Sorry, I missed a detail in my earlier explanation. In Go, if you want to say "this is how this type should be represented as a string," you should implement [`fmt.Stringer`](https://pkg.go.dev/fmt#Stringer). That allows you to just pass this to basically anything in `fmt` like this:

```go
now := &Timestamp{time.Now().UTC()}
fmt.Println("The time is", now)
```

Instead of having to do this (and discover that the method exists):

```go
now := &Timestamp{time.Now().UTC()}
fmt.Println("the time is", now.TimestampToString())
```

So all you should need to do is this:
```suggestion
func (timestamp *Timestamp) String() string {
```

Code after:
package file

import (
	"time"

	"github.com/pkg/errors"
)

type Timestamp struct {
	time.Time
}

const captureFileNameTimestampFormat string = "20060102150405UTC"

func Now() Timestamp {
	return Timestamp{Time: time.Now().UTC().Truncate(time.Second)}
}

func (timestamp *Timestamp) String() string {
	return timestamp.Time.Format(captureFileNameTimestampFormat)
}

func StringToTimestamp(timestamp string) (*Timestamp, error) {
	parsedTime, err := time.Parse(captureFileNameTimestampFormat, timestamp)
	if err != nil {
		return nil, errors.Wrap(err, "failed to create timestamp from string")
	}
	return &Timestamp{parsedTime}, nil
}
Repository: microsoft/retina
File: pkg/capture/file/capture_filename.go (Go)

Code before:
package file

import (
	"fmt"
	"strings"
)

type CaptureFilename struct {
	CaptureName    string
	NodeHostname   string
	StartTimestamp *Timestamp
}

func (cf *CaptureFilename) GenerateCaptureFileName() string {
	formattedTime := strings.Replace(cf.StartTimestamp.TimestampToString(), "#", "", -1)
	uniqueName := fmt.Sprintf("%s-%s-%s", cf.CaptureName, cf.NodeHostname, formattedTime)
	return uniqueName
}


Review comment (suggestion):
This can also likely be a `fmt.Stringer` (see other comment)
```suggestion
func (cf *CaptureFilename) String() string {
```

Code after:
package file

import (
	"fmt"
)

type CaptureFilename struct {
	CaptureName    string
	NodeHostname   string
	StartTimestamp *Timestamp
}

func (cf *CaptureFilename) String() string {
	uniqueName := fmt.Sprintf("%s-%s-%s", cf.CaptureName, cf.NodeHostname, cf.StartTimestamp)
	return uniqueName
}
Repository: microsoft/retina
File: test/e2e/framework/kubernetes/check-pod-status.go (Go)

Code before:
	})

	err := wait.PollUntilContextCancel(ctx, RetryIntervalPodsReady, true, conditionFunc)
	if err != nil {
		PrintPodLogs(ctx, clientset, namespace, labelSelector)
		return fmt.Errorf("error waiting for pods in namespace \"%s\" with label \"%s\" to be in Running state: %w", namespace, labelSelector, err)
	}
	return nil
}

func CheckPodRestarts(ctx context.Context, clientset *kubernetes.Clientset, namespace, labelSelector string) error {
	var podList *corev1.PodList
	podList, err := clientset.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{LabelSelector: labelSelector})
	if err != nil {
		return fmt.Errorf("error listing Pods: %w", err)
	}

	for _, pod := range podList.Items {
		for istatus := range pod.Status.ContainerStatuses {
			status := &pod.Status.ContainerStatuses[istatus]
			if status.RestartCount > 0 {
				return fmt.Errorf("pod %s has %d restarts: status: %+v: %w", pod.Name, status.RestartCount, status, ErrPodCrashed)
			}
		}
	}
	log.Printf("no restarts detected for pods in namespace \"%s\" with label \"%s\"\n", namespace, labelSelector)
	return nil
}


Review comment (suggestion):
this could be pretty noisy with a lot of pods under that label, maybe omit and let caller log if error

Code after:
	})

	err := wait.PollUntilContextCancel(ctx, RetryIntervalPodsReady, true, conditionFunc)
	if err != nil {
		PrintPodLogs(ctx, clientset, namespace, labelSelector)
		return fmt.Errorf("error waiting for pods in namespace \"%s\" with label \"%s\" to be in Running state: %w", namespace, labelSelector, err)
	}
	return nil
}

func CheckContainerRestart(ctx context.Context, clientset *kubernetes.Clientset, namespace, labelSelector string) error {
	var podList *corev1.PodList
	podList, err := clientset.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{LabelSelector: labelSelector})
	if err != nil {
		return fmt.Errorf("error listing Pods: %w", err)
	}

	for _, pod := range podList.Items {
		for istatus := range pod.Status.ContainerStatuses {
			status := &pod.Status.ContainerStatuses[istatus]
			if status.RestartCount > 0 {
				return fmt.Errorf("pod %s has %d container restarts: status: %+v: %w", pod.Name, status.RestartCount, status, ErrPodCrashed)
			}
		}
	}
	return nil
}
Repository: microsoft/retina
File: test/e2e/framework/kubernetes/no-crashes.go (Go)

Code before:
package kubernetes

import (
	"context"
	"fmt"

	"k8s.io/client-go/kubernetes"
	"k8s.io/client-go/tools/clientcmd"
)

var ErrPodCrashed = fmt.Errorf("pod has crashes")

type EnsureStableCluster struct {
	LabelSelector      string
	PodNamespace       string
	KubeConfigFilePath string
	IgnoreRestart      bool
}

func (n *EnsureStableCluster) Run() error {
	config, err := clientcmd.BuildConfigFromFlags("", n.KubeConfigFilePath)
	if err != nil {
		return fmt.Errorf("error building kubeconfig: %w", err)
	}

	clientset, err := kubernetes.NewForConfig(config)
	if err != nil {
		return fmt.Errorf("error creating Kubernetes client: %w", err)
	}

	err = WaitForPodReady(context.TODO(), clientset, n.PodNamespace, n.LabelSelector)
	if err != nil {
		return fmt.Errorf("error waiting for retina pods to be ready: %w", err)
	}

	if !n.IgnoreRestart {
		err = CheckPodRestarts(context.TODO(), clientset, n.PodNamespace, n.LabelSelector)
		if err != nil {
			return fmt.Errorf("error checking pod restarts: %w", err)

Review comment (suggestion):
nit: The struct name is a little misleading. When you say cluster, assumption is everything in a cluster. What I think this actually is a `EnsureStableComponent`. You can use this struct to check specific components in your cluster - like `retina` or `npm`, etc.

Code after:
package kubernetes

import (
	"context"
	"fmt"

	"k8s.io/client-go/kubernetes"
	"k8s.io/client-go/tools/clientcmd"
)

var ErrPodCrashed = fmt.Errorf("pod has crashes")

type EnsureStableComponent struct {
	LabelSelector      string
	PodNamespace       string
	KubeConfigFilePath string

	// Container restarts can occur for various reason, they do not necessarily mean the entire cluster
	// is unstable or needs to be recreated. In some cases, container restarts are expected and acceptable.
	// This flag should be set to true only in those cases and provide additional why restart restarts are acceptable.
	IgnoreContainerRestart bool
}

func (n *EnsureStableComponent) Run() error {
	config, err := clientcmd.BuildConfigFromFlags("", n.KubeConfigFilePath)
	if err != nil {
		return fmt.Errorf("error building kubeconfig: %w", err)
	}

	clientset, err := kubernetes.NewForConfig(config)
	if err != nil {
		return fmt.Errorf("error creating Kubernetes client: %w", err)
	}

	err = WaitForPodReady(context.TODO(), clientset, n.PodNamespace, n.LabelSelector)
	if err != nil {
		return fmt.Errorf("error waiting for retina pods to be ready: %w", err)
	}
Repository: microsoft/retina
File: pkg/k8s/watcher_linux.go (Go)

Code before:
	return w, nil
}

func Start(ctx context.Context, k *watchers.K8sWatcher) {
	logger.Info("Starting Kubernetes watcher")

	option.Config.K8sSyncTimeout = 3 * time.Minute //nolint:gomnd // this duration is self-explanatory
	syncdCache := make(chan struct{})
	go k.InitK8sSubsystem(ctx, k8sResources, []string{}, syncdCache)
	logger.WithField("k8s resources", k8sResources).Info("Kubernetes watcher started, will wait for cache sync")

	// Wait for K8s watcher to sync. If doesn't complete in 3 minutes, causes fatal error.
	<-syncdCache
	logger.Info("Kubernetes watcher synced")
}

// retinaK8sErrorHandler is a custom error handler for the watcher
// that logs the error and tags the error to easily identify
func k8sWatcherErrorHandler(e error) {
	errStr := e.Error()
	switch {
	case strings.Contains(errStr, "Failed to watch *v1.Node"):
	case strings.Contains(errStr, "Failed to watch *v2.CiliumEndpoint"):
	case strings.Contains(errStr, "Failed to watch *v1.Service"):
	case strings.Contains(errStr, "Failed to watch *v2.CiliumNode"):
		logger.WithField("underlyingError", errStr).Error("Error watching k8s resource")
	default:
		k8s.K8sErrorHandler(e)
	}
}


Review comment (suggestion):
I dont think we should combine per resource errors together, I would highly suggest to keep them insividual because sometimes if there is an issue with one of the component, we should know which one has caused it. 

Code after:
	})
	return w, nil
}

func Start(ctx context.Context, k *watchers.K8sWatcher) {
	logger.Info("Starting Kubernetes watcher")

	option.Config.K8sSyncTimeout = 3 * time.Minute //nolint:gomnd // this duration is self-explanatory
	syncdCache := make(chan struct{})
	go k.InitK8sSubsystem(ctx, k8sResources, []string{}, syncdCache)
	logger.WithField("k8s resources", k8sResources).Info("Kubernetes watcher started, will wait for cache sync")

	// Wait for K8s watcher to sync. If doesn't complete in 3 minutes, causes fatal error.
	<-syncdCache
	logger.Info("Kubernetes watcher synced")
}

// retinaK8sErrorHandler is a custom error handler for the watcher
// that logs the error and tags the error to easily identify
func k8sWatcherErrorHandler(e error) {
	errStr := e.Error()
	logError := func(er, r string) {
		logger.WithFields(logrus.Fields{
			"underlyingError": er,
			"resource":        r,
		}).Error("Error watching k8s resource")
	}

	switch {
	case strings.Contains(errStr, "Failed to watch *v1.Node"):
		logError(errStr, "v1.Node")
	case strings.Contains(errStr, "Failed to watch *v2.CiliumEndpoint"):
		logError(errStr, "v2.CiliumEndpoint")
	case strings.Contains(errStr, "Failed to watch *v1.Service"):
		logError(errStr, "v1.Service")
	case strings.Contains(errStr, "Failed to watch *v2.CiliumNode"):
		logError(errStr, "v2.CiliumNode")
	default:
		k8s.K8sErrorHandler(e)
	}
}
Repository: microsoft/retina
File: test/e2e/framework/kubernetes/delete-namespace.go (Go)

Code before:
	"k8s.io/client-go/kubernetes"
	"k8s.io/client-go/tools/clientcmd"
	"k8s.io/client-go/util/retry"
)

const (
	deleteNamespaceTimeoutSeconds = 1200
)

type DeleteNamespace struct {
	Namespace          string
	KubeConfigFilePath string
}

func (d *DeleteNamespace) Run() error {
	config, err := clientcmd.BuildConfigFromFlags("", d.KubeConfigFilePath)
	if err != nil {
		return fmt.Errorf("error building kubeconfig: %w", err)
	}

	clientset, err := kubernetes.NewForConfig(config)
	if err != nil {
		return fmt.Errorf("error creating Kubernetes client: %w", err)
	}

	ctx, cancel := context.WithTimeout(context.Background(), deleteNamespaceTimeoutSeconds*time.Second)
	defer cancel()

	err = clientset.CoreV1().Namespaces().Delete(ctx, d.Namespace, metaV1.DeleteOptions{})
	if err != nil {
		if !errors.IsNotFound(err) {
			return fmt.Errorf("failed to delete namespace \"%s\": %w", d.Namespace, err)
		}
	}

	numberOfSteps := 9

	backoff := wait.Backoff{
		Steps:    numberOfSteps,
		Duration: 10 * time.Second,
		Factor:   2.0,
		// Jitter:   0.1,
	}

	// Check if namespace was deleted
	return retry.OnError(backoff,
		func(err error) bool {
			log.Printf("%v. Checking again soon...", err)

			return true
		},

Review comment (suggestion):
Honestly, just inline this. The const makes this less readable (mute the linter if it complains). Also, (even though we're not doing this) the best practice with Go durations is `const deleteNamespaceTimeout = 1200 * time.Second`. Embedding a unit like `Seconds` in the name is an antipattern.

```suggestion
	ctx, cancel := context.WithTimeout(context.Background(), 1200 * time.Second)
```

Code after:
	"k8s.io/client-go/kubernetes"
	"k8s.io/client-go/tools/clientcmd"
	"k8s.io/client-go/util/retry"
)

type DeleteNamespace struct {
	Namespace          string
	KubeConfigFilePath string
}

func (d *DeleteNamespace) Run() error {
	config, err := clientcmd.BuildConfigFromFlags("", d.KubeConfigFilePath)
	if err != nil {
		return fmt.Errorf("error building kubeconfig: %w", err)
	}

	clientset, err := kubernetes.NewForConfig(config)
	if err != nil {
		return fmt.Errorf("error creating Kubernetes client: %w", err)
	}

	ctx, cancel := context.WithTimeout(context.Background(), 1200*time.Second)
	defer cancel()

	err = clientset.CoreV1().Namespaces().Delete(ctx, d.Namespace, metaV1.DeleteOptions{})
	if err != nil {
		if !errors.IsNotFound(err) {
			return fmt.Errorf("failed to delete namespace \"%s\": %w", d.Namespace, err)
		}
	}

	backoff := wait.Backoff{
		Steps:    9,
		Duration: 10 * time.Second,
		Factor:   2.0,
		// Jitter:   0.1,
	}

	// Check if namespace was deleted
	return retry.OnError(backoff,
		func(err error) bool {
			log.Printf("%v. Checking again soon...", err)

			return true
		},
		func() error {
			_, err = clientset.CoreV1().Namespaces().Get(ctx, d.Namespace, metaV1.GetOptions{})
			if errors.IsNotFound(err) {
				return nil
			}
Repository: microsoft/retina
File: test/e2e/framework/kubernetes/delete-namespace.go (Go)

Code before:

func (d *DeleteNamespace) Run() error {
	config, err := clientcmd.BuildConfigFromFlags("", d.KubeConfigFilePath)
	if err != nil {
		return fmt.Errorf("error building kubeconfig: %w", err)
	}

	clientset, err := kubernetes.NewForConfig(config)
	if err != nil {
		return fmt.Errorf("error creating Kubernetes client: %w", err)
	}

	ctx, cancel := context.WithTimeout(context.Background(), deleteNamespaceTimeoutSeconds*time.Second)
	defer cancel()

	err = clientset.CoreV1().Namespaces().Delete(ctx, d.Namespace, metaV1.DeleteOptions{})
	if err != nil {
		if !errors.IsNotFound(err) {
			return fmt.Errorf("failed to delete namespace \"%s\": %w", d.Namespace, err)
		}
	}

	numberOfSteps := 9

	backoff := wait.Backoff{
		Steps:    numberOfSteps,
		Duration: 10 * time.Second,
		Factor:   2.0,
		// Jitter:   0.1,
	}

	// Check if namespace was deleted
	return retry.OnError(backoff,
		func(err error) bool {
			log.Printf("%v. Checking again soon...", err)

			return true
		},
		func() error {
			_, err = clientset.CoreV1().Namespaces().Get(ctx, d.Namespace, metaV1.GetOptions{})
			if errors.IsNotFound(err) {
				return nil
			}

			if err == nil {
				return fmt.Errorf("namespace \"%s\" still exists", d.Namespace)
			}

			return err
		},
	)

Review comment (suggestion):
Same thing here, the constant just obscures what's going on more than it helps.

```go
Steps: 9
```

Code after:
		return fmt.Errorf("error building kubeconfig: %w", err)
	}

	clientset, err := kubernetes.NewForConfig(config)
	if err != nil {
		return fmt.Errorf("error creating Kubernetes client: %w", err)
	}

	ctx, cancel := context.WithTimeout(context.Background(), 1200*time.Second)
	defer cancel()

	err = clientset.CoreV1().Namespaces().Delete(ctx, d.Namespace, metaV1.DeleteOptions{})
	if err != nil {
		if !errors.IsNotFound(err) {
			return fmt.Errorf("failed to delete namespace \"%s\": %w", d.Namespace, err)
		}
	}

	backoff := wait.Backoff{
		Steps:    9,
		Duration: 10 * time.Second,
		Factor:   2.0,
		// Jitter:   0.1,
	}

	// Check if namespace was deleted
	return retry.OnError(backoff,
		func(err error) bool {
			log.Printf("%v. Checking again soon...", err)

			return true
		},
		func() error {
			_, err = clientset.CoreV1().Namespaces().Get(ctx, d.Namespace, metaV1.GetOptions{})
			if errors.IsNotFound(err) {
				return nil
			}

			if err == nil {
				return fmt.Errorf("namespace \"%s\" still exists", d.Namespace)
			}

			return err
		},
	)
}

func (d *DeleteNamespace) Stop() error {
	return nil
}
Repository: microsoft/retina
File: test/e2e/framework/scaletest/add-shared-labels.go (Go)

Code before:
	}

	for _, resource := range resources.Items {
		err = patchLabel(ctx, clientset, a.Namespace, resource.Name, patchBytes)
		if err != nil {
			log.Printf("Error adding shared labels to pod %s: %s\n", resource.Name, err)
		}
	}

	return nil
}

// Require for background steps
func (a *AddSharedLabelsToAllPods) Stop() error {
	return nil
}

func patchLabel(ctx context.Context, clientset *kubernetes.Clientset, namespace, podName string, patchBytes []byte) error {
	log.Println("Labeling Pod", podName)
	_, err := clientset.CoreV1().Pods(namespace).Patch(ctx, podName,
		types.JSONPatchType,
		patchBytes,
		metav1.PatchOptions{},
	)
	if err != nil {
		return fmt.Errorf("error patching pod: %w", err)
	}

	return nil
}

func getSharedLabelsPatch(numLabels int) ([]byte, error) {
	patch := []patchStringValue{}
	for i := 0; i < numLabels; i++ {
		patch = append(patch, patchStringValue{
			Op:    "add",
			Path:  "/metadata/labels/shared-lab-" + fmt.Sprintf("%05d", i),
			Value: "val",
		})
	}
	b, err := json.Marshal(patch)
	if err != nil {
		return nil, fmt.Errorf("error marshalling patch: %w", err)
	}

	return b, nil
}

func contextToLabelAllPods() (context.Context, context.CancelFunc) {
	return context.WithTimeout(context.Background(), timeoutToLabelAllPodsMinutes*time.Minute)
}

Review comment (suggestion):
When I wrap errors, the word "error" is typically redundant (it's an error, so it will be logged as such). It's sufficient to just write what you were trying to do when the error occurred, as you've done:

```suggestion
		return fmt.Errorf("patching pod: %w", err)
```

Code after:
		if err != nil {
			log.Printf("Error adding shared labels to pod %s: %s\n", resource.Name, err)
		}
	}

	return nil
}

// Require for background steps
func (a *AddSharedLabelsToAllPods) Stop() error {
	return nil
}

func patchLabel(ctx context.Context, clientset *kubernetes.Clientset, namespace, podName string, patchBytes []byte) error {
	log.Println("Labeling Pod", podName)
	_, err := clientset.CoreV1().Pods(namespace).Patch(ctx, podName,
		types.JSONPatchType,
		patchBytes,
		metav1.PatchOptions{},
	)
	if err != nil {
		return fmt.Errorf("failed to patch pod: %w", err)
	}

	return nil
}

func getSharedLabelsPatch(numLabels int) ([]byte, error) {
	patch := []patchStringValue{}
	for i := 0; i < numLabels; i++ {
		patch = append(patch, patchStringValue{
			Op:    "add",
			Path:  "/metadata/labels/shared-lab-" + fmt.Sprintf("%05d", i),
			Value: "val",
		})
	}
	b, err := json.Marshal(patch)
	if err != nil {
		return nil, fmt.Errorf("error marshalling patch: %w", err)
	}

	return b, nil
}

func contextToLabelAllPods() (context.Context, context.CancelFunc) {
	return context.WithTimeout(context.Background(), 120*time.Minute)
}