praefect: migrate list-untracked-repositories sub-command

The 'list-untracked-repositories' sub-command migrated from the old
implementation to the new approach with usage of the
third party library. The invocation of the command remains
the same, but it has more descriptive representation when
help is invoked. Also, it is possible now to invoke help
only for the sub-command to see more details about it and
its usage.

Part of: #5001

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 {