Merge branch 'ps-praefect-cli-list-untracked-repositories' into 'master'

praefect: list-untracked-repositories sub-command migrated

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5689

Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Pavlo Strokov <pstrokov@gitlab.com>
Co-authored-by: Pavlo Strokov <pstrokov@gitlab.com>

4 files changed, 118 insertions, 107 deletions

diff --git a/internal/cli/praefect/main.go b/internal/cli/praefect/main.go
index 9e0e2eb2d..b4e106d97 100644
--- a/internal/cli/praefect/main.go
+++ b/internal/cli/praefect/main.go
@@ -76,6 +76,7 @@ func NewApp() *cli.App {
 			newDatalossCommand(),
 			newDialNodesCommand(),
 			newListStoragesCommand(),
+			newListUntrackedRepositoriesCommand(),
 		},
 		Flags: []cli.Flag{
 			&cli.StringFlag{
diff --git a/internal/cli/praefect/subcmd.go b/internal/cli/praefect/subcmd.go
index 4522d0699..64111af7f 100644
--- a/internal/cli/praefect/subcmd.go
+++ b/internal/cli/praefect/subcmd.go
@@ -33,17 +33,16 @@ const (
 
 func subcommands(logger *logrus.Entry) map[string]subcmd {
 	return map[string]subcmd{
-		sqlPingCmdName:                &sqlPingSubcommand{},
-		sqlMigrateCmdName:             newSQLMigrateSubCommand(os.Stdout),
-		sqlMigrateDownCmdName:         &sqlMigrateDownSubcommand{},
-		sqlMigrateStatusCmdName:       &sqlMigrateStatusSubcommand{},
-		setReplicationFactorCmdName:   newSetReplicatioFactorSubcommand(os.Stdout),
-		removeRepositoryCmdName:       newRemoveRepository(logger, os.Stdout),
-		trackRepositoryCmdName:        newTrackRepository(logger, os.Stdout),
-		trackRepositoriesCmdName:      newTrackRepositories(logger, os.Stdout),
-		listUntrackedRepositoriesName: newListUntrackedRepositories(logger, os.Stdout),
-		metadataCmdName:               newMetadataSubcommand(os.Stdout),
-		verifyCmdName:                 newVerifySubcommand(os.Stdout),
+		sqlPingCmdName:              &sqlPingSubcommand{},
+		sqlMigrateCmdName:           newSQLMigrateSubCommand(os.Stdout),
+		sqlMigrateDownCmdName:       &sqlMigrateDownSubcommand{},
+		sqlMigrateStatusCmdName:     &sqlMigrateStatusSubcommand{},
+		setReplicationFactorCmdName: newSetReplicatioFactorSubcommand(os.Stdout),
+		removeRepositoryCmdName:     newRemoveRepository(logger, os.Stdout),
+		trackRepositoryCmdName:      newTrackRepository(logger, os.Stdout),
+		trackRepositoriesCmdName:    newTrackRepositories(logger, os.Stdout),
+		metadataCmdName:             newMetadataSubcommand(os.Stdout),
+		verifyCmdName:               newVerifySubcommand(os.Stdout),
 	}
 }
 
diff --git a/internal/cli/praefect/subcmd_list_untracked_repositories.go b/internal/cli/praefect/subcmd_list_untracked_repositories.go
index d80d20c74..f432bd929 100644
--- a/internal/cli/praefect/subcmd_list_untracked_repositories.go
+++ b/internal/cli/praefect/subcmd_list_untracked_repositories.go
@@ -4,12 +4,12 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
-	"flag"
 	"fmt"
 	"io"
 	"time"
 
-	"github.com/sirupsen/logrus"
+	"github.com/urfave/cli/v2"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore"
@@ -19,59 +19,59 @@ import (
 	"google.golang.org/grpc/metadata"
 )
 
-const (
-	listUntrackedRepositoriesName = "list-untracked-repositories"
-)
-
-var errNoConnectionToGitalies = errors.New("no connection established to gitaly nodes")
-
-type listUntrackedRepositories struct {
-	logger               logrus.FieldLogger
-	onlyIncludeOlderThan time.Duration
-	delimiter            string
-	out                  io.Writer
-}
-
-func newListUntrackedRepositories(logger logrus.FieldLogger, out io.Writer) *listUntrackedRepositories {
-	return &listUntrackedRepositories{logger: logger, out: out}
-}
-
-func (cmd *listUntrackedRepositories) FlagSet() *flag.FlagSet {
-	fs := flag.NewFlagSet(listUntrackedRepositoriesName, flag.ExitOnError)
-	fs.StringVar(&cmd.delimiter, "delimiter", "\n", "string used as a delimiter in output")
-	fs.DurationVar(
-		&cmd.onlyIncludeOlderThan,
-		"older-than",
-		6*time.Hour,
-		"only include repositories created before this duration",
-	)
-	fs.Usage = func() {
-		printfErr("Description:\n" +
-			"	This command checks if all repositories on all gitaly nodes tracked by praefect.\n" +
-			"	If repository is found on the disk, but it is not known to praefect the location of\n" +
-			"	that repository will be written into stdout stream in JSON format.\n")
-		fs.PrintDefaults()
-		printfErr("NOTE:\n" +
-			"	All errors and log messages directed to the stderr stream.\n" +
-			"	The output is produced as the new data appears, it doesn't wait\n" +
-			"	for the completion of the processing to produce the result.\n")
+func newListUntrackedRepositoriesCommand() *cli.Command {
+	return &cli.Command{
+		Name:  "list-untracked-repositories",
+		Usage: "shows repositories not tracked by Praefect",
+		Description: "This command checks whether all repositories on all Gitaly nodes are tracked by Praefect.\n" +
+			"If a repository is found on the disk, but it is not known to Praefect, then the location of\n" +
+			"that repository will be written to the standard output stream in JSON format.\n" +
+			"NOTE:\n" +
+			"All errors and log messages are written to the standard error stream.\n" +
+			"The output is produced as the new data appears, it doesn't wait\n" +
+			"for the completion of the processing to produce the result.\n",
+
+		Action: listUntrackedRepositoriesAction,
+		Flags: []cli.Flag{
+			&cli.StringFlag{
+				Name:  "delimiter",
+				Value: "\n",
+				Usage: "string used as a delimiter in output",
+			},
+			&cli.DurationFlag{
+				Name:  "older-than",
+				Value: 6 * time.Hour,
+				Usage: "only include repositories created before this duration",
+			},
+		},
+		Before: func(ctx *cli.Context) error {
+			if ctx.Args().Present() {
+				_ = cli.ShowSubcommandHelp(ctx)
+				return cli.Exit(unexpectedPositionalArgsError{Command: ctx.Command.Name}, 1)
+			}
+			return nil
+		},
 	}
-	return fs
 }
 
-func (cmd listUntrackedRepositories) Exec(flags *flag.FlagSet, cfg config.Config) error {
-	if flags.NArg() > 0 {
-		return unexpectedPositionalArgsError{Command: flags.Name()}
+func listUntrackedRepositoriesAction(appCtx *cli.Context) error {
+	logger := log.Default()
+	conf, err := getConfig(logger, appCtx.String(configFlagName))
+	if err != nil {
+		return err
 	}
 
-	ctx := correlation.ContextWithCorrelation(context.Background(), correlation.SafeRandomID())
-	ctx = metadata.AppendToOutgoingContext(ctx, "client_name", listUntrackedRepositoriesName)
+	onlyIncludeOlderThan := appCtx.Duration("older-than")
+	delimiter := appCtx.String("delimiter")
+
+	ctx := correlation.ContextWithCorrelation(appCtx.Context, correlation.SafeRandomID())
+	ctx = metadata.AppendToOutgoingContext(ctx, "client_name", appCtx.Command.Name)
 
-	logger := cmd.logger.WithField("correlation_id", correlation.ExtractFromContext(ctx))
-	logger.Debugf("starting %s command", flags.Name())
+	logger = logger.WithField("correlation_id", correlation.ExtractFromContext(ctx))
+	logger.Debugf("starting %s command", appCtx.App.Name)
 
 	logger.Debug("dialing to gitaly nodes...")
-	nodeSet, err := dialGitalyStorages(ctx, cfg, defaultDialTimeout)
+	nodeSet, err := dialGitalyStorages(ctx, conf, defaultDialTimeout)
 	if err != nil {
 		return fmt.Errorf("dial nodes: %w", err)
 	}
@@ -81,22 +81,22 @@ func (cmd listUntrackedRepositories) Exec(flags *flag.FlagSet, cfg config.Config
 	logger.Debug("connecting to praefect database...")
 	openDBCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
 	defer cancel()
-	db, err := glsql.OpenDB(openDBCtx, cfg.DB)
+	db, err := glsql.OpenDB(openDBCtx, conf.DB)
 	if err != nil {
 		return fmt.Errorf("connect to database: %w", err)
 	}
 	defer func() { _ = db.Close() }()
 	logger.Debug("connected to praefect database")
 
-	walker := repocleaner.NewWalker(nodeSet.Connections(), 16, cmd.onlyIncludeOlderThan)
+	walker := repocleaner.NewWalker(nodeSet.Connections(), 16, onlyIncludeOlderThan)
 	reporter := reportUntrackedRepositories{
 		ctx:         ctx,
 		checker:     datastore.NewStorageCleanup(db),
-		delimiter:   cmd.delimiter,
-		out:         cmd.out,
+		delimiter:   delimiter,
+		out:         appCtx.App.Writer,
 		printHeader: true,
 	}
-	for _, vs := range cfg.VirtualStorages {
+	for _, vs := range conf.VirtualStorages {
 		for _, node := range vs.Nodes {
 			logger.Debugf("check %q/%q storage repositories", vs.Name, node.Storage)
 			if err := walker.ExecOnRepositories(ctx, vs.Name, node.Storage, reporter.Report); err != nil {
@@ -108,6 +108,8 @@ func (cmd listUntrackedRepositories) Exec(flags *flag.FlagSet, cfg config.Config
 	return nil
 }
 
+var errNoConnectionToGitalies = errors.New("no connection established to gitaly nodes")
+
 func dialGitalyStorages(ctx context.Context, cfg config.Config, timeout time.Duration) (praefect.NodeSet, error) {
 	nodeSet := praefect.NodeSet{}
 	for _, vs := range cfg.VirtualStorages {
diff --git a/internal/cli/praefect/subcmd_list_untracked_repositories_test.go b/internal/cli/praefect/subcmd_list_untracked_repositories_test.go
index 96e44b0b8..e04624238 100644
--- a/internal/cli/praefect/subcmd_list_untracked_repositories_test.go
+++ b/internal/cli/praefect/subcmd_list_untracked_repositories_test.go
@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+	"github.com/urfave/cli/v2"
 	"gitlab.com/gitlab-org/gitaly/v15/client"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service/setup"
@@ -21,34 +22,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
 
-func TestListUntrackedRepositories_FlagSet(t *testing.T) {
-	t.Parallel()
-	cmd := &listUntrackedRepositories{}
-	for _, tc := range []struct {
-		desc string
-		args []string
-		exp  []interface{}
-	}{
-		{
-			desc: "custom values",
-			args: []string{"--delimiter", ",", "--older-than", "1s"},
-			exp:  []interface{}{",", time.Second},
-		},
-		{
-			desc: "default values",
-			args: nil,
-			exp:  []interface{}{"\n", 6 * time.Hour},
-		},
-	} {
-		t.Run(tc.desc, func(t *testing.T) {
-			fs := cmd.FlagSet()
-			require.NoError(t, fs.Parse(tc.args))
-			require.ElementsMatch(t, tc.exp, []interface{}{cmd.delimiter, cmd.onlyIncludeOlderThan})
-		})
-	}
-}
-
-func TestListUntrackedRepositories_Exec(t *testing.T) {
+func TestListUntrackedRepositoriesCommand(t *testing.T) {
 	t.Parallel()
 	g1Cfg := testcfg.Build(t, testcfg.WithStorages("gitaly-1"))
 	g2Cfg := testcfg.Build(t, testcfg.WithStorages("gitaly-2"))
@@ -75,6 +49,8 @@ func TestListUntrackedRepositories_Exec(t *testing.T) {
 		DB: dbConf,
 	}
 
+	confPath := writeConfigToFile(t, conf)
+
 	praefectServer := testserver.StartPraefect(t, conf)
 
 	cc, err := client.Dial(praefectServer.Address(), nil)
@@ -87,10 +63,6 @@ func TestListUntrackedRepositories_Exec(t *testing.T) {
 
 	// Repository managed by praefect, exists on gitaly-1 and gitaly-2.
 	createRepo(t, ctx, repoClient, praefectStorage, "path/to/test/repo")
-	out := &bytes.Buffer{}
-	cmd := newListUntrackedRepositories(testhelper.NewDiscardingLogger(t), out)
-	fs := cmd.FlagSet()
-	require.NoError(t, fs.Parse([]string{"-older-than", "4h"}))
 
 	// Repositories not managed by praefect.
 	repo1, repo1Path := gittest.CreateRepository(t, ctx, g1Cfg, gittest.CreateRepositoryConfig{
@@ -103,24 +75,61 @@ func TestListUntrackedRepositories_Exec(t *testing.T) {
 		SkipCreationViaService: true,
 	})
 
+	timeDelta := 4 * time.Hour
 	require.NoError(t, os.Chtimes(
 		repo1Path,
-		time.Now().Add(-(4*time.Hour+1*time.Second)),
-		time.Now().Add(-(4*time.Hour+1*time.Second))))
+		time.Now().Add(-(timeDelta+1*time.Second)),
+		time.Now().Add(-(timeDelta+1*time.Second))))
 	require.NoError(t, os.Chtimes(
 		repo2Path,
-		time.Now().Add(-(4*time.Hour+1*time.Second)),
-		time.Now().Add(-(4*time.Hour+1*time.Second))))
+		time.Now().Add(-(timeDelta+1*time.Second)),
+		time.Now().Add(-(timeDelta+1*time.Second))))
+
+	newApp := func() (cli.App, *bytes.Buffer) {
+		var stdout bytes.Buffer
+		return cli.App{
+			Writer: &stdout,
+			Commands: []*cli.Command{
+				newListUntrackedRepositoriesCommand(),
+			},
+			Flags: []cli.Flag{
+				&cli.StringFlag{
+					Name:  "config",
+					Value: confPath,
+				},
+			},
+		}, &stdout
+	}
+
+	t.Run("positional arguments", func(t *testing.T) {
+		app, _ := newApp()
+		err := app.Run([]string{progname, "list-untracked-repositories", "positional-arg"})
+		require.Equal(t, cli.Exit(unexpectedPositionalArgsError{Command: "list-untracked-repositories"}, 1), err)
+	})
 
-	require.NoError(t, cmd.Exec(fs, conf))
+	t.Run("default flag values used", func(t *testing.T) {
+		app, stdout := newApp()
+		err := app.Run([]string{progname, "list-untracked-repositories"})
+		require.NoError(t, err)
+		require.Empty(t, stdout.String())
+	})
 
-	exp := []string{
-		"The following repositories were found on disk, but missing from the tracking database:",
-		fmt.Sprintf(`{"relative_path":%q,"storage":"gitaly-1","virtual_storage":"praefect"}`, repo1.RelativePath),
-		fmt.Sprintf(`{"relative_path":%q,"storage":"gitaly-1","virtual_storage":"praefect"}`, repo2.RelativePath),
-		"", // an empty extra element required as each line ends with "delimiter" and strings.Split returns all parts
-	}
-	require.ElementsMatch(t, exp, strings.Split(out.String(), "\n"))
+	t.Run("passed flag values used", func(t *testing.T) {
+		app, stdout := newApp()
+		err := app.Run([]string{progname, "list-untracked-repositories", "-older-than", timeDelta.String(), "-delimiter", "~"})
+		require.NoError(t, err)
+
+		exp := []string{
+			"The following repositories were found on disk, but missing from the tracking database:",
+			fmt.Sprintf(`{"relative_path":%q,"storage":"gitaly-1","virtual_storage":"praefect"}`, repo1.RelativePath),
+			fmt.Sprintf(`{"relative_path":%q,"storage":"gitaly-1","virtual_storage":"praefect"}`, repo2.RelativePath),
+			"", // an empty extra element required as each line ends with "delimiter" and strings.Split returns all parts
+		}
+		elems := strings.Split(stdout.String(), "~")
+		require.Len(t, elems, len(exp)-1)
+		elems = append(elems[1:], strings.Split(elems[0], "\n")...)
+		require.ElementsMatch(t, exp, elems)
+	})
 }
 
 func createRepo(t *testing.T, ctx context.Context, repoClient gitalypb.RepositoryServiceClient, storageName, relativePath string) *gitalypb.Repository {