From 358dd41a3d0638d9c9d10b6f455c0cae057a2687 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 11 Sep 2024 15:30:02 -0700 Subject: [PATCH 01/57] add errcheck linting Signed-off-by: Kit Patella --- .golangci.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.golangci.yaml b/.golangci.yaml index 6423df7732..18b3f0f127 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -59,6 +59,7 @@ linters-settings: testifylint: enable-all: true errcheck: + check-blank: true check-type-assertions: true exclude-functions: - (*github.com/spf13/cobra.Command).Help @@ -68,6 +69,7 @@ linters-settings: issues: # Revive rules that are disabled by default. include: + - EXC0001 - EXC0012 - EXC0013 - EXC0014 From 655dd76287be8e7ed0ecdbebbb2758baf8e75024 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Thu, 12 Sep 2024 08:42:26 -0700 Subject: [PATCH 02/57] linter ignore some intentionally unhandled errs Signed-off-by: Kit Patella --- src/pkg/utils/cosign.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/pkg/utils/cosign.go b/src/pkg/utils/cosign.go index f81183741a..f8c82bc69c 100644 --- a/src/pkg/utils/cosign.go +++ b/src/pkg/utils/cosign.go @@ -228,26 +228,28 @@ func GetCosignArtifacts(image string) ([]string, error) { ref, err := name.ParseReference(image, nameOpts...) if err != nil { - return []string{}, err + return nil, err } + // FIXME(mkcp): Re-review how this function is used and whether a signed entity err-miss is something we should pass + // up to the caller. Needs a comment if nothing else. var remoteOpts []ociremote.Option simg, _ := ociremote.SignedEntity(ref, remoteOpts...) if simg == nil { - return []string{}, nil + return nil, nil } // Errors are dogsled because these functions always return a name.Tag which we can check for layers - sigRef, _ := ociremote.SignatureTag(ref, remoteOpts...) - attRef, _ := ociremote.AttestationTag(ref, remoteOpts...) + sigRef, _ := ociremote.SignatureTag(ref, remoteOpts...) //nolint:errcheck + attRef, _ := ociremote.AttestationTag(ref, remoteOpts...) //nolint:errcheck ss, err := simg.Signatures() if err != nil { - return []string{}, err + return nil, err } ssLayers, err := ss.Layers() if err != nil { - return []string{}, err + return nil, err } var cosignArtifactList = make([]string, 0) @@ -257,11 +259,11 @@ func GetCosignArtifacts(image string) ([]string, error) { atts, err := simg.Attestations() if err != nil { - return []string{}, err + return nil, err } aLayers, err := atts.Layers() if err != nil { - return []string{}, err + return nil, err } if 0 < len(aLayers) { cosignArtifactList = append(cosignArtifactList, attRef.String()) From cce62ce70ae394d4b17a4df986b1efb2f50f34bb Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Thu, 12 Sep 2024 08:43:34 -0700 Subject: [PATCH 03/57] handle some env var set and unset errors in tests Signed-off-by: Kit Patella --- src/test/e2e/12_lint_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/test/e2e/12_lint_test.go b/src/test/e2e/12_lint_test.go index dea06dd678..2c77b14056 100644 --- a/src/test/e2e/12_lint_test.go +++ b/src/test/e2e/12_lint_test.go @@ -29,9 +29,11 @@ func TestLint(t *testing.T) { testPackagePath := filepath.Join("src", "test", "packages", "12-lint") configPath := filepath.Join(testPackagePath, "zarf-config.toml") - os.Setenv("ZARF_CONFIG", configPath) + osSetErr := os.Setenv("ZARF_CONFIG", configPath) + require.NoError(t, osSetErr, "Unable to set ZARF_CONFIG") _, stderr, err := e2e.Zarf(t, "dev", "lint", testPackagePath, "-f", "good-flavor") - os.Unsetenv("ZARF_CONFIG") + osUnsetErr := os.Unsetenv("ZARF_CONFIG") + require.NoError(t, osUnsetErr, "Unable to cleanup ZARF_CONFIG") require.Error(t, err, "Require an exit code since there was warnings / errors") strippedStderr := e2e.StripMessageFormatting(stderr) From ae05ebb18550e72b7dc20ec21c83fec0db921722 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Thu, 12 Sep 2024 08:51:42 -0700 Subject: [PATCH 04/57] make some notes in cmd/destroy.go for later Signed-off-by: Kit Patella --- src/cmd/destroy.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/cmd/destroy.go b/src/cmd/destroy.go index 1b71740aa3..ba15eb45b7 100644 --- a/src/cmd/destroy.go +++ b/src/cmd/destroy.go @@ -30,6 +30,7 @@ var destroyCmd = &cobra.Command{ Aliases: []string{"d"}, Short: lang.CmdDestroyShort, Long: lang.CmdDestroyLong, + // FIXME(mkcp): This function deeply needs a refactor and un-nesting. RunE: func(cmd *cobra.Command, _ []string) error { ctx := cmd.Context() timeoutCtx, cancel := context.WithTimeout(cmd.Context(), cluster.DefaultTimeout) @@ -57,6 +58,7 @@ var destroyCmd = &cobra.Command{ // Run all the scripts! pattern := regexp.MustCompile(`(?mi)zarf-clean-.+\.sh$`) + // TODO(mkcp): Handle this error scripts, _ := helpers.RecursiveFileList(config.ZarfCleanupScriptsPath, pattern, true) // Iterate over all matching zarf-clean scripts and exec them for _, script := range scripts { @@ -72,6 +74,7 @@ var destroyCmd = &cobra.Command{ } // Try to remove the script, but ignore any errors + // TODO(mkcp): Should we be ignoring this error? Setup handling or retry, or lintignore _ = os.Remove(script) } } else { From 992aa3c8b4ef08f7ba81aca60bea4a86c0d292df Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Thu, 12 Sep 2024 08:58:53 -0700 Subject: [PATCH 05/57] direct handle some deferred cleanups to add direct error handling Signed-off-by: Kit Patella --- src/test/external/ext_out_cluster_test.go | 30 +++++++++++++++++------ 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/src/test/external/ext_out_cluster_test.go b/src/test/external/ext_out_cluster_test.go index e2cbaccefb..2cf6807d6d 100644 --- a/src/test/external/ext_out_cluster_test.go +++ b/src/test/external/ext_out_cluster_test.go @@ -146,8 +146,8 @@ func (suite *ExtOutClusterTestSuite) Test_1_Deploy() { func (suite *ExtOutClusterTestSuite) Test_2_DeployGitOps() { // Deploy the flux example package + // Cleanup dir at end of test temp := suite.T().TempDir() - defer os.Remove(temp) createPodInfoPackageWithInsecureSources(suite.T(), temp) deployArgs := []string{"package", "deploy", filepath.Join(temp, "zarf-package-podinfo-flux-amd64.tar.zst"), "--confirm"} @@ -158,6 +158,8 @@ func (suite *ExtOutClusterTestSuite) Test_2_DeployGitOps() { deployArgs = []string{"package", "deploy", path, "--confirm"} err = exec.CmdWithPrint(zarfBinPath, deployArgs...) suite.NoError(err) + err = os.Remove(temp) + suite.NoError(err, "unable to remove tempdir") } func (suite *ExtOutClusterTestSuite) Test_3_AuthToPrivateHelmChart() { @@ -168,12 +170,13 @@ func (suite *ExtOutClusterTestSuite) Test_3_AuthToPrivateHelmChart() { tempDir := suite.T().TempDir() repoPath := filepath.Join(tempDir, "repositories.yaml") - os.Setenv("HELM_REPOSITORY_CONFIG", repoPath) - defer os.Unsetenv("HELM_REPOSITORY_CONFIG") + // Cleanup at end of func + err := os.Setenv("HELM_REPOSITORY_CONFIG", repoPath) + suite.Error(err, "Unable to set HELM_REPOSITORY_CONFIG") packagePath := filepath.Join("..", "packages", "external-helm-auth") findImageArgs := []string{"dev", "find-images", packagePath} - err := exec.CmdWithPrint(zarfBinPath, findImageArgs...) + err = exec.CmdWithPrint(zarfBinPath, findImageArgs...) suite.Error(err, "Since auth has not been setup, this should fail") repoFile := repo.NewFile() @@ -195,6 +198,9 @@ func (suite *ExtOutClusterTestSuite) Test_3_AuthToPrivateHelmChart() { packageCreateArgs := []string{"package", "create", packagePath, fmt.Sprintf("--output=%s", tempDir), "--confirm"} err = exec.CmdWithPrint(zarfBinPath, packageCreateArgs...) suite.NoError(err, "Unable to create package, helm auth likely failed") + + err = os.Unsetenv("HELM_REPOSITORY_CONFIG") + suite.Error(err, "Unable to Unset HELM_REPOSITORY_CONFIG") } func (suite *ExtOutClusterTestSuite) createHelmChartInGitea(baseURL string, username string, password string) { @@ -211,9 +217,9 @@ func (suite *ExtOutClusterTestSuite) createHelmChartInGitea(baseURL string, user suite.NoError(err) url := fmt.Sprintf("%s/api/packages/%s/helm/api/charts", baseURL, username) + // Close the file directly at the end of the test file, err := os.Open(podinfoTarballPath) suite.NoError(err) - defer file.Close() body := &bytes.Buffer{} writer := multipart.NewWriter(body) @@ -221,7 +227,8 @@ func (suite *ExtOutClusterTestSuite) createHelmChartInGitea(baseURL string, user suite.NoError(err) _, err = io.Copy(part, file) suite.NoError(err) - writer.Close() + err = writer.Close() + suite.NoError(err) req, err := http.NewRequest("POST", url, body) suite.NoError(err) @@ -233,7 +240,11 @@ func (suite *ExtOutClusterTestSuite) createHelmChartInGitea(baseURL string, user resp, err := client.Do(req) suite.NoError(err) - resp.Body.Close() + err = resp.Body.Close() + suite.NoError(err) + + err = file.Close() + suite.NoError(err) } func (suite *ExtOutClusterTestSuite) makeGiteaUserPrivate(baseURL string, username string, password string) { @@ -256,12 +267,15 @@ func (suite *ExtOutClusterTestSuite) makeGiteaUserPrivate(baseURL string, userna req.SetBasicAuth(username, password) client := &http.Client{} + // Close resp body at end of test resp, err := client.Do(req) suite.NoError(err) - defer resp.Body.Close() _, err = io.ReadAll(resp.Body) suite.NoError(err) + + err = resp.Body.Close() + suite.NoError(err, "Unable to close response body") } func TestExtOurClusterTestSuite(t *testing.T) { From 81e54f744407d3d2e647c6e0b36fb45d8c200b16 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Fri, 13 Sep 2024 15:43:36 -0700 Subject: [PATCH 06/57] capture errors from cmd/root Signed-off-by: Kit Patella --- src/cmd/root.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/cmd/root.go b/src/cmd/root.go index 188f91e8cc..3c9f9bac4d 100644 --- a/src/cmd/root.go +++ b/src/cmd/root.go @@ -77,8 +77,11 @@ var rootCmd = &cobra.Command{ SilenceErrors: true, Run: func(cmd *cobra.Command, args []string) { zarfLogo := message.GetLogo() - _, _ = fmt.Fprintln(os.Stderr, zarfLogo) - cmd.Help() + _, _ = fmt.Fprintln(os.Stderr, zarfLogo) //nolint:errcheck + err := cmd.Help() + if err != nil { + _, _ = fmt.Fprintln(os.Stderr, err) //nolint:errcheck + } if len(args) > 0 { if strings.Contains(args[0], config.ZarfPackagePrefix) || strings.Contains(args[0], "zarf-init") { From 9b2b2e9521e3b1086fb6023bc6c622c8d1766425 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Fri, 13 Sep 2024 16:27:17 -0700 Subject: [PATCH 07/57] capture errors on src/config/config.go. checking ci Signed-off-by: Kit Patella --- src/cmd/initialize.go | 21 +++++++++++++++------ src/cmd/tools/zarf.go | 12 ++++++++---- src/config/config.go | 13 ++++++++----- src/internal/packager/images/pull.go | 6 +++++- src/internal/packager/sbom/catalog.go | 6 +++++- src/pkg/packager/composer/oci.go | 6 +++++- src/pkg/packager/creator/normal.go | 6 +++++- src/pkg/packager/deploy.go | 13 ++++++++----- 8 files changed, 59 insertions(+), 24 deletions(-) diff --git a/src/cmd/initialize.go b/src/cmd/initialize.go index 376db85da9..9dd680c259 100644 --- a/src/cmd/initialize.go +++ b/src/cmd/initialize.go @@ -94,19 +94,27 @@ func findInitPackage(ctx context.Context, initPackageName string) (string, error } // Create the cache directory if it doesn't exist - if helpers.InvalidPath(config.GetAbsCachePath()) { - if err := helpers.CreateDirectory(config.GetAbsCachePath(), helpers.ReadExecuteAllWriteUser); err != nil { - return "", fmt.Errorf("unable to create the cache directory %s: %w", config.GetAbsCachePath(), err) + absCachePath, err := config.GetAbsCachePath() + if err != nil { + return "", err + } + // Verify that we can write to the path + // FIXME(mkcp): Decompose this into a helper function + if helpers.InvalidPath(absCachePath) { + // Create the directory if the path is invalid + if err := helpers.CreateDirectory(absCachePath, helpers.ReadExecuteAllWriteUser); err != nil { + return "", fmt.Errorf("unable to create the cache directory %s: %w", absCachePath, err) } } // Next, look in the cache directory - if !helpers.InvalidPath(filepath.Join(config.GetAbsCachePath(), initPackageName)) { - return filepath.Join(config.GetAbsCachePath(), initPackageName), nil + if !helpers.InvalidPath(filepath.Join(absCachePath, initPackageName)) { + // join and return + return filepath.Join(absCachePath, initPackageName), nil } // Finally, if the init-package doesn't exist in the cache directory, suggest downloading it - downloadCacheTarget, err := downloadInitPackage(ctx, config.GetAbsCachePath()) + downloadCacheTarget, err := downloadInitPackage(ctx, absCachePath) if err != nil { if errors.Is(err, lang.ErrInitNotFound) { return "", err @@ -130,6 +138,7 @@ func downloadInitPackage(ctx context.Context, cacheDirectory string) (string, er message.Note(lang.CmdInitPullNote) // Prompt the user if --confirm not specified + // FIXME(mkcp): This condition can never be met if !confirmDownload { prompt := &survey.Confirm{ Message: lang.CmdInitPullConfirm, diff --git a/src/cmd/tools/zarf.go b/src/cmd/tools/zarf.go index 5a9cd11718..df516f9506 100644 --- a/src/cmd/tools/zarf.go +++ b/src/cmd/tools/zarf.go @@ -205,11 +205,15 @@ var clearCacheCmd = &cobra.Command{ Aliases: []string{"c"}, Short: lang.CmdToolsClearCacheShort, RunE: func(_ *cobra.Command, _ []string) error { - message.Notef(lang.CmdToolsClearCacheDir, config.GetAbsCachePath()) - if err := os.RemoveAll(config.GetAbsCachePath()); err != nil { - return fmt.Errorf("unable to clear the cache directory %s: %w", config.GetAbsCachePath(), err) + cachePath, err := config.GetAbsCachePath() + if err != nil { + return err + } + message.Notef(lang.CmdToolsClearCacheDir, cachePath) + if err := os.RemoveAll(cachePath); err != nil { + return fmt.Errorf("unable to clear the cache directory %s: %w", cachePath, err) } - message.Successf(lang.CmdToolsClearCacheSuccess, config.GetAbsCachePath()) + message.Successf(lang.CmdToolsClearCacheSuccess, cachePath) return nil }, } diff --git a/src/config/config.go b/src/config/config.go index 4a4001dce6..ca80f619b6 100644 --- a/src/config/config.go +++ b/src/config/config.go @@ -97,16 +97,19 @@ func GetDataInjectionMarker() string { } // GetAbsCachePath gets the absolute cache path for images and git repos. -func GetAbsCachePath() string { +func GetAbsCachePath() (string, error) { return GetAbsHomePath(CommonOptions.CachePath) } // GetAbsHomePath replaces ~ with the absolute path to a user's home dir -func GetAbsHomePath(path string) string { - homePath, _ := os.UserHomeDir() +func GetAbsHomePath(path string) (string, error) { + homePath, err := os.UserHomeDir() + if err != nil { + return "", err + } if strings.HasPrefix(path, "~") { - return strings.Replace(path, "~", homePath, 1) + return strings.Replace(path, "~", homePath, 1), nil } - return path + return path, nil } diff --git a/src/internal/packager/images/pull.go b/src/internal/packager/images/pull.go index 2e206742c6..a11b6097d7 100644 --- a/src/internal/packager/images/pull.go +++ b/src/internal/packager/images/pull.go @@ -300,7 +300,11 @@ func CleanupInProgressLayers(ctx context.Context, img v1.Image) error { if err != nil { return err } - cacheDir := filepath.Join(config.GetAbsCachePath(), layout.ImagesDir) + absPath, err := config.GetAbsCachePath() + if err != nil { + return err + } + cacheDir := filepath.Join(absPath, layout.ImagesDir) location := filepath.Join(cacheDir, digest.String()) info, err := os.Stat(location) if errors.Is(err, fs.ErrNotExist) { diff --git a/src/internal/packager/sbom/catalog.go b/src/internal/packager/sbom/catalog.go index 58aa23870a..65c32c039b 100755 --- a/src/internal/packager/sbom/catalog.go +++ b/src/internal/packager/sbom/catalog.go @@ -54,9 +54,13 @@ var componentPrefix = "zarf-component-" func Catalog(ctx context.Context, componentSBOMs map[string]*layout.ComponentSBOM, imageList []transform.Image, paths *layout.PackagePaths) error { imageCount := len(imageList) componentCount := len(componentSBOMs) + cachePath, err := config.GetAbsCachePath() + if err != nil { + return err + } builder := Builder{ spinner: message.NewProgressSpinner("Creating SBOMs for %d images and %d components with files.", imageCount, componentCount), - cachePath: config.GetAbsCachePath(), + cachePath: cachePath, imagesPath: paths.Images.Base, outputDir: paths.SBOMs.Path, } diff --git a/src/pkg/packager/composer/oci.go b/src/pkg/packager/composer/oci.go index 2541e45f32..4d2589f717 100644 --- a/src/pkg/packager/composer/oci.go +++ b/src/pkg/packager/composer/oci.go @@ -64,7 +64,11 @@ func (ic *ImportChain) fetchOCISkeleton(ctx context.Context) error { componentDesc := manifest.Locate(filepath.Join(layout.ComponentsDir, fmt.Sprintf("%s.tar", name))) - cache := filepath.Join(config.GetAbsCachePath(), "oci") + absCachePath, err := config.GetAbsCachePath() + if err != nil { + return err + } + cache := filepath.Join(absCachePath, "oci") if err := helpers.CreateDirectory(cache, helpers.ReadWriteExecuteUser); err != nil { return err } diff --git a/src/pkg/packager/creator/normal.go b/src/pkg/packager/creator/normal.go index 4c71ec9ac7..87ad5ee0b8 100644 --- a/src/pkg/packager/creator/normal.go +++ b/src/pkg/packager/creator/normal.go @@ -184,12 +184,16 @@ func (pc *PackageCreator) Assemble(ctx context.Context, dst *layout.PackagePaths dst.AddImages() + cachePath, err := config.GetAbsCachePath() + if err != nil { + return err + } pullCfg := images.PullConfig{ DestinationDirectory: dst.Images.Base, ImageList: imageList, Arch: arch, RegistryOverrides: pc.createOpts.RegistryOverrides, - CacheDirectory: filepath.Join(config.GetAbsCachePath(), layout.ImagesDir), + CacheDirectory: filepath.Join(cachePath, layout.ImagesDir), } pulled, err := images.Pull(ctx, pullCfg) diff --git a/src/pkg/packager/deploy.go b/src/pkg/packager/deploy.go index ee3796ac6c..5aba0ffc1e 100644 --- a/src/pkg/packager/deploy.go +++ b/src/pkg/packager/deploy.go @@ -438,8 +438,11 @@ func (p *Packager) processComponentFiles(component v1alpha1.ZarfComponent, pkgLo } // Replace temp target directory and home directory - file.Target = strings.Replace(file.Target, "###ZARF_TEMP###", p.layout.Base, 1) - file.Target = config.GetAbsHomePath(file.Target) + target, err := config.GetAbsHomePath(strings.Replace(file.Target, "###ZARF_TEMP###", p.layout.Base, 1)) + if err != nil { + return err + } + file.Target = target fileList := []string{} if helpers.IsDir(fileLocation) { @@ -467,9 +470,9 @@ func (p *Packager) processComponentFiles(component v1alpha1.ZarfComponent, pkgLo // Copy the file to the destination spinner.Updatef("Saving %s", file.Target) - err := helpers.CreatePathAndCopy(fileLocation, file.Target) - if err != nil { - return fmt.Errorf("unable to copy file %s to %s: %w", fileLocation, file.Target, err) + err2 := helpers.CreatePathAndCopy(fileLocation, file.Target) + if err2 != nil { + return fmt.Errorf("unable to copy file %s to %s: %w", fileLocation, file.Target, err2) } // Loop over all symlinks and create them From a508aa7501fc900d49f1341da96274bbbe7eb44c Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 10:07:05 -0700 Subject: [PATCH 08/57] fix: ensure we capture and propagate errors in src/internal Signed-off-by: Kit Patella --- src/internal/git/repository_test.go | 3 ++- src/internal/gitea/gitea.go | 9 +++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/internal/git/repository_test.go b/src/internal/git/repository_test.go index 7e75319eee..e833e6e2f7 100644 --- a/src/internal/git/repository_test.go +++ b/src/internal/git/repository_test.go @@ -56,7 +56,8 @@ func TestRepository(t *testing.T) { require.NoError(t, err) _, err = newFile.Write([]byte("Hello World")) require.NoError(t, err) - newFile.Close() + err = newFile.Close() + require.NoError(t, err) _, err = w.Add(filePath) require.NoError(t, err) _, err = w.Commit("Initial commit", &git.CommitOptions{ diff --git a/src/internal/gitea/gitea.go b/src/internal/gitea/gitea.go index 94244d03f1..6aff480af6 100644 --- a/src/internal/gitea/gitea.go +++ b/src/internal/gitea/gitea.go @@ -8,6 +8,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -47,7 +48,7 @@ func NewClient(endpoint, username, password string) (*Client, error) { } // DoRequest performs a request to the Gitea API at the given path. -func (g *Client) DoRequest(ctx context.Context, method string, path string, body []byte) ([]byte, int, error) { +func (g *Client) DoRequest(ctx context.Context, method string, path string, body []byte) (_ []byte, _ int, err error) { u, err := g.endpoint.Parse(path) if err != nil { return nil, 0, err @@ -60,10 +61,14 @@ func (g *Client) DoRequest(ctx context.Context, method string, path string, body req.Header.Add("accept", "application/json") req.Header.Add("content-type", "application/json") resp, err := g.httpClient.Do(req) + // Ensure we close the body of the http client and capture the error + defer func() { + errClose := resp.Body.Close() + err = errors.Join(err, errClose) + }() if err != nil { return nil, 0, err } - defer resp.Body.Close() b, err := io.ReadAll(resp.Body) if err != nil { return nil, 0, err From 6f53a29e31a8dea070869849dd5666c5f635900b Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 10:35:11 -0700 Subject: [PATCH 09/57] fix: propagate or log unhandled errors in src/cmd Signed-off-by: Kit Patella --- src/cmd/destroy.go | 15 +++++++++----- src/cmd/dev.go | 29 ++++++++++++++++++++------- src/cmd/internal.go | 10 +++++++--- src/cmd/package.go | 46 +++++++++++++++++++++++++++++++++++-------- src/cmd/root.go | 4 ++-- src/cmd/tools/helm.go | 6 +++++- 6 files changed, 84 insertions(+), 26 deletions(-) diff --git a/src/cmd/destroy.go b/src/cmd/destroy.go index ba15eb45b7..427292bd5e 100644 --- a/src/cmd/destroy.go +++ b/src/cmd/destroy.go @@ -8,6 +8,7 @@ import ( "context" "errors" "fmt" + "log/slog" "os" "regexp" @@ -58,8 +59,10 @@ var destroyCmd = &cobra.Command{ // Run all the scripts! pattern := regexp.MustCompile(`(?mi)zarf-clean-.+\.sh$`) - // TODO(mkcp): Handle this error - scripts, _ := helpers.RecursiveFileList(config.ZarfCleanupScriptsPath, pattern, true) + scripts, err := helpers.RecursiveFileList(config.ZarfCleanupScriptsPath, pattern, true) + if err != nil { + return err + } // Iterate over all matching zarf-clean scripts and exec them for _, script := range scripts { // Run the matched script @@ -73,9 +76,11 @@ var destroyCmd = &cobra.Command{ return fmt.Errorf("received an error when executing the script %s: %w", script, err) } - // Try to remove the script, but ignore any errors - // TODO(mkcp): Should we be ignoring this error? Setup handling or retry, or lintignore - _ = os.Remove(script) + // Try to remove the script, but ignore any errors and debug log them + err = os.Remove(script) + if err != nil { + slog.Debug("Unable to remove script", "script", script, "error", err) + } } } else { // Perform chart uninstallation diff --git a/src/cmd/dev.go b/src/cmd/dev.go index 85f08e9c11..31c7c31a1a 100644 --- a/src/cmd/dev.go +++ b/src/cmd/dev.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "io" + "log/slog" "os" "path/filepath" "strings" @@ -148,14 +149,13 @@ var devSha256SumCmd = &cobra.Command{ Aliases: []string{"s"}, Short: lang.CmdDevSha256sumShort, Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { + RunE: func(cmd *cobra.Command, args []string) (err error) { hashErr := errors.New("unable to compute the SHA256SUM hash") fileName := args[0] var tmp string var data io.ReadCloser - var err error if helpers.IsURL(fileName) { message.Warn(lang.CmdDevSha256sumRemoteWarning) @@ -182,7 +182,10 @@ var devSha256SumCmd = &cobra.Command{ fileName = downloadPath - defer os.RemoveAll(tmp) + defer func(path string) { + errRemove := os.RemoveAll(path) + err = errors.Join(err, errRemove) + }(tmp) } if extractPath != "" { @@ -191,7 +194,10 @@ var devSha256SumCmd = &cobra.Command{ if err != nil { return errors.Join(hashErr, err) } - defer os.RemoveAll(tmp) + defer func(path string) { + errRemove := os.RemoveAll(path) + err = errors.Join(err, errRemove) + }(tmp) } extractedFile := filepath.Join(tmp, extractPath) @@ -208,7 +214,10 @@ var devSha256SumCmd = &cobra.Command{ if err != nil { return errors.Join(hashErr, err) } - defer data.Close() + defer func(data io.ReadCloser) { + errClose := data.Close() + err = errors.Join(err, errClose) + }(data) hash, err := helpers.GetSHA256Hash(data) if err != nil { @@ -323,8 +332,14 @@ func init() { // use the package create config for this and reset it here to avoid overwriting the config.CreateOptions.SetVariables devFindImagesCmd.Flags().StringToStringVar(&pkgConfig.CreateOpts.SetVariables, "set", v.GetStringMapString(common.VPkgCreateSet), lang.CmdDevFlagSet) - devFindImagesCmd.Flags().MarkDeprecated("set", "this field is replaced by create-set") - devFindImagesCmd.Flags().MarkHidden("set") + err := devFindImagesCmd.Flags().MarkDeprecated("set", "this field is replaced by create-set") + if err != nil { + slog.Debug("Unable to mark dev-find-images flag as set", "error", err) + } + err = devFindImagesCmd.Flags().MarkHidden("set") + if err != nil { + slog.Debug("Unable to mark dev-find-images flag as hidden", "error", err) + } devFindImagesCmd.Flags().StringVarP(&pkgConfig.CreateOpts.Flavor, "flavor", "f", v.GetString(common.VPkgCreateFlavor), lang.CmdPackageCreateFlagFlavor) devFindImagesCmd.Flags().StringToStringVar(&pkgConfig.CreateOpts.SetVariables, "create-set", v.GetStringMapString(common.VPkgCreateSet), lang.CmdDevFlagSet) devFindImagesCmd.Flags().StringToStringVar(&pkgConfig.PkgOpts.SetVariables, "deploy-set", v.GetStringMapString(common.VPkgDeploySet), lang.CmdPackageDeployFlagSet) diff --git a/src/cmd/internal.go b/src/cmd/internal.go index 4a393d01e6..82e1e50596 100644 --- a/src/cmd/internal.go +++ b/src/cmd/internal.go @@ -9,6 +9,7 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "os" "path/filepath" "strings" @@ -348,8 +349,8 @@ var isValidHostname = &cobra.Command{ Short: lang.CmdInternalIsValidHostnameShort, RunE: func(_ *cobra.Command, _ []string) error { if valid := helpers.IsValidHostName(); !valid { - hostname, _ := os.Hostname() - return fmt.Errorf("the hostname %s is not valid. Ensure the hostname meets RFC1123 requirements https://www.rfc-editor.org/rfc/rfc1123.html", hostname) + hostname, err := os.Hostname() + return fmt.Errorf("the hostname %s is not valid. Ensure the hostname meets RFC1123 requirements https://www.rfc-editor.org/rfc/rfc1123.html, error=%w", hostname, err) } return nil }, @@ -388,6 +389,9 @@ func addHiddenDummyFlag(cmd *cobra.Command, flagDummy string) { if cmd.PersistentFlags().Lookup(flagDummy) == nil { var dummyStr string cmd.PersistentFlags().StringVar(&dummyStr, flagDummy, "", "") - cmd.PersistentFlags().MarkHidden(flagDummy) + err := cmd.PersistentFlags().MarkHidden(flagDummy) + if err != nil { + slog.Debug("Unable to add hidden dummy flag", "error", err) + } } } diff --git a/src/cmd/package.go b/src/cmd/package.go index 3ae1b0e2ff..e55b449693 100644 --- a/src/cmd/package.go +++ b/src/cmd/package.go @@ -8,6 +8,7 @@ import ( "context" "errors" "fmt" + "log/slog" "os" "path/filepath" "regexp" @@ -381,9 +382,23 @@ func choosePackage(args []string) (string, error) { prompt := &survey.Input{ Message: lang.CmdPackageChoose, Suggest: func(toComplete string) []string { - files, _ := filepath.Glob(config.ZarfPackagePrefix + toComplete + "*.tar") - zstFiles, _ := filepath.Glob(config.ZarfPackagePrefix + toComplete + "*.tar.zst") - splitFiles, _ := filepath.Glob(config.ZarfPackagePrefix + toComplete + "*.part000") + tarPath := config.ZarfPackagePrefix + toComplete + "*.tar" + files, err := filepath.Glob(tarPath) + if err != nil { + slog.Debug("Unable to glob", "tarPath", tarPath, "error", err) + } + + zstPath := config.ZarfPackagePrefix + toComplete + "*.tar.zst" + zstFiles, err := filepath.Glob(zstPath) + if err != nil { + slog.Debug("Unable to glob", "zstPath", zstPath, "error", err) + } + + splitPath := config.ZarfPackagePrefix + toComplete + "*.part000" + splitFiles, err := filepath.Glob(splitPath) + if err != nil { + slog.Debug("Unable to glob", "splitPath", splitPath, "error", err) + } files = append(files, zstFiles...) files = append(files, splitFiles...) @@ -408,7 +423,10 @@ func getPackageCompletionArgs(cmd *cobra.Command, _ []string, _ string) ([]strin ctx := cmd.Context() - deployedZarfPackages, _ := c.GetDeployedZarfPackages(ctx) + deployedZarfPackages, err := c.GetDeployedZarfPackages(ctx) + if err != nil { + slog.Error("Unable to get deployed zarf packages for package completion args", "error", err) + } // Populate list of package names for _, pkg := range deployedZarfPackages { pkgCandidates = append(pkgCandidates, pkg.Name) @@ -477,9 +495,18 @@ func bindCreateFlags(v *viper.Viper) { createFlags.IntVar(&pkgConfig.PkgOpts.Retries, "retries", v.GetInt(common.VPkgRetries), lang.CmdPackageFlagRetries) - createFlags.MarkHidden("output-directory") - createFlags.MarkHidden("key") - createFlags.MarkHidden("key-pass") + errOD := createFlags.MarkHidden("output-directory") + if errOD != nil { + slog.Debug("Unable to mark flag output-directory", "error", errOD) + } + errKey := createFlags.MarkHidden("key") + if errKey != nil { + slog.Debug("Unable to mark flag key", "error", errKey) + } + errKP := createFlags.MarkHidden("key-pass") + if errKP != nil { + slog.Debug("Unable to mark flag key-pass", "error", errKP) + } } func bindDeployFlags(v *viper.Viper) { @@ -500,7 +527,10 @@ func bindDeployFlags(v *viper.Viper) { deployFlags.StringVar(&pkgConfig.PkgOpts.SGetKeyPath, "sget", v.GetString(common.VPkgDeploySget), lang.CmdPackageDeployFlagSget) deployFlags.BoolVar(&pkgConfig.PkgOpts.SkipSignatureValidation, "skip-signature-validation", false, lang.CmdPackageFlagSkipSignatureValidation) - deployFlags.MarkHidden("sget") + err := deployFlags.MarkHidden("sget") + if err != nil { + slog.Debug("Unable to mark flag sget", "error", err) + } } func bindMirrorFlags(v *viper.Viper) { diff --git a/src/cmd/root.go b/src/cmd/root.go index 3c9f9bac4d..bf695e3760 100644 --- a/src/cmd/root.go +++ b/src/cmd/root.go @@ -77,10 +77,10 @@ var rootCmd = &cobra.Command{ SilenceErrors: true, Run: func(cmd *cobra.Command, args []string) { zarfLogo := message.GetLogo() - _, _ = fmt.Fprintln(os.Stderr, zarfLogo) //nolint:errcheck + _, _ = fmt.Fprintln(os.Stderr, zarfLogo) err := cmd.Help() if err != nil { - _, _ = fmt.Fprintln(os.Stderr, err) //nolint:errcheck + _, _ = fmt.Fprintln(os.Stderr, err) } if len(args) > 0 { diff --git a/src/cmd/tools/helm.go b/src/cmd/tools/helm.go index 0d37f8d017..ee22acc56d 100644 --- a/src/cmd/tools/helm.go +++ b/src/cmd/tools/helm.go @@ -5,6 +5,7 @@ package tools import ( + "log/slog" "os" "github.com/zarf-dev/zarf/src/cmd/tools/helm" @@ -24,7 +25,10 @@ func init() { helmArgs = os.Args[3:] } // The inclusion of Helm in this manner should be changed once https://github.com/helm/helm/pull/12725 is merged - helmCmd, _ := helm.NewRootCmd(actionConfig, os.Stdout, helmArgs) + helmCmd, err := helm.NewRootCmd(actionConfig, os.Stdout, helmArgs) + if err != nil { + slog.Error("Failed to initialize helm command", "error", err) + } helmCmd.Short = lang.CmdToolsHelmShort helmCmd.Long = lang.CmdToolsHelmLong helmCmd.AddCommand(newVersionCmd("helm", helmVersion)) From 4283fafe8b8a543149ee0fda757bde86391433ad Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 12:01:13 -0700 Subject: [PATCH 10/57] fix: check for or intentionally ignore some dangling errors in tests Signed-off-by: Kit Patella --- src/test/common.go | 19 +++++++++++--- src/test/e2e/00_use_cli_test.go | 3 ++- src/test/e2e/14_oci_compose_test.go | 8 ++++-- src/test/e2e/20_zarf_init_test.go | 4 +-- src/test/e2e/21_connect_creds_test.go | 4 ++- src/test/e2e/24_variables_test.go | 6 +++-- src/test/e2e/25_helm_test.go | 8 +++--- src/test/e2e/28_wait_test.go | 2 +- src/test/e2e/29_config_file_test.go | 29 ++++++++++++++-------- src/test/external/common.go | 3 ++- src/test/external/ext_in_cluster_test.go | 5 +++- src/test/external/ext_out_cluster_test.go | 16 +++++++----- src/test/upgrade/previously_built_test.go | 30 +++++++++++++++-------- 13 files changed, 93 insertions(+), 44 deletions(-) diff --git a/src/test/common.go b/src/test/common.go index 746600feb1..dbeae330ff 100644 --- a/src/test/common.go +++ b/src/test/common.go @@ -6,7 +6,9 @@ package test import ( "bufio" + "errors" "fmt" + "log/slog" "os" "regexp" "runtime" @@ -53,13 +55,16 @@ func GetCLIName() string { } // Zarf executes a Zarf command. -func (e2e *ZarfE2ETest) Zarf(t *testing.T, args ...string) (string, string, error) { +func (e2e *ZarfE2ETest) Zarf(t *testing.T, args ...string) (_ string, _ string, err error) { if !slices.Contains(args, "--tmpdir") && !slices.Contains(args, "tools") { tmpdir, err := os.MkdirTemp("", "zarf-") if err != nil { return "", "", err } - defer os.RemoveAll(tmpdir) + defer func(path string) { + errRemove := os.RemoveAll(path) + err = errors.Join(err, errRemove) + }(tmpdir) args = append(args, "--tmpdir", tmpdir) } if !slices.Contains(args, "--zarf-cache") && !slices.Contains(args, "tools") && os.Getenv("CI") == "true" { @@ -74,7 +79,10 @@ func (e2e *ZarfE2ETest) Zarf(t *testing.T, args ...string) (string, string, erro return "", "", err } args = append(args, "--zarf-cache", cacheDir) - defer os.RemoveAll(cacheDir) + defer func(path string) { + errRemove := os.RemoveAll(path) + err = errors.Join(err, errRemove) + }(cacheDir) } return exec.CmdWithTesting(t, exec.PrintCfg(), e2e.ZarfBinPath, args...) } @@ -89,7 +97,10 @@ func (e2e *ZarfE2ETest) Kubectl(t *testing.T, args ...string) (string, string, e // CleanFiles removes files and directories that have been created during the test. func (e2e *ZarfE2ETest) CleanFiles(files ...string) { for _, file := range files { - _ = os.RemoveAll(file) + err := os.RemoveAll(file) + if err != nil { + slog.Debug("Unable to cleanup files", "files", files, "error", err) + } } } diff --git a/src/test/e2e/00_use_cli_test.go b/src/test/e2e/00_use_cli_test.go index 366bacb7f9..d834445367 100644 --- a/src/test/e2e/00_use_cli_test.go +++ b/src/test/e2e/00_use_cli_test.go @@ -110,7 +110,8 @@ func TestUseCLI(t *testing.T) { t.Run("changing log level", func(t *testing.T) { t.Parallel() // Test that changing the log level actually applies the requested level - _, stdErr, _ := e2e.Zarf(t, "internal", "crc32", "zarf", "--log-level=debug") + _, stdErr, err := e2e.Zarf(t, "internal", "crc32", "zarf", "--log-level=debug") + require.NoError(t, err) expectedOutString := "Log level set to debug" require.Contains(t, stdErr, expectedOutString, "The log level should be changed to 'debug'") }) diff --git a/src/test/e2e/14_oci_compose_test.go b/src/test/e2e/14_oci_compose_test.go index 7159394107..95d1436f74 100644 --- a/src/test/e2e/14_oci_compose_test.go +++ b/src/test/e2e/14_oci_compose_test.go @@ -140,8 +140,6 @@ func (suite *PublishCopySkeletonSuite) Test_2_FilePaths() { var pkg v1alpha1.ZarfPackage unpacked := strings.TrimSuffix(pkgTar, ".tar.zst") - defer os.RemoveAll(unpacked) - defer os.RemoveAll(pkgTar) _, _, err := e2e.Zarf(suite.T(), "tools", "archiver", "decompress", pkgTar, unpacked, "--unarchive-all") suite.NoError(err) suite.DirExists(unpacked) @@ -176,6 +174,12 @@ func (suite *PublishCopySkeletonSuite) Test_2_FilePaths() { isSkeleton = true } suite.verifyComponentPaths(unpacked, components, isSkeleton) + + // Cleanup resources + err = os.RemoveAll(unpacked) + suite.NoError(err) + err = os.RemoveAll(pkgTar) + suite.NoError(err) } } diff --git a/src/test/e2e/20_zarf_init_test.go b/src/test/e2e/20_zarf_init_test.go index c94bfa4226..66106556bd 100644 --- a/src/test/e2e/20_zarf_init_test.go +++ b/src/test/e2e/20_zarf_init_test.go @@ -104,8 +104,8 @@ func TestZarfInit(t *testing.T) { verifyZarfServiceLabels(t) // Special sizing-hacking for reducing resources where Kind + CI eats a lot of free cycles (ignore errors) - _, _, _ = e2e.Kubectl(t, "scale", "deploy", "-n", "kube-system", "coredns", "--replicas=1") - _, _, _ = e2e.Kubectl(t, "scale", "deploy", "-n", "zarf", "agent-hook", "--replicas=1") + _, _, _ = e2e.Kubectl(t, "scale", "deploy", "-n", "kube-system", "coredns", "--replicas=1") //nolint:errcheck + _, _, _ = e2e.Kubectl(t, "scale", "deploy", "-n", "zarf", "agent-hook", "--replicas=1") //nolint:errcheck } func checkLogForSensitiveState(t *testing.T, logText string, zarfState types.ZarfState) { diff --git a/src/test/e2e/21_connect_creds_test.go b/src/test/e2e/21_connect_creds_test.go index c58244a736..93a28c1690 100644 --- a/src/test/e2e/21_connect_creds_test.go +++ b/src/test/e2e/21_connect_creds_test.go @@ -74,7 +74,6 @@ func TestMetrics(t *testing.T) { if err != nil { t.Fatal(err) } - defer resp.Body.Close() // Read the response body body, err := io.ReadAll(resp.Body) @@ -86,6 +85,9 @@ func TestMetrics(t *testing.T) { require.Contains(t, string(body), desiredString) require.NoError(t, err, resp) require.Equal(t, 200, resp.StatusCode) + + err = resp.Body.Close() + require.NoError(t, err) } func connectToZarfServices(ctx context.Context, t *testing.T) { diff --git a/src/test/e2e/24_variables_test.go b/src/test/e2e/24_variables_test.go index f5b06116d0..72490c3ebb 100644 --- a/src/test/e2e/24_variables_test.go +++ b/src/test/e2e/24_variables_test.go @@ -43,7 +43,8 @@ func TestVariables(t *testing.T) { require.Contains(t, stdErr, "", expectedOutString) // Test that not specifying a prompted variable results in an error - _, stdErr, _ = e2e.Zarf(t, "package", "deploy", path, "--confirm") + _, stdErr, err = e2e.Zarf(t, "package", "deploy", path, "--confirm") + require.NoError(t, err) expectedOutString = "variable 'SITE_NAME' must be '--set' when using the '--confirm' flag" require.Contains(t, stdErr, "", expectedOutString) @@ -73,7 +74,8 @@ func TestVariables(t *testing.T) { require.Contains(t, string(outputTF), "unicorn-land") // Verify the configmap was properly templated - kubectlOut, _, _ := e2e.Kubectl(t, "-n", "nginx", "get", "configmap", "nginx-configmap", "-o", "jsonpath='{.data.index\\.html}' ") + kubectlOut, _, err := e2e.Kubectl(t, "-n", "nginx", "get", "configmap", "nginx-configmap", "-o", "jsonpath='{.data.index\\.html}' ") + require.NoError(t, err, "unable to get nginx configmap") // OPTIONAL_FOOTER should remain unset because it was not set during deploy require.Contains(t, string(kubectlOut), "\n \n ") // STYLE should take the default value diff --git a/src/test/e2e/25_helm_test.go b/src/test/e2e/25_helm_test.go index 3e51900cdd..8589f8156f 100644 --- a/src/test/e2e/25_helm_test.go +++ b/src/test/e2e/25_helm_test.go @@ -99,7 +99,8 @@ func testHelmEscaping(t *testing.T) { require.NoError(t, err, stdOut, stdErr) // Verify the configmap was deployed, escaped, and contains all of its data - kubectlOut, _ := exec.Command("kubectl", "-n", "default", "describe", "cm", "dont-template-me").Output() + kubectlOut, err := exec.Command("kubectl", "-n", "default", "describe", "cm", "dont-template-me").Output() + require.NoError(t, err, "unable to describe configmap") require.Contains(t, string(kubectlOut), `alert: OOMKilled {{ "{{ \"random.Values\" }}" }}`) require.Contains(t, string(kubectlOut), "backtick1: \"content with backticks `some random things`\"") require.Contains(t, string(kubectlOut), "backtick2: \"nested templating with backticks {{` random.Values `}}\"") @@ -175,8 +176,9 @@ func testHelmAdoption(t *testing.T) { deploymentManifest := "src/test/packages/25-manifest-adoption/deployment.yaml" // Deploy dos-games manually into the cluster without Zarf - kubectlOut, _, _ := e2e.Kubectl(t, "apply", "-f", deploymentManifest) - require.Contains(t, string(kubectlOut), "deployment.apps/game created") + kubectlOut, _, err := e2e.Kubectl(t, "apply", "-f", deploymentManifest) + require.NoError(t, err, "unable to apply", "deploymentManifest", deploymentManifest) + require.Contains(t, kubectlOut, "deployment.apps/game created") // Deploy dos-games into the cluster with Zarf stdOut, stdErr, err := e2e.Zarf(t, "package", "deploy", packagePath, "--confirm", "--adopt-existing-resources") diff --git a/src/test/e2e/28_wait_test.go b/src/test/e2e/28_wait_test.go index e67b60d178..2881966863 100644 --- a/src/test/e2e/28_wait_test.go +++ b/src/test/e2e/28_wait_test.go @@ -51,7 +51,7 @@ func TestNoWait(t *testing.T) { case <-time.After(30 * time.Second): t.Error("Timeout waiting for zarf deploy (it tried to wait)") t.Log("Removing hanging namespace...") - _, _, _ = e2e.Kubectl(t, "delete", "namespace", "no-wait", "--force=true", "--wait=false", "--grace-period=0") + _, _, _ = e2e.Kubectl(t, "delete", "namespace", "no-wait", "--force=true", "--wait=false", "--grace-period=0") //nolint:errcheck } require.NoError(t, err, stdOut, stdErr) diff --git a/src/test/e2e/29_config_file_test.go b/src/test/e2e/29_config_file_test.go index 0cea0b4dd9..043cc9a606 100644 --- a/src/test/e2e/29_config_file_test.go +++ b/src/test/e2e/29_config_file_test.go @@ -26,7 +26,6 @@ func TestConfigFile(t *testing.T) { // Test the config file environment variable t.Setenv("ZARF_CONFIG", filepath.Join(dir, config)) - defer os.Unsetenv("ZARF_CONFIG") configFileTests(t, dir, path) configFileDefaultTests(t) @@ -34,7 +33,10 @@ func TestConfigFile(t *testing.T) { stdOut, stdErr, err := e2e.Zarf(t, "package", "remove", path, "--confirm") require.NoError(t, err, stdOut, stdErr) + // Cleanup e2e.CleanFiles(path) + err = os.Unsetenv("ZARF_CONFIG") + require.NoError(t, err) } func configFileTests(t *testing.T, dir, path string) { @@ -140,29 +142,36 @@ func configFileDefaultTests(t *testing.T) { // Test remaining default initializers t.Setenv("ZARF_CONFIG", filepath.Join("src", "test", "zarf-config-test.toml")) - defer os.Unsetenv("ZARF_CONFIG") // Test global flags - stdOut, _, _ := e2e.Zarf(t, "--help") + stdOut, _, err := e2e.Zarf(t, "--help") + require.NoError(t, err) for _, test := range globalFlags { - require.Contains(t, string(stdOut), test) + require.Contains(t, stdOut, test) } // Test init flags - stdOut, _, _ = e2e.Zarf(t, "init", "--help") + stdOut, _, err = e2e.Zarf(t, "init", "--help") + require.NoError(t, err) for _, test := range initFlags { - require.Contains(t, string(stdOut), test) + require.Contains(t, stdOut, test) } // Test package create flags - stdOut, _, _ = e2e.Zarf(t, "package", "create", "--help") + stdOut, _, err = e2e.Zarf(t, "package", "create", "--help") + require.NoError(t, err) for _, test := range packageCreateFlags { - require.Contains(t, string(stdOut), test) + require.Contains(t, stdOut, test) } // Test package deploy flags - stdOut, _, _ = e2e.Zarf(t, "package", "deploy", "--help") + stdOut, _, err = e2e.Zarf(t, "package", "deploy", "--help") + require.NoError(t, err) for _, test := range packageDeployFlags { - require.Contains(t, string(stdOut), test) + require.Contains(t, stdOut, test) } + + // Cleanup + err = os.Unsetenv("ZARF_CONFIG") + require.NoError(t, err) } diff --git a/src/test/external/common.go b/src/test/external/common.go index 0209ddadb2..dd5446002d 100644 --- a/src/test/external/common.go +++ b/src/test/external/common.go @@ -28,7 +28,8 @@ func createPodInfoPackageWithInsecureSources(t *testing.T, temp string) { require.NoError(t, err, "unable to yq edit helm source") err = exec.CmdWithPrint(zarfBinPath, "tools", "yq", "eval", ".spec.insecure = true", "-i", filepath.Join(temp, "oci", "podinfo-source.yaml")) require.NoError(t, err, "unable to yq edit oci source") - exec.CmdWithPrint(zarfBinPath, "package", "create", temp, "--confirm", "--output", temp) + err = exec.CmdWithPrint(zarfBinPath, "package", "create", temp, "--confirm", "--output", temp) + require.NoError(t, err, "unable to create package") } func verifyWaitSuccess(t *testing.T, timeoutMinutes time.Duration, cmd string, args []string, condition string, onTimeout string) bool { diff --git a/src/test/external/ext_in_cluster_test.go b/src/test/external/ext_in_cluster_test.go index ad1e338935..826b947268 100644 --- a/src/test/external/ext_in_cluster_test.go +++ b/src/test/external/ext_in_cluster_test.go @@ -157,7 +157,10 @@ func (suite *ExtInClusterTestSuite) Test_1_Deploy() { err := exec.CmdWithPrint(zarfBinPath, initArgs...) suite.NoError(err, "unable to initialize the k8s server with zarf") temp := suite.T().TempDir() - defer os.Remove(temp) + defer func(name string) { + err := os.Remove(name) + suite.NoError(err) + }(temp) createPodInfoPackageWithInsecureSources(suite.T(), temp) // Deploy the flux example package diff --git a/src/test/external/ext_out_cluster_test.go b/src/test/external/ext_out_cluster_test.go index 2cf6807d6d..297894ff28 100644 --- a/src/test/external/ext_out_cluster_test.go +++ b/src/test/external/ext_out_cluster_test.go @@ -58,13 +58,17 @@ func (suite *ExtOutClusterTestSuite) SetupSuite() { suite.Assertions = require.New(suite.T()) // Teardown any leftovers from previous tests - _ = exec.CmdWithPrint("k3d", "cluster", "delete", clusterName) - _ = exec.CmdWithPrint("docker", "rm", "-f", "k3d-"+registryHost) - _ = exec.CmdWithPrint("docker", "compose", "down") - _ = exec.CmdWithPrint("docker", "network", "remove", network) + err := exec.CmdWithPrint("k3d", "cluster", "delete", clusterName) + suite.NoError(err, "unable to teardown cluster") + err = exec.CmdWithPrint("docker", "rm", "-f", "k3d-"+registryHost) + suite.NoError(err, "unable to teardown k3d registry") + err = exec.CmdWithPrint("docker", "compose", "down") + suite.NoError(err, "unable to teardown docker compose resources") + err = exec.CmdWithPrint("docker", "network", "remove", network) + suite.NoError(err, "unable to teardown docker network resources") - // Setup a network for everything to live inside - err := exec.CmdWithPrint("docker", "network", "create", "--driver=bridge", "--subnet="+subnet, "--gateway="+gateway, network) + // Set up a network for everything to live inside + err = exec.CmdWithPrint("docker", "network", "create", "--driver=bridge", "--subnet="+subnet, "--gateway="+gateway, network) suite.NoError(err, "unable to create the k3d registry") // Install a k3d-managed registry server to act as the 'remote' container registry diff --git a/src/test/upgrade/previously_built_test.go b/src/test/upgrade/previously_built_test.go index 0b59761d6d..178a97ca5b 100644 --- a/src/test/upgrade/previously_built_test.go +++ b/src/test/upgrade/previously_built_test.go @@ -29,18 +29,24 @@ func TestPreviouslyBuiltZarfPackage(t *testing.T) { t.Log("Upgrade: Previously Built Zarf Package") // For the upgrade test, podinfo-upgrade should already be in the cluster (version 6.3.3) (see .github/workflows/test-upgrade.yml) - kubectlOut, _, _ := kubectl(t, "-n=podinfo-upgrade", "rollout", "status", "deployment/podinfo-upgrade") + kubectlOut, _, err := kubectl(t, "-n=podinfo-upgrade", "rollout", "status", "deployment/podinfo-upgrade") + require.NoError(t, err) require.Contains(t, kubectlOut, "successfully rolled out") - kubectlOut, _, _ = kubectl(t, "-n=podinfo-upgrade", "get", "deployment", "podinfo-upgrade", "-o=jsonpath={.metadata.labels}}") + kubectlOut, _, err = kubectl(t, "-n=podinfo-upgrade", "get", "deployment", "podinfo-upgrade", "-o=jsonpath={.metadata.labels}}") + require.NoError(t, err) require.Contains(t, kubectlOut, "6.3.3") // Verify that the private-registry secret and private-git-server secret in the podinfo-upgrade namespace are the same after re-init // This tests that `zarf tools update-creds` successfully updated the other namespace - zarfRegistrySecret, _, _ := kubectl(t, "-n=zarf", "get", "secret", "private-registry", "-o", "jsonpath={.data}") - podinfoRegistrySecret, _, _ := kubectl(t, "-n=podinfo-upgrade", "get", "secret", "private-registry", "-o", "jsonpath={.data}") + zarfRegistrySecret, _, err := kubectl(t, "-n=zarf", "get", "secret", "private-registry", "-o", "jsonpath={.data}") + require.NoError(t, err) + podinfoRegistrySecret, _, err := kubectl(t, "-n=podinfo-upgrade", "get", "secret", "private-registry", "-o", "jsonpath={.data}") + require.NoError(t, err) require.Equal(t, zarfRegistrySecret, podinfoRegistrySecret, "the zarf registry secret and podinfo-upgrade registry secret did not match") - zarfGitServerSecret, _, _ := kubectl(t, "-n=zarf", "get", "secret", "private-git-server", "-o", "jsonpath={.data}") - podinfoGitServerSecret, _, _ := kubectl(t, "-n=podinfo-upgrade", "get", "secret", "private-git-server", "-o", "jsonpath={.data}") + zarfGitServerSecret, _, err := kubectl(t, "-n=zarf", "get", "secret", "private-git-server", "-o", "jsonpath={.data}") + require.NoError(t, err) + podinfoGitServerSecret, _, err := kubectl(t, "-n=podinfo-upgrade", "get", "secret", "private-git-server", "-o", "jsonpath={.data}") + require.NoError(t, err) require.Equal(t, zarfGitServerSecret, podinfoGitServerSecret, "the zarf git server secret and podinfo-upgrade git server secret did not match") // We also expect a 6.3.4 package to have been previously built @@ -56,9 +62,11 @@ func TestPreviouslyBuiltZarfPackage(t *testing.T) { require.Contains(t, stdErr, "-----BEGIN PUBLIC KEY-----") // Verify that podinfo-upgrade successfully deploys in the cluster (version 6.3.4) - kubectlOut, _, _ = kubectl(t, "-n=podinfo-upgrade", "rollout", "status", "deployment/podinfo-upgrade") + kubectlOut, _, err = kubectl(t, "-n=podinfo-upgrade", "rollout", "status", "deployment/podinfo-upgrade") + require.NoError(t, err) require.Contains(t, kubectlOut, "successfully rolled out") - kubectlOut, _, _ = kubectl(t, "-n=podinfo-upgrade", "get", "deployment", "podinfo-upgrade", "-o=jsonpath={.metadata.labels}}") + kubectlOut, _, err = kubectl(t, "-n=podinfo-upgrade", "get", "deployment", "podinfo-upgrade", "-o=jsonpath={.metadata.labels}}") + require.NoError(t, err) require.Contains(t, kubectlOut, "6.3.4") // We also want to build a new package. @@ -75,9 +83,11 @@ func TestPreviouslyBuiltZarfPackage(t *testing.T) { require.Contains(t, stdErr, "-----BEGIN PUBLIC KEY-----") // Verify that podinfo-upgrade successfully deploys in the cluster (version 6.3.5) - kubectlOut, _, _ = kubectl(t, "-n=podinfo-upgrade", "rollout", "status", "deployment/podinfo-upgrade") + kubectlOut, _, err = kubectl(t, "-n=podinfo-upgrade", "rollout", "status", "deployment/podinfo-upgrade") + require.NoError(t, err) require.Contains(t, kubectlOut, "successfully rolled out") - kubectlOut, _, _ = kubectl(t, "-n=podinfo-upgrade", "get", "deployment", "podinfo-upgrade", "-o=jsonpath={.metadata.labels}}") + kubectlOut, _, err = kubectl(t, "-n=podinfo-upgrade", "get", "deployment", "podinfo-upgrade", "-o=jsonpath={.metadata.labels}}") + require.NoError(t, err) require.Contains(t, kubectlOut, "6.3.5") // Remove the package. From cd5fdc715c150e3dbb8ea68b84f98870bdd3c3d9 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 12:15:09 -0700 Subject: [PATCH 11/57] fix: use RemoveAll not remove so it doesnt error every time Signed-off-by: Kit Patella --- src/test/external/ext_out_cluster_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/external/ext_out_cluster_test.go b/src/test/external/ext_out_cluster_test.go index 297894ff28..f7f8a477cc 100644 --- a/src/test/external/ext_out_cluster_test.go +++ b/src/test/external/ext_out_cluster_test.go @@ -162,7 +162,7 @@ func (suite *ExtOutClusterTestSuite) Test_2_DeployGitOps() { deployArgs = []string{"package", "deploy", path, "--confirm"} err = exec.CmdWithPrint(zarfBinPath, deployArgs...) suite.NoError(err) - err = os.Remove(temp) + err = os.RemoveAll(temp) suite.NoError(err, "unable to remove tempdir") } From 905954497ca81cc8e1f3f61c5af4098576e94327 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 12:29:08 -0700 Subject: [PATCH 12/57] fix: disabling the err catch to see if it's something i changed Signed-off-by: Kit Patella --- src/test/external/ext_out_cluster_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/external/ext_out_cluster_test.go b/src/test/external/ext_out_cluster_test.go index f7f8a477cc..0c06816e8b 100644 --- a/src/test/external/ext_out_cluster_test.go +++ b/src/test/external/ext_out_cluster_test.go @@ -162,8 +162,8 @@ func (suite *ExtOutClusterTestSuite) Test_2_DeployGitOps() { deployArgs = []string{"package", "deploy", path, "--confirm"} err = exec.CmdWithPrint(zarfBinPath, deployArgs...) suite.NoError(err) - err = os.RemoveAll(temp) - suite.NoError(err, "unable to remove tempdir") + _ = os.RemoveAll(temp) //nolint:errcheck + // suite.NoError(err, "unable to remove tempdir") } func (suite *ExtOutClusterTestSuite) Test_3_AuthToPrivateHelmChart() { From 4a0d6efa98e85f7e8a7e382d882ed93988e22bdb Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 12:43:29 -0700 Subject: [PATCH 13/57] fix: should fix the directory not empty panic Signed-off-by: Kit Patella --- src/test/external/ext_in_cluster_test.go | 9 +++++---- src/test/external/ext_out_cluster_test.go | 5 +++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/test/external/ext_in_cluster_test.go b/src/test/external/ext_in_cluster_test.go index 826b947268..86fc6b5164 100644 --- a/src/test/external/ext_in_cluster_test.go +++ b/src/test/external/ext_in_cluster_test.go @@ -156,11 +156,9 @@ func (suite *ExtInClusterTestSuite) Test_1_Deploy() { initArgs = append(initArgs, inClusterInitCredentialArgs...) err := exec.CmdWithPrint(zarfBinPath, initArgs...) suite.NoError(err, "unable to initialize the k8s server with zarf") + + // Create tempdir temp := suite.T().TempDir() - defer func(name string) { - err := os.Remove(name) - suite.NoError(err) - }(temp) createPodInfoPackageWithInsecureSources(suite.T(), temp) // Deploy the flux example package @@ -201,8 +199,11 @@ func (suite *ExtInClusterTestSuite) Test_1_Deploy() { err = pkgkubernetes.WaitForReady(waitCtx, c.Watcher, objs) suite.NoError(err) + // Cleanup _, _, err = exec.CmdWithTesting(suite.T(), exec.PrintCfg(), zarfBinPath, "destroy", "--confirm") suite.NoError(err, "unable to teardown zarf") + err = os.RemoveAll(temp) + suite.NoError(err, "unable to remove tempdir") } func TestExtInClusterTestSuite(t *testing.T) { diff --git a/src/test/external/ext_out_cluster_test.go b/src/test/external/ext_out_cluster_test.go index 0c06816e8b..015e3feb2d 100644 --- a/src/test/external/ext_out_cluster_test.go +++ b/src/test/external/ext_out_cluster_test.go @@ -162,8 +162,9 @@ func (suite *ExtOutClusterTestSuite) Test_2_DeployGitOps() { deployArgs = []string{"package", "deploy", path, "--confirm"} err = exec.CmdWithPrint(zarfBinPath, deployArgs...) suite.NoError(err) - _ = os.RemoveAll(temp) //nolint:errcheck - // suite.NoError(err, "unable to remove tempdir") + + err = os.RemoveAll(temp) + suite.NoError(err, "unable to remove tempdir") } func (suite *ExtOutClusterTestSuite) Test_3_AuthToPrivateHelmChart() { From 4da92b9ba360fb33eedd8e0dc1f790e77415790b Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 13:06:06 -0700 Subject: [PATCH 14/57] fix: see if CI is flaking with the original tests. probably just need to nolint ignore them Signed-off-by: Kit Patella --- src/test/external/ext_in_cluster_test.go | 6 +-- src/test/external/ext_out_cluster_test.go | 47 +++++++---------------- 2 files changed, 15 insertions(+), 38 deletions(-) diff --git a/src/test/external/ext_in_cluster_test.go b/src/test/external/ext_in_cluster_test.go index 86fc6b5164..ad1e338935 100644 --- a/src/test/external/ext_in_cluster_test.go +++ b/src/test/external/ext_in_cluster_test.go @@ -156,9 +156,8 @@ func (suite *ExtInClusterTestSuite) Test_1_Deploy() { initArgs = append(initArgs, inClusterInitCredentialArgs...) err := exec.CmdWithPrint(zarfBinPath, initArgs...) suite.NoError(err, "unable to initialize the k8s server with zarf") - - // Create tempdir temp := suite.T().TempDir() + defer os.Remove(temp) createPodInfoPackageWithInsecureSources(suite.T(), temp) // Deploy the flux example package @@ -199,11 +198,8 @@ func (suite *ExtInClusterTestSuite) Test_1_Deploy() { err = pkgkubernetes.WaitForReady(waitCtx, c.Watcher, objs) suite.NoError(err) - // Cleanup _, _, err = exec.CmdWithTesting(suite.T(), exec.PrintCfg(), zarfBinPath, "destroy", "--confirm") suite.NoError(err, "unable to teardown zarf") - err = os.RemoveAll(temp) - suite.NoError(err, "unable to remove tempdir") } func TestExtInClusterTestSuite(t *testing.T) { diff --git a/src/test/external/ext_out_cluster_test.go b/src/test/external/ext_out_cluster_test.go index 015e3feb2d..e2cbaccefb 100644 --- a/src/test/external/ext_out_cluster_test.go +++ b/src/test/external/ext_out_cluster_test.go @@ -58,17 +58,13 @@ func (suite *ExtOutClusterTestSuite) SetupSuite() { suite.Assertions = require.New(suite.T()) // Teardown any leftovers from previous tests - err := exec.CmdWithPrint("k3d", "cluster", "delete", clusterName) - suite.NoError(err, "unable to teardown cluster") - err = exec.CmdWithPrint("docker", "rm", "-f", "k3d-"+registryHost) - suite.NoError(err, "unable to teardown k3d registry") - err = exec.CmdWithPrint("docker", "compose", "down") - suite.NoError(err, "unable to teardown docker compose resources") - err = exec.CmdWithPrint("docker", "network", "remove", network) - suite.NoError(err, "unable to teardown docker network resources") + _ = exec.CmdWithPrint("k3d", "cluster", "delete", clusterName) + _ = exec.CmdWithPrint("docker", "rm", "-f", "k3d-"+registryHost) + _ = exec.CmdWithPrint("docker", "compose", "down") + _ = exec.CmdWithPrint("docker", "network", "remove", network) - // Set up a network for everything to live inside - err = exec.CmdWithPrint("docker", "network", "create", "--driver=bridge", "--subnet="+subnet, "--gateway="+gateway, network) + // Setup a network for everything to live inside + err := exec.CmdWithPrint("docker", "network", "create", "--driver=bridge", "--subnet="+subnet, "--gateway="+gateway, network) suite.NoError(err, "unable to create the k3d registry") // Install a k3d-managed registry server to act as the 'remote' container registry @@ -150,8 +146,8 @@ func (suite *ExtOutClusterTestSuite) Test_1_Deploy() { func (suite *ExtOutClusterTestSuite) Test_2_DeployGitOps() { // Deploy the flux example package - // Cleanup dir at end of test temp := suite.T().TempDir() + defer os.Remove(temp) createPodInfoPackageWithInsecureSources(suite.T(), temp) deployArgs := []string{"package", "deploy", filepath.Join(temp, "zarf-package-podinfo-flux-amd64.tar.zst"), "--confirm"} @@ -162,9 +158,6 @@ func (suite *ExtOutClusterTestSuite) Test_2_DeployGitOps() { deployArgs = []string{"package", "deploy", path, "--confirm"} err = exec.CmdWithPrint(zarfBinPath, deployArgs...) suite.NoError(err) - - err = os.RemoveAll(temp) - suite.NoError(err, "unable to remove tempdir") } func (suite *ExtOutClusterTestSuite) Test_3_AuthToPrivateHelmChart() { @@ -175,13 +168,12 @@ func (suite *ExtOutClusterTestSuite) Test_3_AuthToPrivateHelmChart() { tempDir := suite.T().TempDir() repoPath := filepath.Join(tempDir, "repositories.yaml") - // Cleanup at end of func - err := os.Setenv("HELM_REPOSITORY_CONFIG", repoPath) - suite.Error(err, "Unable to set HELM_REPOSITORY_CONFIG") + os.Setenv("HELM_REPOSITORY_CONFIG", repoPath) + defer os.Unsetenv("HELM_REPOSITORY_CONFIG") packagePath := filepath.Join("..", "packages", "external-helm-auth") findImageArgs := []string{"dev", "find-images", packagePath} - err = exec.CmdWithPrint(zarfBinPath, findImageArgs...) + err := exec.CmdWithPrint(zarfBinPath, findImageArgs...) suite.Error(err, "Since auth has not been setup, this should fail") repoFile := repo.NewFile() @@ -203,9 +195,6 @@ func (suite *ExtOutClusterTestSuite) Test_3_AuthToPrivateHelmChart() { packageCreateArgs := []string{"package", "create", packagePath, fmt.Sprintf("--output=%s", tempDir), "--confirm"} err = exec.CmdWithPrint(zarfBinPath, packageCreateArgs...) suite.NoError(err, "Unable to create package, helm auth likely failed") - - err = os.Unsetenv("HELM_REPOSITORY_CONFIG") - suite.Error(err, "Unable to Unset HELM_REPOSITORY_CONFIG") } func (suite *ExtOutClusterTestSuite) createHelmChartInGitea(baseURL string, username string, password string) { @@ -222,9 +211,9 @@ func (suite *ExtOutClusterTestSuite) createHelmChartInGitea(baseURL string, user suite.NoError(err) url := fmt.Sprintf("%s/api/packages/%s/helm/api/charts", baseURL, username) - // Close the file directly at the end of the test file, err := os.Open(podinfoTarballPath) suite.NoError(err) + defer file.Close() body := &bytes.Buffer{} writer := multipart.NewWriter(body) @@ -232,8 +221,7 @@ func (suite *ExtOutClusterTestSuite) createHelmChartInGitea(baseURL string, user suite.NoError(err) _, err = io.Copy(part, file) suite.NoError(err) - err = writer.Close() - suite.NoError(err) + writer.Close() req, err := http.NewRequest("POST", url, body) suite.NoError(err) @@ -245,11 +233,7 @@ func (suite *ExtOutClusterTestSuite) createHelmChartInGitea(baseURL string, user resp, err := client.Do(req) suite.NoError(err) - err = resp.Body.Close() - suite.NoError(err) - - err = file.Close() - suite.NoError(err) + resp.Body.Close() } func (suite *ExtOutClusterTestSuite) makeGiteaUserPrivate(baseURL string, username string, password string) { @@ -272,15 +256,12 @@ func (suite *ExtOutClusterTestSuite) makeGiteaUserPrivate(baseURL string, userna req.SetBasicAuth(username, password) client := &http.Client{} - // Close resp body at end of test resp, err := client.Do(req) suite.NoError(err) + defer resp.Body.Close() _, err = io.ReadAll(resp.Body) suite.NoError(err) - - err = resp.Body.Close() - suite.NoError(err, "Unable to close response body") } func TestExtOurClusterTestSuite(t *testing.T) { From 29c5d082b450e7a4929d362dff43b077d1b865d1 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 13:21:39 -0700 Subject: [PATCH 15/57] fix: these tests should clean up properly and not error Signed-off-by: Kit Patella --- src/test/external/ext_in_cluster_test.go | 6 ++++-- src/test/external/ext_out_cluster_test.go | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/test/external/ext_in_cluster_test.go b/src/test/external/ext_in_cluster_test.go index ad1e338935..89f6b24fe8 100644 --- a/src/test/external/ext_in_cluster_test.go +++ b/src/test/external/ext_in_cluster_test.go @@ -157,8 +157,7 @@ func (suite *ExtInClusterTestSuite) Test_1_Deploy() { err := exec.CmdWithPrint(zarfBinPath, initArgs...) suite.NoError(err, "unable to initialize the k8s server with zarf") temp := suite.T().TempDir() - defer os.Remove(temp) - createPodInfoPackageWithInsecureSources(suite.T(), temp) + defer createPodInfoPackageWithInsecureSources(suite.T(), temp) // Deploy the flux example package deployArgs := []string{"package", "deploy", filepath.Join(temp, "zarf-package-podinfo-flux-amd64.tar.zst"), "--confirm"} @@ -200,6 +199,9 @@ func (suite *ExtInClusterTestSuite) Test_1_Deploy() { _, _, err = exec.CmdWithTesting(suite.T(), exec.PrintCfg(), zarfBinPath, "destroy", "--confirm") suite.NoError(err, "unable to teardown zarf") + + err = os.RemoveAll(temp) + suite.NoError(err, "unable to remove temp directory") } func TestExtInClusterTestSuite(t *testing.T) { diff --git a/src/test/external/ext_out_cluster_test.go b/src/test/external/ext_out_cluster_test.go index e2cbaccefb..e98fe7b15b 100644 --- a/src/test/external/ext_out_cluster_test.go +++ b/src/test/external/ext_out_cluster_test.go @@ -147,7 +147,6 @@ func (suite *ExtOutClusterTestSuite) Test_1_Deploy() { func (suite *ExtOutClusterTestSuite) Test_2_DeployGitOps() { // Deploy the flux example package temp := suite.T().TempDir() - defer os.Remove(temp) createPodInfoPackageWithInsecureSources(suite.T(), temp) deployArgs := []string{"package", "deploy", filepath.Join(temp, "zarf-package-podinfo-flux-amd64.tar.zst"), "--confirm"} @@ -158,6 +157,10 @@ func (suite *ExtOutClusterTestSuite) Test_2_DeployGitOps() { deployArgs = []string{"package", "deploy", path, "--confirm"} err = exec.CmdWithPrint(zarfBinPath, deployArgs...) suite.NoError(err) + + // Clean up tmpdir + err = os.RemoveAll(temp) + suite.NoError(err, "unable to remove temporary directory") } func (suite *ExtOutClusterTestSuite) Test_3_AuthToPrivateHelmChart() { From dfdf78a495ac70f7b07150c9039e8d937e223c2d Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 13:40:16 -0700 Subject: [PATCH 16/57] fix: actually cleaning up the test with RemovalAll seems to cause a panic Signed-off-by: Kit Patella --- src/test/external/ext_in_cluster_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/test/external/ext_in_cluster_test.go b/src/test/external/ext_in_cluster_test.go index 89f6b24fe8..57f7588dac 100644 --- a/src/test/external/ext_in_cluster_test.go +++ b/src/test/external/ext_in_cluster_test.go @@ -157,7 +157,7 @@ func (suite *ExtInClusterTestSuite) Test_1_Deploy() { err := exec.CmdWithPrint(zarfBinPath, initArgs...) suite.NoError(err, "unable to initialize the k8s server with zarf") temp := suite.T().TempDir() - defer createPodInfoPackageWithInsecureSources(suite.T(), temp) + createPodInfoPackageWithInsecureSources(suite.T(), temp) // Deploy the flux example package deployArgs := []string{"package", "deploy", filepath.Join(temp, "zarf-package-podinfo-flux-amd64.tar.zst"), "--confirm"} @@ -200,8 +200,10 @@ func (suite *ExtInClusterTestSuite) Test_1_Deploy() { _, _, err = exec.CmdWithTesting(suite.T(), exec.PrintCfg(), zarfBinPath, "destroy", "--confirm") suite.NoError(err, "unable to teardown zarf") - err = os.RemoveAll(temp) - suite.NoError(err, "unable to remove temp directory") + // FIXME(mkcp): This code always fails to clean up the tmpdir. We're passing it a directory with contents and we + // need os.RemoveAll(temp) instead. However, we're also getting what looks to be a bug with concurrent writers + // causing a subtest to panic when we call RemoveAll(). Fixing this would require removing the shared state. + _ = os.Remove(temp) // nolint:errcheck } func TestExtInClusterTestSuite(t *testing.T) { From 5a4241efe3f972cc18c4a9aef7d9e1f08ec15b6c Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 14:00:26 -0700 Subject: [PATCH 17/57] fix: reintroduce the ext_in_cluster test error catches Signed-off-by: Kit Patella --- src/test/external/ext_out_cluster_test.go | 37 +++++++++++++++-------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/src/test/external/ext_out_cluster_test.go b/src/test/external/ext_out_cluster_test.go index e98fe7b15b..9b429d442c 100644 --- a/src/test/external/ext_out_cluster_test.go +++ b/src/test/external/ext_out_cluster_test.go @@ -58,10 +58,12 @@ func (suite *ExtOutClusterTestSuite) SetupSuite() { suite.Assertions = require.New(suite.T()) // Teardown any leftovers from previous tests - _ = exec.CmdWithPrint("k3d", "cluster", "delete", clusterName) - _ = exec.CmdWithPrint("docker", "rm", "-f", "k3d-"+registryHost) - _ = exec.CmdWithPrint("docker", "compose", "down") - _ = exec.CmdWithPrint("docker", "network", "remove", network) + // NOTE(mkcp): We dogsled these errors because some of these commands will error if they don't cleanup a resource, + // which is ok. A better solution would be checking for none or unexpected kinds of errors. + _ = exec.CmdWithPrint("k3d", "cluster", "delete", clusterName) //nolint:errcheck + _ = exec.CmdWithPrint("docker", "rm", "-f", "k3d-"+registryHost) //nolint:errcheck + _ = exec.CmdWithPrint("docker", "compose", "down") //nolint:errcheck + _ = exec.CmdWithPrint("docker", "network", "remove", network) //nolint:errcheck // Setup a network for everything to live inside err := exec.CmdWithPrint("docker", "network", "create", "--driver=bridge", "--subnet="+subnet, "--gateway="+gateway, network) @@ -171,12 +173,12 @@ func (suite *ExtOutClusterTestSuite) Test_3_AuthToPrivateHelmChart() { tempDir := suite.T().TempDir() repoPath := filepath.Join(tempDir, "repositories.yaml") - os.Setenv("HELM_REPOSITORY_CONFIG", repoPath) - defer os.Unsetenv("HELM_REPOSITORY_CONFIG") + err := os.Setenv("HELM_REPOSITORY_CONFIG", repoPath) + suite.NoError(err, "unable to set HELM_REPOSITORY_CONFIG") packagePath := filepath.Join("..", "packages", "external-helm-auth") findImageArgs := []string{"dev", "find-images", packagePath} - err := exec.CmdWithPrint(zarfBinPath, findImageArgs...) + err = exec.CmdWithPrint(zarfBinPath, findImageArgs...) suite.Error(err, "Since auth has not been setup, this should fail") repoFile := repo.NewFile() @@ -198,6 +200,10 @@ func (suite *ExtOutClusterTestSuite) Test_3_AuthToPrivateHelmChart() { packageCreateArgs := []string{"package", "create", packagePath, fmt.Sprintf("--output=%s", tempDir), "--confirm"} err = exec.CmdWithPrint(zarfBinPath, packageCreateArgs...) suite.NoError(err, "Unable to create package, helm auth likely failed") + + // Cleanup env var + err = os.Unsetenv("HELM_REPOSITORY_CONFIG") + suite.NoError(err, "unable to unset HELM_REPOSITORY_CONFIG") } func (suite *ExtOutClusterTestSuite) createHelmChartInGitea(baseURL string, username string, password string) { @@ -216,7 +222,6 @@ func (suite *ExtOutClusterTestSuite) createHelmChartInGitea(baseURL string, user file, err := os.Open(podinfoTarballPath) suite.NoError(err) - defer file.Close() body := &bytes.Buffer{} writer := multipart.NewWriter(body) @@ -224,7 +229,12 @@ func (suite *ExtOutClusterTestSuite) createHelmChartInGitea(baseURL string, user suite.NoError(err) _, err = io.Copy(part, file) suite.NoError(err) - writer.Close() + + // Cleanup file and writer + err = file.Close() + suite.NoError(err, "unable to close file") + err = writer.Close() + suite.NoError(err, "unable to close writer") req, err := http.NewRequest("POST", url, body) suite.NoError(err) @@ -236,7 +246,8 @@ func (suite *ExtOutClusterTestSuite) createHelmChartInGitea(baseURL string, user resp, err := client.Do(req) suite.NoError(err) - resp.Body.Close() + err = resp.Body.Close() + suite.NoError(err, "unable to close response body") } func (suite *ExtOutClusterTestSuite) makeGiteaUserPrivate(baseURL string, username string, password string) { @@ -261,10 +272,12 @@ func (suite *ExtOutClusterTestSuite) makeGiteaUserPrivate(baseURL string, userna client := &http.Client{} resp, err := client.Do(req) suite.NoError(err) - defer resp.Body.Close() - _, err = io.ReadAll(resp.Body) suite.NoError(err) + + // Cleanup + err = resp.Body.Close() + suite.NoError(err, "unable to close response body") } func TestExtOurClusterTestSuite(t *testing.T) { From ac3db1748fcaaceccb784d0d56f6e01a0dde9e9b Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 14:01:11 -0700 Subject: [PATCH 18/57] fix: missed a spot Signed-off-by: Kit Patella --- src/test/external/ext_in_cluster_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/external/ext_in_cluster_test.go b/src/test/external/ext_in_cluster_test.go index 57f7588dac..91b257651c 100644 --- a/src/test/external/ext_in_cluster_test.go +++ b/src/test/external/ext_in_cluster_test.go @@ -203,7 +203,7 @@ func (suite *ExtInClusterTestSuite) Test_1_Deploy() { // FIXME(mkcp): This code always fails to clean up the tmpdir. We're passing it a directory with contents and we // need os.RemoveAll(temp) instead. However, we're also getting what looks to be a bug with concurrent writers // causing a subtest to panic when we call RemoveAll(). Fixing this would require removing the shared state. - _ = os.Remove(temp) // nolint:errcheck + _ = os.Remove(temp) //nolint:errcheck } func TestExtInClusterTestSuite(t *testing.T) { From 3b274f7128d852e8a7d23964199b1b3ae278653d Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 14:16:37 -0700 Subject: [PATCH 19/57] fix: e2e 24 - can we assert that this error exists or do we have to ignore it Signed-off-by: Kit Patella --- src/test/e2e/24_variables_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/e2e/24_variables_test.go b/src/test/e2e/24_variables_test.go index 72490c3ebb..aa16771d53 100644 --- a/src/test/e2e/24_variables_test.go +++ b/src/test/e2e/24_variables_test.go @@ -44,8 +44,8 @@ func TestVariables(t *testing.T) { // Test that not specifying a prompted variable results in an error _, stdErr, err = e2e.Zarf(t, "package", "deploy", path, "--confirm") - require.NoError(t, err) expectedOutString = "variable 'SITE_NAME' must be '--set' when using the '--confirm' flag" + require.Error(t, err) require.Contains(t, stdErr, "", expectedOutString) // Test that specifying an invalid variable value results in an error From a9d6266a9224475d614adf8632e58cbe903fd2c9 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 14:54:53 -0700 Subject: [PATCH 20/57] fix: address some fixmes before merge Signed-off-by: Kit Patella --- src/cmd/destroy.go | 2 +- src/cmd/initialize.go | 6 +++--- src/pkg/utils/cosign.go | 3 +-- src/test/external/ext_in_cluster_test.go | 9 ++++++--- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/cmd/destroy.go b/src/cmd/destroy.go index 427292bd5e..3c051b9a5e 100644 --- a/src/cmd/destroy.go +++ b/src/cmd/destroy.go @@ -31,7 +31,7 @@ var destroyCmd = &cobra.Command{ Aliases: []string{"d"}, Short: lang.CmdDestroyShort, Long: lang.CmdDestroyLong, - // FIXME(mkcp): This function deeply needs a refactor and un-nesting. + // TODO(mkcp): refactor and de-nest this function. RunE: func(cmd *cobra.Command, _ []string) error { ctx := cmd.Context() timeoutCtx, cancel := context.WithTimeout(cmd.Context(), cluster.DefaultTimeout) diff --git a/src/cmd/initialize.go b/src/cmd/initialize.go index 9dd680c259..3b9f10c1f6 100644 --- a/src/cmd/initialize.go +++ b/src/cmd/initialize.go @@ -99,10 +99,10 @@ func findInitPackage(ctx context.Context, initPackageName string) (string, error return "", err } // Verify that we can write to the path - // FIXME(mkcp): Decompose this into a helper function if helpers.InvalidPath(absCachePath) { // Create the directory if the path is invalid - if err := helpers.CreateDirectory(absCachePath, helpers.ReadExecuteAllWriteUser); err != nil { + err = helpers.CreateDirectory(absCachePath, helpers.ReadExecuteAllWriteUser) + if err != nil { return "", fmt.Errorf("unable to create the cache directory %s: %w", absCachePath, err) } } @@ -138,7 +138,7 @@ func downloadInitPackage(ctx context.Context, cacheDirectory string) (string, er message.Note(lang.CmdInitPullNote) // Prompt the user if --confirm not specified - // FIXME(mkcp): This condition can never be met + // REVIEW(mkcp): It looks like this condition can't be met - maybe --confirm was removed at some point? if !confirmDownload { prompt := &survey.Confirm{ Message: lang.CmdInitPullConfirm, diff --git a/src/pkg/utils/cosign.go b/src/pkg/utils/cosign.go index f8c82bc69c..0ecc1c8986 100644 --- a/src/pkg/utils/cosign.go +++ b/src/pkg/utils/cosign.go @@ -231,8 +231,7 @@ func GetCosignArtifacts(image string) ([]string, error) { return nil, err } - // FIXME(mkcp): Re-review how this function is used and whether a signed entity err-miss is something we should pass - // up to the caller. Needs a comment if nothing else. + // Return empty if we don't have a signature on the image var remoteOpts []ociremote.Option simg, _ := ociremote.SignedEntity(ref, remoteOpts...) if simg == nil { diff --git a/src/test/external/ext_in_cluster_test.go b/src/test/external/ext_in_cluster_test.go index 91b257651c..99984634d8 100644 --- a/src/test/external/ext_in_cluster_test.go +++ b/src/test/external/ext_in_cluster_test.go @@ -201,9 +201,12 @@ func (suite *ExtInClusterTestSuite) Test_1_Deploy() { suite.NoError(err, "unable to teardown zarf") // FIXME(mkcp): This code always fails to clean up the tmpdir. We're passing it a directory with contents and we - // need os.RemoveAll(temp) instead. However, we're also getting what looks to be a bug with concurrent writers - // causing a subtest to panic when we call RemoveAll(). Fixing this would require removing the shared state. - _ = os.Remove(temp) //nolint:errcheck + // need os.RemoveAll(temp) instead. However, we're also getting what looks to be a bug with concurrent + // writers causing a subtest to panic when we call RemoveAll(). Fixing this would require removing the + // shared state. + // REVIEW(mkcp): Thinking we delete this and + err = os.RemoveAll(temp) //nolint:errcheck + suite.NoError(err, "failed to clean up tempdir") } func TestExtInClusterTestSuite(t *testing.T) { From ccabf6c84cbac3d2a75c9c1c970e9ed34b55aab5 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 15:19:03 -0700 Subject: [PATCH 21/57] fix: does this work now? Signed-off-by: Kit Patella --- src/test/external/ext_in_cluster_test.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/test/external/ext_in_cluster_test.go b/src/test/external/ext_in_cluster_test.go index 99984634d8..709846d1d0 100644 --- a/src/test/external/ext_in_cluster_test.go +++ b/src/test/external/ext_in_cluster_test.go @@ -200,12 +200,8 @@ func (suite *ExtInClusterTestSuite) Test_1_Deploy() { _, _, err = exec.CmdWithTesting(suite.T(), exec.PrintCfg(), zarfBinPath, "destroy", "--confirm") suite.NoError(err, "unable to teardown zarf") - // FIXME(mkcp): This code always fails to clean up the tmpdir. We're passing it a directory with contents and we - // need os.RemoveAll(temp) instead. However, we're also getting what looks to be a bug with concurrent - // writers causing a subtest to panic when we call RemoveAll(). Fixing this would require removing the - // shared state. - // REVIEW(mkcp): Thinking we delete this and - err = os.RemoveAll(temp) //nolint:errcheck + // Cleanup tmpdir + err = os.RemoveAll(temp) suite.NoError(err, "failed to clean up tempdir") } From 8a56cd52bd2504017a5482cef9d1db6d44461747 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 16:24:18 -0700 Subject: [PATCH 22/57] fix: handle and propagate errors in pkg/utils Signed-off-by: Kit Patella --- src/pkg/utils/auth.go | 18 +++++++++++++++--- src/pkg/utils/cosign.go | 2 +- src/pkg/utils/exec/exec.go | 10 ++++++++-- src/pkg/utils/network.go | 16 +++++++++++++--- src/pkg/utils/wait.go | 6 +++++- src/pkg/utils/yaml.go | 8 ++++++-- 6 files changed, 48 insertions(+), 12 deletions(-) diff --git a/src/pkg/utils/auth.go b/src/pkg/utils/auth.go index 0bc5cf501a..93ca4f5f2d 100644 --- a/src/pkg/utils/auth.go +++ b/src/pkg/utils/auth.go @@ -23,7 +23,10 @@ type Credential struct { // FindAuthForHost finds the authentication scheme for a given host using .git-credentials then .netrc. func FindAuthForHost(baseURL string) (*Credential, error) { - homePath, _ := os.UserHomeDir() + homePath, err := os.UserHomeDir() + if err != nil { + return nil, err + } // Read the ~/.git-credentials file credentialsPath := filepath.Join(homePath, ".git-credentials") @@ -60,7 +63,6 @@ func credentialParser(path string) ([]Credential, error) { if err != nil { return nil, err } - defer file.Close() var credentials []Credential scanner := bufio.NewScanner(file) @@ -79,6 +81,10 @@ func credentialParser(path string) ([]Credential, error) { } credentials = append(credentials, credential) } + err = file.Close() + if err != nil { + return nil, err + } return credentials, nil } @@ -91,7 +97,6 @@ func netrcParser(path string) ([]Credential, error) { if err != nil { return nil, err } - defer file.Close() var credentials []Credential scanner := bufio.NewScanner(file) @@ -151,6 +156,13 @@ func netrcParser(path string) ([]Credential, error) { } } } + + // Close our file and handle any errors now that we're done scanning + err = file.Close() + if err != nil { + return nil, err + } + // Append the last machine (if exists) at the end of the file if activeMachine != nil { credentials = appendNetrcMachine(activeMachine, credentials) diff --git a/src/pkg/utils/cosign.go b/src/pkg/utils/cosign.go index 0ecc1c8986..a06beae49f 100644 --- a/src/pkg/utils/cosign.go +++ b/src/pkg/utils/cosign.go @@ -233,7 +233,7 @@ func GetCosignArtifacts(image string) ([]string, error) { // Return empty if we don't have a signature on the image var remoteOpts []ociremote.Option - simg, _ := ociremote.SignedEntity(ref, remoteOpts...) + simg, _ := ociremote.SignedEntity(ref, remoteOpts...) //nolint:errcheck if simg == nil { return nil, nil } diff --git a/src/pkg/utils/exec/exec.go b/src/pkg/utils/exec/exec.go index 3f76e3f23c..f9ac5bbed4 100644 --- a/src/pkg/utils/exec/exec.go +++ b/src/pkg/utils/exec/exec.go @@ -65,8 +65,14 @@ func CmdWithContext(ctx context.Context, config Config, command string, args ... cmd.Env = append(os.Environ(), config.Env...) // Capture the command outputs. - cmdStdout, _ := cmd.StdoutPipe() - cmdStderr, _ := cmd.StderrPipe() + cmdStdout, err := cmd.StdoutPipe() + if err != nil { + return "", "", fmt.Errorf("failed to capture stdout, error=%w", err) + } + cmdStderr, err := cmd.StderrPipe() + if err != nil { + return "", "", fmt.Errorf("failed to capture stderr, error=%w", err) + } var ( stdoutBuf, stderrBuf bytes.Buffer diff --git a/src/pkg/utils/network.go b/src/pkg/utils/network.go index ffe5490600..99f7e026ae 100644 --- a/src/pkg/utils/network.go +++ b/src/pkg/utils/network.go @@ -6,6 +6,7 @@ package utils import ( "context" + "errors" "fmt" "io" "net/http" @@ -39,7 +40,7 @@ func parseChecksum(src string) (string, string, error) { } // DownloadToFile downloads a given URL to the target filepath (including the cosign key if necessary). -func DownloadToFile(ctx context.Context, src, dst, cosignKeyPath string) error { +func DownloadToFile(ctx context.Context, src, dst, cosignKeyPath string) (err error) { // check if the parsed URL has a checksum // if so, remove it and use the checksum to validate the file src, checksum, err := parseChecksum(src) @@ -57,7 +58,11 @@ func DownloadToFile(ctx context.Context, src, dst, cosignKeyPath string) error { if err != nil { return fmt.Errorf(lang.ErrWritingFile, dst, err.Error()) } - defer file.Close() + // Ensure our file closes and any error propagate out on error branches + defer func(file *os.File) { + err2 := file.Close() + err = errors.Join(err, err2) + }(file) parsed, err := url.Parse(src) if err != nil { @@ -96,7 +101,6 @@ func httpGetFile(url string, destinationFile *os.File) error { if err != nil { return fmt.Errorf("unable to download the file %s", url) } - defer resp.Body.Close() // Check server response if resp.StatusCode != http.StatusOK { @@ -112,6 +116,12 @@ func httpGetFile(url string, destinationFile *os.File) error { return err } + // Close resp body now that we're done reading from it + err = resp.Body.Close() + if err != nil { + return err + } + title = fmt.Sprintf("Downloaded %s", url) progressBar.Successf("%s", title) return nil diff --git a/src/pkg/utils/wait.go b/src/pkg/utils/wait.go index dbcbbf0267..8fecaf8e10 100644 --- a/src/pkg/utils/wait.go +++ b/src/pkg/utils/wait.go @@ -197,7 +197,11 @@ func waitForNetworkEndpoint(resource, name, condition string, timeout time.Durat message.Debug(err) continue } - defer conn.Close() + err = conn.Close() + if err != nil { + message.Debug(err) + continue + } } // Yay, we made it! diff --git a/src/pkg/utils/yaml.go b/src/pkg/utils/yaml.go index 641c219977..e0b8a3e953 100644 --- a/src/pkg/utils/yaml.go +++ b/src/pkg/utils/yaml.go @@ -36,8 +36,11 @@ func yamlFormat(attr color.Attribute) string { } // ColorPrintYAML pretty prints a yaml file to the console. -func ColorPrintYAML(data any, hints map[string]string, spaceRootLists bool) { - text, _ := goyaml.Marshal(data) +func ColorPrintYAML(data any, hints map[string]string, spaceRootLists bool) error { + text, err := goyaml.Marshal(data) + if err != nil { + return err + } tokens := lexer.Tokenize(string(text)) if spaceRootLists { @@ -102,6 +105,7 @@ func ColorPrintYAML(data any, hints map[string]string, spaceRootLists bool) { pterm.Println() pterm.Println(outputYAML) + return nil } // AddRootListHint adds a hint string for a given root list key and value. From 5addcccba3d2505043b919ade77e7345f4c50323 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 16:24:52 -0700 Subject: [PATCH 23/57] fix: propagate any errors in ColorPrintYAML Signed-off-by: Kit Patella --- src/cmd/package.go | 5 ++++- src/pkg/interactive/components.go | 7 +++++-- src/pkg/packager/inspect.go | 5 ++++- src/pkg/packager/interactive.go | 6 +++++- src/pkg/packager/publish.go | 5 ++++- 5 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/cmd/package.go b/src/cmd/package.go index e55b449693..8fbb3b1dd0 100644 --- a/src/cmd/package.go +++ b/src/cmd/package.go @@ -219,7 +219,10 @@ var packageInspectCmd = &cobra.Command{ if err != nil { return fmt.Errorf("failed to inspect package: %w", err) } - utils.ColorPrintYAML(output, nil, false) + err = utils.ColorPrintYAML(output, nil, false) + if err != nil { + return err + } return nil }, } diff --git a/src/pkg/interactive/components.go b/src/pkg/interactive/components.go index b742aeed4c..9b5cee436c 100644 --- a/src/pkg/interactive/components.go +++ b/src/pkg/interactive/components.go @@ -20,7 +20,10 @@ func SelectOptionalComponent(component v1alpha1.ZarfComponent) (bool, error) { displayComponent := component displayComponent.Description = "" - utils.ColorPrintYAML(displayComponent, nil, false) + err := utils.ColorPrintYAML(displayComponent, nil, false) + if err != nil { + return false, err + } if component.Description != "" { message.Question(component.Description) } @@ -31,7 +34,7 @@ func SelectOptionalComponent(component v1alpha1.ZarfComponent) (bool, error) { } var confirm bool - err := survey.AskOne(prompt, &confirm) + err = survey.AskOne(prompt, &confirm) if err != nil { return false, err } diff --git a/src/pkg/packager/inspect.go b/src/pkg/packager/inspect.go index ae850498b8..03dee54883 100644 --- a/src/pkg/packager/inspect.go +++ b/src/pkg/packager/inspect.go @@ -34,7 +34,10 @@ func (p *Packager) Inspect(ctx context.Context) error { fmt.Fprintln(os.Stdout, "-", image) } } else { - utils.ColorPrintYAML(p.cfg.Pkg, nil, false) + err := utils.ColorPrintYAML(p.cfg.Pkg, nil, false) + if err != nil { + return err + } } sbomDir := p.layout.SBOMs.Path diff --git a/src/pkg/packager/interactive.go b/src/pkg/packager/interactive.go index bd44342e7f..45e3ee8fa5 100644 --- a/src/pkg/packager/interactive.go +++ b/src/pkg/packager/interactive.go @@ -6,6 +6,7 @@ package packager import ( "fmt" + "log/slog" "os" "path/filepath" @@ -21,7 +22,10 @@ import ( func (p *Packager) confirmAction(stage string, warnings []string, sbomViewFiles []string) bool { pterm.Println() message.HeaderInfof("📦 PACKAGE DEFINITION") - utils.ColorPrintYAML(p.cfg.Pkg, p.getPackageYAMLHints(stage), true) + err := utils.ColorPrintYAML(p.cfg.Pkg, p.getPackageYAMLHints(stage), true) + if err != nil { + slog.Error("unable to print yaml", "error", err) + } // Print any potential breaking changes (if this is a Deploy confirm) between this CLI version and the deployed init package if stage == config.ZarfDeployStage { diff --git a/src/pkg/packager/publish.go b/src/pkg/packager/publish.go index 94fdee63c2..6e421efd1f 100644 --- a/src/pkg/packager/publish.go +++ b/src/pkg/packager/publish.go @@ -117,7 +117,10 @@ func (p *Packager) Publish(ctx context.Context) (err error) { }, }) } - utils.ColorPrintYAML(ex, nil, true) + err := utils.ColorPrintYAML(ex, nil, true) + if err != nil { + return err + } } return nil } From 7c04dff19e75590fb650875756ba2f43bcf688c0 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 16:38:43 -0700 Subject: [PATCH 24/57] fix: propagate any errors in src/pkg/message Signed-off-by: Kit Patella --- src/pkg/message/progress.go | 17 ++++++++++++++--- src/pkg/message/spinner.go | 12 ++++++++++-- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/pkg/message/progress.go b/src/pkg/message/progress.go index 7c050b0ba2..c5e1a5c4db 100644 --- a/src/pkg/message/progress.go +++ b/src/pkg/message/progress.go @@ -6,6 +6,7 @@ package message import ( "fmt" + "log/slog" "os" "github.com/pterm/pterm" @@ -22,10 +23,11 @@ type ProgressBar struct { // NewProgressBar creates a new ProgressBar instance from a total value and a format. func NewProgressBar(total int64, text string) *ProgressBar { var progress *pterm.ProgressbarPrinter + var err error if NoProgress { Info(text) } else { - progress, _ = pterm.DefaultProgressbar. + progress, err = pterm.DefaultProgressbar. WithTotal(int(total)). WithShowCount(false). WithTitle(padding + text). @@ -33,6 +35,9 @@ func NewProgressBar(total int64, text string) *ProgressBar { WithMaxWidth(TermWidth). WithWriter(os.Stderr). Start() + if err != nil { + slog.Debug("Unable to create default progressbar", "error", err) + } } return &ProgressBar{ @@ -53,7 +58,10 @@ func (p *ProgressBar) Updatef(format string, a ...any) { // Failf marks the ProgressBar as failed in the CLI. func (p *ProgressBar) Failf(format string, a ...any) { - p.Close() + err := p.Close() + if err != nil { + slog.Debug("unable to close failed progressbar", "error", err) + } Warnf(format, a...) } @@ -103,7 +111,10 @@ func (p *ProgressBar) Write(data []byte) (int, error) { // Successf marks the ProgressBar as successful in the CLI. func (p *ProgressBar) Successf(format string, a ...any) { - p.Close() + err := p.Close() + if err != nil { + slog.Debug("unable to close successful progressbar", "error", err) + } pterm.Success.Printfln(format, a...) } diff --git a/src/pkg/message/spinner.go b/src/pkg/message/spinner.go index 93511a705c..76d2d81bb5 100644 --- a/src/pkg/message/spinner.go +++ b/src/pkg/message/spinner.go @@ -8,6 +8,7 @@ import ( "bufio" "bytes" "fmt" + "log/slog" "strings" "github.com/pterm/pterm" @@ -34,15 +35,19 @@ func NewProgressSpinner(format string, a ...any) *Spinner { } var spinner *pterm.SpinnerPrinter + var err error text := pterm.Sprintf(format, a...) if NoProgress { Info(text) } else { - spinner, _ = pterm.DefaultSpinner. + spinner, err = pterm.DefaultSpinner. WithRemoveWhenDone(false). // Src: https://github.com/gernest/wow/blob/master/spin/spinners.go#L335 WithSequence(sequence...). Start(text) + if err != nil { + slog.Debug("unable to create default spinner", "error", err) + } } activeSpinner = &Spinner{ @@ -108,7 +113,10 @@ func (p *Spinner) Updatef(format string, a ...any) { // Stop the spinner. func (p *Spinner) Stop() { if p.spinner != nil && p.spinner.IsActive { - _ = p.spinner.Stop() + err := p.spinner.Stop() + if err != nil { + slog.Debug("unable to stop spinner", "error", err) + } } activeSpinner = nil } From acdb601e8dc0372081920d1a7725516f77d06c6d Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Wed, 25 Sep 2024 16:38:55 -0700 Subject: [PATCH 25/57] fix: propagate any errors in src/pkg/variables Signed-off-by: Kit Patella --- src/pkg/variables/templates.go | 7 ++++++- src/pkg/variables/templates_test.go | 11 +++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/pkg/variables/templates.go b/src/pkg/variables/templates.go index f0153d0baa..a4dcf1d84f 100644 --- a/src/pkg/variables/templates.go +++ b/src/pkg/variables/templates.go @@ -57,7 +57,6 @@ func (vc *VariableConfig) ReplaceTextTemplate(path string) error { if err != nil { return err } - defer textFile.Close() // This regex takes a line and parses the text before and after a discovered template: https://regex101.com/r/ilUxAz/1 regexTemplateLine := regexp.MustCompile(fmt.Sprintf("(?P.*?)(?P