From 1a23f1c7d77d943f35506c41db9be91526bc5979 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Tue, 10 Sep 2024 17:04:33 -0700 Subject: [PATCH 1/2] chore: ensure we return zeroed value when returning errors Signed-off-by: Kit Patella --- src/cmd/tools/helm/load_plugins.go | 7 +++++-- src/pkg/cluster/pvc.go | 3 +++ src/pkg/cluster/tunnel.go | 7 ++++--- src/pkg/cluster/zarf.go | 17 ++++++++++++----- src/pkg/transform/artifact.go | 10 +++++----- src/pkg/utils/io.go | 9 ++++++--- src/pkg/utils/yaml.go | 6 +++--- src/pkg/zoci/pull.go | 5 ++++- 8 files changed, 42 insertions(+), 22 deletions(-) diff --git a/src/cmd/tools/helm/load_plugins.go b/src/cmd/tools/helm/load_plugins.go index 28ea155030..e1b6c0b6fd 100644 --- a/src/cmd/tools/helm/load_plugins.go +++ b/src/cmd/tools/helm/load_plugins.go @@ -318,11 +318,14 @@ func loadFile(path string) (*pluginCommand, error) { cmds := new(pluginCommand) b, err := os.ReadFile(path) if err != nil { - return cmds, fmt.Errorf("file (%s) not provided by plugin. No plugin auto-completion possible", path) + return &pluginCommand{}, fmt.Errorf("file (%s) not provided by plugin. No plugin auto-completion possible", path) } err = yaml.Unmarshal(b, cmds) - return cmds, err + if err != nil { + return &pluginCommand{}, err + } + return cmds, nil } // pluginDynamicComp call the plugin.complete script of the plugin (if available) diff --git a/src/pkg/cluster/pvc.go b/src/pkg/cluster/pvc.go index 21a0a45ecf..ea946b6a18 100644 --- a/src/pkg/cluster/pvc.go +++ b/src/pkg/cluster/pvc.go @@ -10,6 +10,9 @@ 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. 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{}) diff --git a/src/pkg/cluster/tunnel.go b/src/pkg/cluster/tunnel.go index 764ae9a6eb..9798fd3272 100644 --- a/src/pkg/cluster/tunnel.go +++ b/src/pkg/cluster/tunnel.go @@ -83,7 +83,6 @@ func (c *Cluster) ListConnections(ctx context.Context) (types.ConnectStrings, er // NewTargetTunnelInfo returns a new TunnelInfo object for the specified target. func (c *Cluster) NewTargetTunnelInfo(ctx context.Context, target string) (TunnelInfo, error) { - var err error zt := TunnelInfo{ Namespace: ZarfNamespaceName, ResourceType: SvcResource, @@ -102,9 +101,11 @@ func (c *Cluster) NewTargetTunnelInfo(ctx context.Context, target string) (Tunne zt.RemotePort = ZarfInjectorPort default: if target != "" { - if zt, err = c.checkForZarfConnectLabel(ctx, target); err != nil { + ztNew, err := c.checkForZarfConnectLabel(ctx, target) + if err != nil { return TunnelInfo{}, fmt.Errorf("problem looking for a zarf connect label in the cluster: %s", err.Error()) } + zt = ztNew } if zt.ResourceName == "" { return TunnelInfo{}, fmt.Errorf("missing resource name") @@ -113,7 +114,7 @@ func (c *Cluster) NewTargetTunnelInfo(ctx context.Context, target string) (Tunne return TunnelInfo{}, fmt.Errorf("missing remote port") } } - return zt, err + return zt, nil } // Connect will establish a tunnel to the specified target. diff --git a/src/pkg/cluster/zarf.go b/src/pkg/cluster/zarf.go index eebbd6d295..b5dd812514 100644 --- a/src/pkg/cluster/zarf.go +++ b/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 nil, err + return []types.DeployedPackage{}, err } - errs := []error{} - deployedPackages := []types.DeployedPackage{} + errs := make([]error, 0) + deployedPackages := make([]types.DeployedPackage, 0) for _, secret := range secrets.Items { if !strings.HasPrefix(secret.Name, config.ZarfPackagePrefix) { continue @@ -52,7 +52,11 @@ func (c *Cluster) GetDeployedZarfPackages(ctx context.Context) ([]types.Deployed deployedPackages = append(deployedPackages, deployedPackage) } - return deployedPackages, errors.Join(errs...) + // 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...) + } + return deployedPackages, nil } // GetDeployedPackage gets the metadata information about the package name provided (if it exists in the cluster). @@ -325,7 +329,10 @@ func (c *Cluster) UpdateInternalArtifactServerToken(ctx context.Context, oldGitS } return nil }) - return newToken, err + if err != nil { + return "", err + } + return newToken, nil } // UpdateInternalGitServerSecret updates the internal gitea server secrets with the new git server info diff --git a/src/pkg/transform/artifact.go b/src/pkg/transform/artifact.go index 0aed7a46ea..837c3c344d 100644 --- a/src/pkg/transform/artifact.go +++ b/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) - url, err := url.Parse(transformedURL) + parsedUrl, err := url.Parse(transformedURL) if err != nil { - return url, err + return &url.URL{}, err } // Drop the RawQuery and Fragment to avoid them being interpreted for generic packages - url.RawQuery = "" - url.Fragment = "" + parsedUrl.RawQuery = "" + parsedUrl.Fragment = "" - return url, err + return parsedUrl, nil } // transformRegistryPath transforms a given request path using a new base URL and regex. diff --git a/src/pkg/utils/io.go b/src/pkg/utils/io.go index 7edee56422..f4ec9b07ab 100755 --- a/src/pkg/utils/io.go +++ b/src/pkg/utils/io.go @@ -40,7 +40,10 @@ func GetFinalExecutablePath() (string, error) { // In case the binary is symlinked somewhere else, get the final destination linkedPath, err := filepath.EvalSymlinks(binaryPath) - return linkedPath, err + if err != nil { + return "", err + } + return linkedPath, nil } // GetFinalExecutableCommand returns the final path to the Zarf executable including and library prefixes and overrides. @@ -48,7 +51,7 @@ func GetFinalExecutableCommand() (string, error) { // In case the binary is symlinked somewhere else, get the final destination zarfCommand, err := GetFinalExecutablePath() if err != nil { - return zarfCommand, err + return "", err } if config.ActionsCommandZarfPrefix != "" { @@ -60,5 +63,5 @@ func GetFinalExecutableCommand() (string, error) { zarfCommand = "zarf" } - return zarfCommand, err + return zarfCommand, nil } diff --git a/src/pkg/utils/yaml.go b/src/pkg/utils/yaml.go index f3fdaa53b7..641c219977 100644 --- a/src/pkg/utils/yaml.go +++ b/src/pkg/utils/yaml.go @@ -192,12 +192,12 @@ func SplitYAML(yamlData []byte) ([]*unstructured.Unstructured, error) { var objs []*unstructured.Unstructured ymls, err := SplitYAMLToString(yamlData) if err != nil { - return nil, err + return []*unstructured.Unstructured{}, err } for _, yml := range ymls { u := &unstructured.Unstructured{} if err := k8syaml.Unmarshal([]byte(yml), u); err != nil { - return objs, fmt.Errorf("failed to unmarshal manifest: %w", err) + return []*unstructured.Unstructured{}, fmt.Errorf("failed to unmarshal manifest: %w", err) } objs = append(objs, u) } @@ -220,7 +220,7 @@ func SplitYAMLToString(yamlData []byte) ([]string, error) { if errors.Is(err, io.EOF) { break } - return objs, fmt.Errorf("failed to unmarshal manifest: %w", err) + return []string{}, fmt.Errorf("failed to unmarshal manifest: %w", err) } ext.Raw = bytes.TrimSpace(ext.Raw) if len(ext.Raw) == 0 || bytes.Equal(ext.Raw, []byte("null")) { diff --git a/src/pkg/zoci/pull.go b/src/pkg/zoci/pull.go index 44c16b5646..d8ba73775e 100644 --- a/src/pkg/zoci/pull.go +++ b/src/pkg/zoci/pull.go @@ -70,7 +70,10 @@ func (r *Remote) PullPackage(ctx context.Context, destinationDir string, concurr err = r.CopyToTarget(ctx, layersToPull, dst, copyOpts) doneSaving <- err <-doneSaving - return layersToPull, err + if err != nil { + return nil, err + } + return layersToPull, nil } // LayersFromRequestedComponents returns the descriptors for the given components from the root manifest. From 2d71bda6f88ce1bbfb34554c8d51a4465a18343a Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 11 Sep 2024 14:20:03 -0700 Subject: [PATCH 2/2] fix: apply review comments Signed-off-by: Kit Patella --- src/cmd/tools/helm/load_plugins.go | 4 ++-- src/pkg/cluster/pvc.go | 5 ++--- src/pkg/cluster/zarf.go | 12 ++++++------ src/pkg/transform/artifact.go | 8 ++++---- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/cmd/tools/helm/load_plugins.go b/src/cmd/tools/helm/load_plugins.go index e1b6c0b6fd..df8b7cad67 100644 --- a/src/cmd/tools/helm/load_plugins.go +++ b/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 } diff --git a/src/pkg/cluster/pvc.go b/src/pkg/cluster/pvc.go index ea946b6a18..6bef179623 100644 --- a/src/pkg/cluster/pvc.go +++ b/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{}) diff --git a/src/pkg/cluster/zarf.go b/src/pkg/cluster/zarf.go index b5dd812514..b38b55d783 100644 --- a/src/pkg/cluster/zarf.go +++ b/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 } diff --git a/src/pkg/transform/artifact.go b/src/pkg/transform/artifact.go index 837c3c344d..2c78dac233 100644 --- a/src/pkg/transform/artifact.go +++ b/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.