diff options
author | Pavlo Strokov <pstrokov@gitlab.com> | 2020-04-23 03:07:14 +0300 |
---|---|---|
committer | Paul Okstad <pokstad@gitlab.com> | 2020-04-23 03:07:14 +0300 |
commit | afe9210ba500164d21f638f0c4721fc0da7f5350 (patch) | |
tree | fb279d48a6ab51085a8e0fa68338e0a4a8a40c9c | |
parent | 253cbb3e251613aa68ec1fb3f7e6f160c88e306c (diff) |
Custom error replacement
With usage of `errors` introduced in 1.13 version we don't
need most of custom errors we have right now.
Walker changed to always remove temp dir after run.
-rw-r--r-- | internal/cache/walker.go | 21 | ||||
-rw-r--r-- | internal/cache/walker_test.go | 1 | ||||
-rw-r--r-- | internal/command/command.go | 4 | ||||
-rw-r--r-- | internal/command/command_test.go | 8 | ||||
-rw-r--r-- | internal/command/spawntoken.go | 2 | ||||
-rw-r--r-- | internal/config/config.go | 13 | ||||
-rw-r--r-- | internal/git/safecmd.go | 51 | ||||
-rw-r--r-- | internal/git/safecmd_test.go | 8 | ||||
-rw-r--r-- | internal/testhelper/testhelper.go | 4 |
9 files changed, 50 insertions, 62 deletions
diff --git a/internal/cache/walker.go b/internal/cache/walker.go index d69f29fa0..bd7c06457 100644 --- a/internal/cache/walker.go +++ b/internal/cache/walker.go @@ -139,6 +139,18 @@ func moveAndClear(storagePath string) error { return err } + defer func() { + dontpanic.Go(func() { + start := time.Now() + if err := os.RemoveAll(tmpDir); err != nil { + logger.Errorf("unable to remove disk cache objects: %q", err) + return + } + + logger.Infof("cleared all cache object files in %s after %s", tmpDir, time.Since(start)) + }) + }() + logger.Infof("moving disk cache object folder to %s", tmpDir) cachePath := tempdir.AppendCacheDir(storagePath) if err := os.Rename(cachePath, filepath.Join(tmpDir, "moved")); err != nil { @@ -150,15 +162,6 @@ func moveAndClear(storagePath string) error { return err } - dontpanic.Go(func() { - start := time.Now() - if err := os.RemoveAll(tmpDir); err != nil { - logger.Errorf("unable to remove disk cache objects: %q", err) - } - - logger.Infof("cleared all cache object files in %s after %s", tmpDir, time.Since(start)) - }) - return nil } diff --git a/internal/cache/walker_test.go b/internal/cache/walker_test.go index ef7ba95f9..dd95f945f 100644 --- a/internal/cache/walker_test.go +++ b/internal/cache/walker_test.go @@ -128,7 +128,6 @@ func pollCountersUntil(t testing.TB, expectRemovals int) { if cache.ExportMockRemovalCounter.Count() == expectRemovals { break } - t.Log(cache.ExportMockRemovalCounter.Count()) time.Sleep(time.Millisecond) } } diff --git a/internal/command/command.go b/internal/command/command.go index 3477c33ea..0baa29e42 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -142,9 +142,7 @@ func WaitAllDone() { wg.Wait() } -type spawnTimeoutError struct{ error } type contextWithoutDonePanic string -type nullInArgvError struct{ error } // noopWriteCloser has a noop Close(). The reason for this is so we can close any WriteClosers that get // passed into writeLines. We need this for WriteClosers such as the Logrus writer, which has a @@ -420,7 +418,7 @@ func checkNullArgv(cmd *exec.Cmd) error { for _, arg := range cmd.Args { if strings.IndexByte(arg, 0) > -1 { // Use %q so that the null byte gets printed as \x00 - return nullInArgvError{fmt.Errorf("detected null byte in command argument %q", arg)} + return fmt.Errorf("detected null byte in command argument %q", arg) } } diff --git a/internal/command/command_test.go b/internal/command/command_test.go index 782635242..e16c7a7cd 100644 --- a/internal/command/command_test.go +++ b/internal/command/command_test.go @@ -156,9 +156,7 @@ wait: require.True(t, timePassed, "time must have passed") require.Error(t, err) - - _, ok := err.(spawnTimeoutError) - require.True(t, ok, "type of error should be spawnTimeoutError") + require.Contains(t, err.Error(), "process spawn timed out after") } func TestNewCommandWithSetupStdin(t *testing.T) { @@ -188,9 +186,7 @@ func TestNewCommandNullInArg(t *testing.T) { _, err := New(ctx, exec.Command("sh", "-c", "hello\x00world"), nil, nil, nil) require.Error(t, err) - - _, ok := err.(nullInArgvError) - require.True(t, ok, "expected %+v to be nullInArgvError", err) + require.EqualError(t, err, `detected null byte in command argument "hello\x00world"`) } func TestCommandStdErr(t *testing.T) { diff --git a/internal/command/spawntoken.go b/internal/command/spawntoken.go index 8c5b21783..623d686d8 100644 --- a/internal/command/spawntoken.go +++ b/internal/command/spawntoken.go @@ -68,7 +68,7 @@ func getSpawnToken(ctx context.Context) (putToken func(), err error) { logTime(ctx, start, "spawn token timeout") spawnTimeoutCount.Inc() - return nil, spawnTimeoutError{fmt.Errorf("process spawn timed out after %v", spawnConfig.Timeout)} + return nil, fmt.Errorf("process spawn timed out after %v", spawnConfig.Timeout) case <-ctx.Done(): return nil, ctx.Err() } diff --git a/internal/config/config.go b/internal/config/config.go index b07945472..aa252ceed 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -253,15 +253,20 @@ func validateStorages() error { for i, storage := range Config.Storages { if storage.Name == "" { - return fmt.Errorf("empty storage name in %v", storage) + return fmt.Errorf("empty storage name in %+v", storage) } if storage.Path == "" { - return fmt.Errorf("empty storage path in %v", storage) + return fmt.Errorf("empty storage path in %+v", storage) } - if fs, err := os.Stat(storage.Path); err != nil || !fs.IsDir() { - return fmt.Errorf("storage paths have to exist %v", storage) + fs, err := os.Stat(storage.Path) + if err != nil { + return fmt.Errorf("storage %+v path must exist: %w", storage, err) + } + + if !fs.IsDir() { + return fmt.Errorf("storage %+v path must be a dir", storage) } stPath := filepath.Clean(storage.Path) diff --git a/internal/git/safecmd.go b/internal/git/safecmd.go index eaee4f5be..dc1885e48 100644 --- a/internal/git/safecmd.go +++ b/internal/git/safecmd.go @@ -2,6 +2,7 @@ package git import ( "context" + "errors" "fmt" "io" "regexp" @@ -12,12 +13,17 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git/repository" ) -var invalidationTotal = prometheus.NewCounterVec( - prometheus.CounterOpts{ - Name: "gitaly_invalid_commands_total", - Help: "Total number of invalid arguments tried to execute", - }, - []string{"command"}, +var ( + invalidationTotal = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "gitaly_invalid_commands_total", + Help: "Total number of invalid arguments tried to execute", + }, + []string{"command"}, + ) + + // ErrInvalidArg represent family of errors to report about bad argument used to make a call. + ErrInvalidArg = errors.New("invalid argument") ) func init() { @@ -52,9 +58,7 @@ func (sc SubCmd) ValidateArgs() ([]string, error) { var safeArgs []string if !subCmdNameRegex.MatchString(sc.Name) { - return nil, &invalidArgErr{ - msg: fmt.Sprintf("invalid sub command name %q", sc.Name), - } + return nil, fmt.Errorf("invalid sub command name %q: %w", sc.Name, ErrInvalidArg) } safeArgs = append(safeArgs, sc.Name) @@ -102,9 +106,7 @@ func (SubSubCmd) IsOption() {} // sanitary func (sc SubSubCmd) ValidateArgs() ([]string, error) { if !subCmdNameRegex.MatchString(sc.Name) { - return nil, &invalidArgErr{ - msg: fmt.Sprintf("invalid sub-sub command name %q", sc.Name), - } + return nil, fmt.Errorf("invalid sub-sub command name %q: %w", sc.Name, ErrInvalidArg) } return []string{sc.Name}, nil } @@ -123,9 +125,7 @@ var configKeyRegex = regexp.MustCompile(`^[[:alnum:]]+[-[:alnum:]]*\.(.+\.)*[[:a // ValidateArgs validates the config pair args func (cp ConfigPair) ValidateArgs() ([]string, error) { if !configKeyRegex.MatchString(cp.Key) { - return nil, &invalidArgErr{ - msg: fmt.Sprintf("config key %q failed regexp validation", cp.Key), - } + return nil, fmt.Errorf("config key %q failed regexp validation: %w", cp.Key, ErrInvalidArg) } return []string{cp.Key, cp.Value}, nil } @@ -142,9 +142,7 @@ func (Flag) IsOption() {} // ValidateArgs returns an error if the flag is not sanitary func (f Flag) ValidateArgs() ([]string, error) { if !flagRegex.MatchString(f.Name) { - return nil, &invalidArgErr{ - msg: fmt.Sprintf("flag %q failed regex validation", f.Name), - } + return nil, fmt.Errorf("flag %q failed regex validation: %w", f.Name, ErrInvalidArg) } return []string{f.Name}, nil } @@ -162,32 +160,21 @@ func (ValueFlag) IsOption() {} // ValidateArgs returns an error if the flag is not sanitary func (vf ValueFlag) ValidateArgs() ([]string, error) { if !flagRegex.MatchString(vf.Name) { - return nil, &invalidArgErr{ - msg: fmt.Sprintf("value flag %q failed regex validation", vf.Name), - } + return nil, fmt.Errorf("value flag %q failed regex validation: %w", vf.Name, ErrInvalidArg) } return []string{vf.Name, vf.Value}, nil } var flagRegex = regexp.MustCompile(`^(-|--)[[:alnum:]]`) -type invalidArgErr struct { - msg string -} - -func (iae *invalidArgErr) Error() string { return iae.msg } - // IsInvalidArgErr relays if the error is due to an argument validation failure func IsInvalidArgErr(err error) bool { - _, ok := err.(*invalidArgErr) - return ok + return errors.Is(err, ErrInvalidArg) } func validatePositionalArg(arg string) error { if strings.HasPrefix(arg, "-") { - return &invalidArgErr{ - msg: fmt.Sprintf("positional arg %q cannot start with dash '-'", arg), - } + return fmt.Errorf("positional arg %q cannot start with dash '-': %w", arg, ErrInvalidArg) } return nil } diff --git a/internal/git/safecmd_test.go b/internal/git/safecmd_test.go index 8d9a508ef..6aa8676ef 100644 --- a/internal/git/safecmd_test.go +++ b/internal/git/safecmd_test.go @@ -80,28 +80,28 @@ func TestSafeCmdInvalidArg(t *testing.T) { }{ { subCmd: git.SubCmd{Name: "--meow"}, - errMsg: "invalid sub command name \"--meow\"", + errMsg: `invalid sub command name "--meow": invalid argument`, }, { subCmd: git.SubCmd{ Name: "meow", Flags: []git.Option{git.Flag{"woof"}}, }, - errMsg: "flag \"woof\" failed regex validation", + errMsg: `flag "woof" failed regex validation: invalid argument`, }, { subCmd: git.SubCmd{ Name: "meow", Args: []string{"--tweet"}, }, - errMsg: "positional arg \"--tweet\" cannot start with dash '-'", + errMsg: `positional arg "--tweet" cannot start with dash '-': invalid argument`, }, { subCmd: git.SubCmd{ Name: "meow", Flags: []git.Option{git.SubSubCmd{"-invalid"}}, }, - errMsg: "invalid sub-sub command name \"-invalid\"", + errMsg: `invalid sub-sub command name "-invalid": invalid argument`, }, } { _, err := git.SafeCmd( diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index bdc415e1c..03712db58 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -55,6 +55,8 @@ var configureOnce sync.Once // terminates the program. func Configure() { configureOnce.Do(func() { + gitalylog.Configure("json", "info") + config.Config.Storages = []config.Storage{ {Name: "default", Path: GitlabTestStoragePath()}, } @@ -86,8 +88,6 @@ func Configure() { log.Fatalf("error configuring tests: %v", err) } } - - gitalylog.Configure("json", "info") }) } |