config: Create errors with helpers

We should not use status package to create errors as it could lead
to a double status wrapping with overriding of the initial status
code. Also, it may confuse as it seems like there was a gPRC call
that returned an error. By using own helpers we also can change
implementation without affecting places of usage.

3 files changed, 25 insertions, 27 deletions

diff --git a/internal/gitaly/config/locator.go b/internal/gitaly/config/locator.go
index c9c2dfc97..3812c6190 100644
--- a/internal/gitaly/config/locator.go
+++ b/internal/gitaly/config/locator.go
@@ -6,8 +6,7 @@ import (
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/storage"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 )
 
 const (
@@ -49,7 +48,7 @@ func (l *configLocator) GetRepoPath(repo repository.GitRepo) (string, error) {
 		return repoPath, nil
 	}
 
-	return "", status.Errorf(codes.NotFound, "GetRepoPath: not a git repository: %q", repoPath)
+	return "", helper.ErrNotFoundf("GetRepoPath: not a git repository: %q", repoPath)
 }
 
 // GetPath returns the path of the repo passed as first argument. An error is
@@ -63,19 +62,19 @@ func (l *configLocator) GetPath(repo repository.GitRepo) (string, error) {
 
 	if _, err := os.Stat(storagePath); err != nil {
 		if os.IsNotExist(err) {
-			return "", status.Errorf(codes.NotFound, "GetPath: does not exist: %v", err)
+			return "", helper.ErrNotFoundf("GetPath: does not exist: %v", err)
 		}
-		return "", status.Errorf(codes.Internal, "GetPath: storage path: %v", err)
+		return "", helper.ErrInternalf("GetPath: storage path: %v", err)
 	}
 
 	relativePath := repo.GetRelativePath()
 	if len(relativePath) == 0 {
-		err := status.Errorf(codes.InvalidArgument, "GetPath: relative path missing from %+v", repo)
+		err := helper.ErrInvalidArgumentf("GetPath: relative path missing from %+v", repo)
 		return "", err
 	}
 
 	if _, err := storage.ValidateRelativePath(storagePath, relativePath); err != nil {
-		return "", status.Errorf(codes.InvalidArgument, "GetRepoPath: %s", err)
+		return "", helper.ErrInvalidArgumentf("GetRepoPath: %s", err)
 	}
 
 	return filepath.Join(storagePath, relativePath), nil
@@ -86,7 +85,7 @@ func (l *configLocator) GetPath(repo repository.GitRepo) (string, error) {
 func (l *configLocator) GetStorageByName(storageName string) (string, error) {
 	storagePath, ok := l.conf.StoragePath(storageName)
 	if !ok {
-		return "", status.Errorf(codes.InvalidArgument, "GetStorageByName: no such storage: %q", storageName)
+		return "", helper.ErrInvalidArgumentf("GetStorageByName: no such storage: %q", storageName)
 	}
 
 	return storagePath, nil
@@ -110,7 +109,7 @@ func (l *configLocator) TempDir(storageName string) (string, error) {
 func (l *configLocator) getPath(storageName, prefix string) (string, error) {
 	storagePath, ok := l.conf.StoragePath(storageName)
 	if !ok {
-		return "", status.Errorf(codes.InvalidArgument, "%s dir: no such storage: %q",
+		return "", helper.ErrInvalidArgumentf("%s dir: no such storage: %q",
 			filepath.Base(prefix), storageName)
 	}
 
diff --git a/internal/gitaly/config/locator_test.go b/internal/gitaly/config/locator_test.go
index 96268ef42..9f34e463d 100644
--- a/internal/gitaly/config/locator_test.go
+++ b/internal/gitaly/config/locator_test.go
@@ -11,12 +11,11 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service/setup"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 func TestConfigLocator_GetRepoPath(t *testing.T) {
@@ -49,37 +48,37 @@ func TestConfigLocator_GetRepoPath(t *testing.T) {
 		{
 			desc:   "storage is empty",
 			repo:   &gitalypb.Repository{RelativePath: repo.RelativePath},
-			expErr: status.Error(codes.InvalidArgument, `GetStorageByName: no such storage: ""`),
+			expErr: helper.ErrInvalidArgumentf(`GetStorageByName: no such storage: ""`),
 		},
 		{
 			desc:   "unknown storage",
 			repo:   &gitalypb.Repository{StorageName: "invalid", RelativePath: repo.RelativePath},
-			expErr: status.Error(codes.InvalidArgument, `GetStorageByName: no such storage: "invalid"`),
+			expErr: helper.ErrInvalidArgumentf(`GetStorageByName: no such storage: "invalid"`),
 		},
 		{
 			desc:   "storage doesn't exist on disk",
 			repo:   &gitalypb.Repository{StorageName: cfg.Storages[1].Name, RelativePath: repo.RelativePath},
-			expErr: status.Errorf(codes.NotFound, `GetPath: does not exist: stat %s: no such file or directory`, cfg.Storages[1].Path),
+			expErr: helper.ErrNotFoundf(`GetPath: does not exist: stat %s: no such file or directory`, cfg.Storages[1].Path),
 		},
 		{
 			desc:   "relative path is empty",
 			repo:   &gitalypb.Repository{StorageName: storageName, RelativePath: ""},
-			expErr: status.Error(codes.InvalidArgument, `GetPath: relative path missing from storage_name:"exists"`),
+			expErr: helper.ErrInvalidArgumentf(`GetPath: relative path missing from storage_name:"exists"`),
 		},
 		{
 			desc:   "unknown relative path",
 			repo:   &gitalypb.Repository{StorageName: storageName, RelativePath: "invalid"},
-			expErr: status.Errorf(codes.NotFound, `GetRepoPath: not a git repository: %q`, filepath.Join(cfg.Storages[0].Path, "invalid")),
+			expErr: helper.ErrNotFoundf(`GetRepoPath: not a git repository: %q`, filepath.Join(cfg.Storages[0].Path, "invalid")),
 		},
 		{
 			desc:   "path exists but not a git repository",
 			repo:   &gitalypb.Repository{StorageName: storageName, RelativePath: notRepositoryFolder},
-			expErr: status.Errorf(codes.NotFound, `GetRepoPath: not a git repository: %q`, filepath.Join(cfg.Storages[0].Path, notRepositoryFolder)),
+			expErr: helper.ErrNotFoundf(`GetRepoPath: not a git repository: %q`, filepath.Join(cfg.Storages[0].Path, notRepositoryFolder)),
 		},
 		{
 			desc:   "relative path escapes parent folder",
 			repo:   &gitalypb.Repository{StorageName: storageName, RelativePath: "../.."},
-			expErr: status.Error(codes.InvalidArgument, `GetRepoPath: relative path escapes root directory`),
+			expErr: helper.ErrInvalidArgumentf(`GetRepoPath: relative path escapes root directory`),
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
@@ -147,27 +146,27 @@ func TestConfigLocator_GetPath(t *testing.T) {
 		{
 			desc:   "storage is empty",
 			repo:   &gitalypb.Repository{RelativePath: repo.RelativePath},
-			expErr: status.Error(codes.InvalidArgument, `GetStorageByName: no such storage: ""`),
+			expErr: helper.ErrInvalidArgumentf(`GetStorageByName: no such storage: ""`),
 		},
 		{
 			desc:   "unknown storage",
 			repo:   &gitalypb.Repository{StorageName: "invalid", RelativePath: repo.RelativePath},
-			expErr: status.Error(codes.InvalidArgument, `GetStorageByName: no such storage: "invalid"`),
+			expErr: helper.ErrInvalidArgumentf(`GetStorageByName: no such storage: "invalid"`),
 		},
 		{
 			desc:   "storage doesn't exist on disk",
 			repo:   &gitalypb.Repository{StorageName: cfg.Storages[1].Name, RelativePath: repo.RelativePath},
-			expErr: status.Errorf(codes.NotFound, `GetPath: does not exist: stat %s: no such file or directory`, cfg.Storages[1].Path),
+			expErr: helper.ErrNotFoundf(`GetPath: does not exist: stat %s: no such file or directory`, cfg.Storages[1].Path),
 		},
 		{
 			desc:   "relative path is empty",
 			repo:   &gitalypb.Repository{StorageName: storageName, RelativePath: ""},
-			expErr: status.Error(codes.InvalidArgument, `GetPath: relative path missing from storage_name:"exists"`),
+			expErr: helper.ErrInvalidArgumentf(`GetPath: relative path missing from storage_name:"exists"`),
 		},
 		{
 			desc:   "relative path escapes parent folder",
 			repo:   &gitalypb.Repository{StorageName: storageName, RelativePath: "../.."},
-			expErr: status.Error(codes.InvalidArgument, `GetRepoPath: relative path escapes root directory`),
+			expErr: helper.ErrInvalidArgumentf(`GetRepoPath: relative path escapes root directory`),
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
@@ -199,7 +198,7 @@ func TestConfigLocator_CacheDir(t *testing.T) {
 
 	t.Run("unknown storage", func(t *testing.T) {
 		_, err := locator.CacheDir("unknown")
-		require.Equal(t, status.Error(codes.InvalidArgument, `cache dir: no such storage: "unknown"`), err)
+		require.Equal(t, helper.ErrInvalidArgumentf(`cache dir: no such storage: "unknown"`), err)
 	})
 }
 
@@ -224,7 +223,7 @@ func TestConfigLocator_StateDir(t *testing.T) {
 
 	t.Run("unknown storage", func(t *testing.T) {
 		_, err := locator.StateDir("unknown")
-		require.Equal(t, status.Error(codes.InvalidArgument, `state dir: no such storage: "unknown"`), err)
+		require.Equal(t, helper.ErrInvalidArgumentf(`state dir: no such storage: "unknown"`), err)
 	})
 }
 
@@ -249,6 +248,6 @@ func TestConfigLocator_TempDir(t *testing.T) {
 
 	t.Run("unknown storage", func(t *testing.T) {
 		_, err := locator.TempDir("unknown")
-		require.Equal(t, status.Error(codes.InvalidArgument, `tmp dir: no such storage: "unknown"`), err)
+		require.Equal(t, helper.ErrInvalidArgumentf(`tmp dir: no such storage: "unknown"`), err)
 	})
 }
diff --git a/internal/tempdir/clean_test.go b/internal/tempdir/clean_test.go
index b21368d06..17c858ed6 100644
--- a/internal/tempdir/clean_test.go
+++ b/internal/tempdir/clean_test.go
@@ -87,7 +87,7 @@ func TestCleanNoStorageExists(t *testing.T) {
 	locator := config.NewLocator(cfg)
 
 	err := clean(locator, config.Storage{Name: "does-not-exist", Path: "/something"})
-	require.EqualError(t, err, "temporary dir: rpc error: code = InvalidArgument desc = tmp dir: no such storage: \"does-not-exist\"")
+	require.EqualError(t, err, "temporary dir: tmp dir: no such storage: \"does-not-exist\"")
 }
 
 type mockLocator struct {