fix: apply review comments

Signed-off-by: Kit Patella <[email protected]>

src/cmd/tools/helm/load_plugins.go

@@ -318,12 +318,12 @@ func loadFile(path string) (*pluginCommand, error) {
 	cmds := new(pluginCommand)
 	b, err := os.ReadFile(path)
 	if err != nil {
-		return &pluginCommand{}, fmt.Errorf("file (%s) not provided by plugin. No plugin auto-completion possible", path)
+		return nil, fmt.Errorf("file (%s) not provided by plugin. No plugin auto-completion possible", path)
 	}
 
 	err = yaml.Unmarshal(b, cmds)
 	if err != nil {
-		return &pluginCommand{}, err
+		return nil, err
 	}
 	return cmds, nil
 }

src/pkg/cluster/pvc.go

@@ -10,9 +10,8 @@ import (
 )
 
 // UpdateGiteaPVC updates the existing Gitea persistent volume claim and tells Gitea whether to create or not.
-// REVIEW(mkcp): It looks like we've got some error+signal coming back from these functions where we return a both a
-// string true/false downstream but sometimes with errors. So I'm not going to make these `return "", err` but we may
-// want to consider if returning the error is necessary and not, for example, better served with a new type.
+// TODO(mkcp): We return both string true/false and errors here so our callers get a string. This should be returning an
+// empty val if we error, but we'll have to refactor upstream beforehand.
 func (c *Cluster) UpdateGiteaPVC(ctx context.Context, pvcName string, shouldRollBack bool) (string, error) {
 	if shouldRollBack {
 		pvc, err := c.Clientset.CoreV1().PersistentVolumeClaims(ZarfNamespaceName).Get(ctx, pvcName, metav1.GetOptions{})

src/pkg/cluster/zarf.go

@@ -33,11 +33,11 @@ func (c *Cluster) GetDeployedZarfPackages(ctx context.Context) ([]types.Deployed
 	listOpts := metav1.ListOptions{LabelSelector: ZarfPackageInfoLabel}
 	secrets, err := c.Clientset.CoreV1().Secrets(ZarfNamespaceName).List(ctx, listOpts)
 	if err != nil {
-		return []types.DeployedPackage{}, err
+		return nil, err
 	}
 
-	errs := make([]error, 0)
-	deployedPackages := make([]types.DeployedPackage, 0)
+	errs := []error{}
+	deployedPackages := []types.DeployedPackage{}
 	for _, secret := range secrets.Items {
 		if !strings.HasPrefix(secret.Name, config.ZarfPackagePrefix) {
 			continue
@@ -52,9 +52,9 @@ func (c *Cluster) GetDeployedZarfPackages(ctx context.Context) ([]types.Deployed
 		deployedPackages = append(deployedPackages, deployedPackage)
 	}
 
-	// REVIEW(mkcp): This seems like it could be a breaking behavior change. Can I get a second set of eyes here?
-	if 0 < len(errs) {
-		return []types.DeployedPackage{}, errors.Join(errs...)
+	err = errors.Join(errs...)
+	if err != nil {
+		return nil, err
 	}
 	return deployedPackages, nil
 }

src/pkg/transform/artifact.go

@@ -87,16 +87,16 @@ func GenTransformURL(targetBaseURL string, sourceURL string) (*url.URL, error) {
 	// Rebuild the generic URL
 	transformedURL := fmt.Sprintf("%s/generic/%s/%s/%s", targetBaseURL, packageNameGlobal, version, fileName)
 
-	parsedUrl, err := url.Parse(transformedURL)
+	parsedURL, err := url.Parse(transformedURL)
 	if err != nil {
 		return &url.URL{}, err
 	}
 
 	// Drop the RawQuery and Fragment to avoid them being interpreted for generic packages
-	parsedUrl.RawQuery = ""
-	parsedUrl.Fragment = ""
+	parsedURL.RawQuery = ""
+	parsedURL.Fragment = ""
 
-	return parsedUrl, nil
+	return parsedURL, nil
 }
 
 // transformRegistryPath transforms a given request path using a new base URL and regex.