From bbb50dfb1493226b44981006b6c8f50d416d917e Mon Sep 17 00:00:00 2001
From: John Cai <jcai@gitlab.com>
Date: Tue, 14 Dec 2021 16:03:52 -0500
Subject: praefect: Fix output of remove-repository

There was a bug in the logic so that we printed out "Did not remove"
even though we did remove the repository from disk. This change also
deletes some wrong code that checks for an error message that only
exists in praefect-but we are calling gitaly directly so this error
message will never appear.

Changelog: fixed
---
 cmd/praefect/subcmd_remove_repository.go      |  9 +++-----
 cmd/praefect/subcmd_remove_repository_test.go | 33 +++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/cmd/praefect/subcmd_remove_repository.go b/cmd/praefect/subcmd_remove_repository.go
index f6ee01358..49b049615 100644
--- a/cmd/praefect/subcmd_remove_repository.go
+++ b/cmd/praefect/subcmd_remove_repository.go
@@ -241,11 +241,7 @@ func (cmd *removeRepository) removeRepository(ctx context.Context, repo *gitalyp
 	ctx = metadata.AppendToOutgoingContext(ctx, "client_name", removeRepositoryCmdName)
 	repositoryClient := gitalypb.NewRepositoryServiceClient(conn)
 	if _, err := repositoryClient.RemoveRepository(ctx, &gitalypb.RemoveRepositoryRequest{Repository: repo}); err != nil {
-		s, ok := status.FromError(err)
-		if !ok {
-			return false, fmt.Errorf("RemoveRepository: %w", err)
-		}
-		if !strings.Contains(s.Message(), fmt.Sprintf("get primary: repository %q/%q not found", cmd.virtualStorage, cmd.relativePath)) {
+		if _, ok := status.FromError(err); !ok {
 			return false, fmt.Errorf("RemoveRepository: %w", err)
 		}
 		return false, nil
@@ -312,8 +308,9 @@ func (cmd *removeRepository) removeNode(
 	} else {
 		if removed {
 			fmt.Fprintf(cmd.w, "Successfully removed %s from %s\n", cmd.relativePath, node.Storage)
+		} else {
+			fmt.Fprintf(cmd.w, "Did not remove %s from %s\n", cmd.relativePath, node.Storage)
 		}
-		fmt.Fprintf(cmd.w, "Did not remove %s from %s\n", cmd.relativePath, node.Storage)
 	}
 	logger.Debugf("repository removal call to gitaly %q completed", node.Storage)
 }
diff --git a/cmd/praefect/subcmd_remove_repository_test.go b/cmd/praefect/subcmd_remove_repository_test.go
index bf2ece389..6e920ec0e 100644
--- a/cmd/praefect/subcmd_remove_repository_test.go
+++ b/cmd/praefect/subcmd_remove_repository_test.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"flag"
 	"fmt"
+	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -158,6 +159,38 @@ func TestRemoveRepository_Exec(t *testing.T) {
 		assert.Contains(t, out.String(), "Repository found on the following gitaly nodes:")
 		assert.Contains(t, out.String(), fmt.Sprintf("Attempting to remove %s from the database, and delete it from all gitaly nodes...\n", repo.RelativePath))
 		assert.Contains(t, out.String(), fmt.Sprintf("Successfully removed %s from", repo.RelativePath))
+		assert.NotContains(t, out.String(), fmt.Sprintf("Did not remove %s from", repo.RelativePath))
+
+		var repositoryRowExists bool
+		require.NoError(t, db.QueryRow(
+			`SELECT EXISTS(SELECT FROM repositories WHERE virtual_storage = $1 AND relative_path = $2)`,
+			cmd.virtualStorage, cmd.relativePath,
+		).Scan(&repositoryRowExists))
+		require.False(t, repositoryRowExists)
+	})
+
+	t.Run("repository doesnt exist on one gitaly", func(t *testing.T) {
+		var out bytes.Buffer
+		repo := createRepo(t, ctx, repoClient, praefectStorage, t.Name())
+
+		require.NoError(t, os.RemoveAll(filepath.Join(g2Cfg.Storages[0].Path, repo.RelativePath)))
+
+		cmd := &removeRepository{
+			logger:         testhelper.NewDiscardingLogger(t),
+			virtualStorage: repo.StorageName,
+			relativePath:   repo.RelativePath,
+			dialTimeout:    time.Second,
+			apply:          true,
+			w:              &writer{w: &out},
+		}
+		require.NoError(t, cmd.Exec(flag.NewFlagSet("", flag.PanicOnError), conf))
+
+		require.NoDirExists(t, filepath.Join(g1Cfg.Storages[0].Path, repo.RelativePath))
+		require.NoDirExists(t, filepath.Join(g2Cfg.Storages[0].Path, repo.RelativePath))
+		assert.Contains(t, out.String(), "Repository found in the database.\n")
+		assert.Contains(t, out.String(), "Repository found on the following gitaly nodes: gitaly-1")
+		assert.Contains(t, out.String(), fmt.Sprintf("Attempting to remove %s from the database, and delete it from all gitaly nodes...\n", repo.RelativePath))
+		assert.Contains(t, out.String(), fmt.Sprintf("Successfully removed %s from gitaly-1", repo.RelativePath))
 
 		var repositoryRowExists bool
 		require.NoError(t, db.QueryRow(
-- 
cgit v1.2.3