Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: directly handle ignored errs and nolint intentionally ignored errs #2993

Merged
merged 49 commits into from
Oct 2, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
358dd41
add errcheck linting
mkcp Sep 11, 2024
655dd76
linter ignore some intentionally unhandled errs
mkcp Sep 12, 2024
cce62ce
handle some env var set and unset errors in tests
mkcp Sep 12, 2024
ae05ebb
make some notes in cmd/destroy.go for later
mkcp Sep 12, 2024
992aa3c
direct handle some deferred cleanups to add direct error handling
mkcp Sep 12, 2024
81e54f7
capture errors from cmd/root
mkcp Sep 13, 2024
9b2b2e9
capture errors on src/config/config.go. checking ci
mkcp Sep 13, 2024
a508aa7
fix: ensure we capture and propagate errors in src/internal
mkcp Sep 25, 2024
6f53a29
fix: propagate or log unhandled errors in src/cmd
mkcp Sep 25, 2024
4283faf
fix: check for or intentionally ignore some dangling errors in tests
mkcp Sep 25, 2024
cd5fdc7
fix: use RemoveAll not remove so it doesnt error every time
mkcp Sep 25, 2024
9059544
fix: disabling the err catch to see if it's something i changed
mkcp Sep 25, 2024
4a0d6ef
fix: should fix the directory not empty panic
mkcp Sep 25, 2024
4da92b9
fix: see if CI is flaking with the original tests. probably just need…
mkcp Sep 25, 2024
29c5d08
fix: these tests should clean up properly and not error
mkcp Sep 25, 2024
dfdf78a
fix: actually cleaning up the test with RemovalAll seems to cause a p…
mkcp Sep 25, 2024
5a4241e
fix: reintroduce the ext_in_cluster test error catches
mkcp Sep 25, 2024
ac3db17
fix: missed a spot
mkcp Sep 25, 2024
3b274f7
fix: e2e 24 - can we assert that this error exists or do we have to i…
mkcp Sep 25, 2024
a9d6266
fix: address some fixmes before merge
mkcp Sep 25, 2024
ccabf6c
fix: does this work now?
mkcp Sep 25, 2024
8a56cd5
fix: handle and propagate errors in pkg/utils
mkcp Sep 25, 2024
5addccc
fix: propagate any errors in ColorPrintYAML
mkcp Sep 25, 2024
7c04dff
fix: propagate any errors in src/pkg/message
mkcp Sep 25, 2024
acdb601
fix: propagate any errors in src/pkg/variables
mkcp Sep 25, 2024
803b55e
fix: propagate any errors in src/pkg/layout
mkcp Sep 25, 2024
ffc2d20
fix: srcfile is already closed and the error handled, no need to defe…
mkcp Sep 25, 2024
35cf75e
fix: ensure errors from deferred resource cleanup are passed back to …
mkcp Sep 26, 2024
53a175b
chore: undo linter rule for CI so we can merge incremental changes
mkcp Sep 26, 2024
391296f
chore: undo the other linter rule
mkcp Sep 26, 2024
2c61d4c
chore: extract test changes to another branch
mkcp Sep 26, 2024
423c96a
fix: remove nolints for now to make ci happy. we'll have to readd later
mkcp Sep 26, 2024
41265d3
Merge branch 'main' into mkcp/2949-errcheck-linter
mkcp Sep 26, 2024
d2334bb
chore: address some review comments
mkcp Sep 30, 2024
a043273
chore: fix some bugs in nil returns for errors.Join and address revie…
mkcp Sep 30, 2024
59c3113
review comment
mkcp Sep 30, 2024
7f53091
chore: missed some slogs
mkcp Sep 30, 2024
1602735
chore: gofmt cmd/tools/helm.go
mkcp Sep 30, 2024
7d4da41
Merge branch 'main' into mkcp/2949-errcheck-linter
mkcp Sep 30, 2024
91b28ba
chore: mark some todos for lint ignores
mkcp Sep 30, 2024
910027f
fix review comments
mkcp Oct 1, 2024
e56681c
fix: actually store the error values in the deferred cleanup
mkcp Oct 1, 2024
193254c
a/b testing if inline closes not defers fixes windows tests
mkcp Oct 1, 2024
bb4b8b6
a/b testing... is it really two closes that fixes it?
mkcp Oct 1, 2024
6c86974
a/b testing... what about inline only, no defer inside the loop?
mkcp Oct 1, 2024
ff0df92
a/b testing... no inline inside loop. defer only. and defer outside o…
mkcp Oct 1, 2024
df0d3b4
fix: ensure we close the file before trying to remove it
mkcp Oct 1, 2024
2afa430
fix: if the file is already closed, don't join to err return
mkcp Oct 1, 2024
1c69af4
missed some spots
mkcp Oct 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .golangci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -68,6 +69,7 @@ linters-settings:
issues:
# Revive rules that are disabled by default.
include:
- EXC0001
- EXC0012
- EXC0013
- EXC0014
Expand Down
14 changes: 11 additions & 3 deletions src/cmd/destroy.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"context"
"errors"
"fmt"
"log/slog"
"os"
"regexp"

Expand All @@ -30,6 +31,7 @@ var destroyCmd = &cobra.Command{
Aliases: []string{"d"},
Short: lang.CmdDestroyShort,
Long: lang.CmdDestroyLong,
// 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)
Expand Down Expand Up @@ -57,7 +59,10 @@ var destroyCmd = &cobra.Command{

// Run all the scripts!
pattern := regexp.MustCompile(`(?mi)zarf-clean-.+\.sh$`)
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
Expand All @@ -71,8 +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
_ = 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)
mkcp marked this conversation as resolved.
Show resolved Hide resolved
}
}
} else {
// Perform chart uninstallation
Expand Down
29 changes: 22 additions & 7 deletions src/cmd/dev.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"errors"
"fmt"
"io"
"log/slog"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -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)
Expand All @@ -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 != "" {
Expand All @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down
21 changes: 15 additions & 6 deletions src/cmd/initialize.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
if helpers.InvalidPath(absCachePath) {
// Create the directory if the path is invalid
err = helpers.CreateDirectory(absCachePath, helpers.ReadExecuteAllWriteUser)
if 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
Expand All @@ -130,6 +138,7 @@ func downloadInitPackage(ctx context.Context, cacheDirectory string) (string, er
message.Note(lang.CmdInitPullNote)

// Prompt the user if --confirm not specified
// REVIEW(mkcp): It looks like this condition can't be met - maybe --confirm was removed at some point?
mkcp marked this conversation as resolved.
Show resolved Hide resolved
if !confirmDownload {
prompt := &survey.Confirm{
Message: lang.CmdInitPullConfirm,
Expand Down
10 changes: 7 additions & 3 deletions src/cmd/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"encoding/json"
"errors"
"fmt"
"log/slog"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -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
},
Expand Down Expand Up @@ -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)
}
}
}
46 changes: 38 additions & 8 deletions src/cmd/package.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"context"
"errors"
"fmt"
"log/slog"
"os"
"path/filepath"
"regexp"
Expand Down Expand Up @@ -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...)
Expand All @@ -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)
Expand Down Expand Up @@ -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")
mkcp marked this conversation as resolved.
Show resolved Hide resolved
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) {
Expand All @@ -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) {
Expand Down
5 changes: 4 additions & 1 deletion src/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,10 @@ var rootCmd = &cobra.Command{
Run: func(cmd *cobra.Command, args []string) {
zarfLogo := message.GetLogo()
_, _ = fmt.Fprintln(os.Stderr, zarfLogo)
cmd.Help()
err := cmd.Help()
if err != nil {
_, _ = fmt.Fprintln(os.Stderr, err)
}

if len(args) > 0 {
if strings.Contains(args[0], config.ZarfPackagePrefix) || strings.Contains(args[0], "zarf-init") {
Expand Down
6 changes: 5 additions & 1 deletion src/cmd/tools/helm.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package tools

import (
"log/slog"
"os"

"github.com/zarf-dev/zarf/src/cmd/tools/helm"
Expand All @@ -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))
Expand Down
12 changes: 8 additions & 4 deletions src/cmd/tools/zarf.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
}
Expand Down
13 changes: 8 additions & 5 deletions src/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
3 changes: 2 additions & 1 deletion src/internal/git/repository_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
Loading
Loading