Merge branch 'ps-praefect-cli-metadata' into 'master'

praefect: migrate metadata sub-command

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

Merged-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Approved-by: Christian Couder <chriscool@tuxfamily.org>

4 files changed, 114 insertions, 64 deletions

diff --git a/internal/cli/praefect/main.go b/internal/cli/praefect/main.go
index ed4f4de30..6c159b6b8 100644
--- a/internal/cli/praefect/main.go
+++ b/internal/cli/praefect/main.go
@@ -80,6 +80,7 @@ func NewApp() *cli.App {
 			newListUntrackedRepositoriesCommand(),
 			newTrackRepositoryCommand(),
 			newVerifyCommand(),
+			newMetadataCommand(),
 		},
 		Flags: []cli.Flag{
 			&cli.StringFlag{
diff --git a/internal/cli/praefect/subcmd.go b/internal/cli/praefect/subcmd.go
index f9c46719e..1c0011736 100644
--- a/internal/cli/praefect/subcmd.go
+++ b/internal/cli/praefect/subcmd.go
@@ -40,7 +40,6 @@ func subcommands(logger *logrus.Entry) map[string]subcmd {
 		setReplicationFactorCmdName: newSetReplicatioFactorSubcommand(os.Stdout),
 		removeRepositoryCmdName:     newRemoveRepository(logger, os.Stdout),
 		trackRepositoriesCmdName:    newTrackRepositories(logger, os.Stdout),
-		metadataCmdName:             newMetadataSubcommand(os.Stdout),
 	}
 }
 
diff --git a/internal/cli/praefect/subcmd_metadata.go b/internal/cli/praefect/subcmd_metadata.go
index 1cce06899..30ef2d516 100644
--- a/internal/cli/praefect/subcmd_metadata.go
+++ b/internal/cli/praefect/subcmd_metadata.go
@@ -1,96 +1,109 @@
 package praefect
 
 import (
-	"context"
 	"errors"
-	"flag"
 	"fmt"
-	"io"
 
-	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/config"
+	"github.com/urfave/cli/v2"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
 )
 
-const metadataCmdName = "metadata"
-
-type metadataSubcommand struct {
-	stdout         io.Writer
-	repositoryID   int64
-	virtualStorage string
-	relativePath   string
-}
-
-func newMetadataSubcommand(stdout io.Writer) *metadataSubcommand {
-	return &metadataSubcommand{stdout: stdout}
-}
-
-func (cmd *metadataSubcommand) FlagSet() *flag.FlagSet {
-	fs := flag.NewFlagSet(metadataCmdName, flag.ContinueOnError)
-	fs.Int64Var(&cmd.repositoryID, "repository-id", 0, "the repository's ID")
-	fs.StringVar(&cmd.virtualStorage, "virtual-storage", "", "the repository's virtual storage")
-	fs.StringVar(&cmd.relativePath, "relative-path", "", "the repository's relative path in the virtual storage")
-	return fs
-}
-
-func (cmd *metadataSubcommand) println(format string, args ...interface{}) {
-	fmt.Fprintf(cmd.stdout, format+"\n", args...)
+func newMetadataCommand() *cli.Command {
+	return &cli.Command{
+		Name:  "metadata",
+		Usage: "shows metadata information about repository",
+		Description: "The command provides metadata information about the repository. It includes " +
+			"identifier of the repository, path on the disk for it and it's replicas, information " +
+			"about replicas such as if it is assigned or not, its generation, health state, the storage, " +
+			"if it is a valid primary, etc. It can be invoked by providing repository identifier or " +
+			"virtual repository name and relative path.",
+		HideHelpCommand: true,
+		Action:          metadataAction,
+		Flags: []cli.Flag{
+			&cli.Int64Flag{
+				Name:  "repository-id",
+				Usage: "the repository's ID",
+			},
+			&cli.StringFlag{
+				Name:  paramVirtualStorage,
+				Usage: "the repository's virtual storage",
+			},
+			&cli.StringFlag{
+				Name:  "relative-path",
+				Usage: "the repository's relative path in the virtual storage",
+			},
+		},
+		Before: func(ctx *cli.Context) error {
+			if ctx.Args().Present() {
+				_ = cli.ShowSubcommandHelp(ctx)
+				return cli.Exit(unexpectedPositionalArgsError{Command: ctx.Command.Name}, 1)
+			}
+			return nil
+		},
+	}
 }
 
-func (cmd *metadataSubcommand) Exec(flags *flag.FlagSet, cfg config.Config) error {
-	if flags.NArg() > 0 {
-		return unexpectedPositionalArgsError{Command: flags.Name()}
+func metadataAction(appCtx *cli.Context) error {
+	logger := log.Default()
+	conf, err := getConfig(logger, appCtx.String(configFlagName))
+	if err != nil {
+		return err
 	}
 
+	repositoryID := appCtx.Int64("repository-id")
+	virtualStorage := appCtx.String(paramVirtualStorage)
+	relativePath := appCtx.String("relative-path")
+
 	var request gitalypb.GetRepositoryMetadataRequest
 	switch {
-	case cmd.repositoryID != 0:
-		if cmd.virtualStorage != "" || cmd.relativePath != "" {
+	case repositoryID != 0:
+		if virtualStorage != "" || relativePath != "" {
 			return errors.New("virtual storage and relative path can't be provided with a repository ID")
 		}
-		request.Query = &gitalypb.GetRepositoryMetadataRequest_RepositoryId{RepositoryId: cmd.repositoryID}
-	case cmd.virtualStorage != "" || cmd.relativePath != "":
-		if cmd.virtualStorage == "" {
+		request.Query = &gitalypb.GetRepositoryMetadataRequest_RepositoryId{RepositoryId: repositoryID}
+	case virtualStorage != "" || relativePath != "":
+		if virtualStorage == "" {
 			return errors.New("virtual storage is required with relative path")
-		} else if cmd.relativePath == "" {
+		} else if relativePath == "" {
 			return errors.New("relative path is required with virtual storage")
 		}
 		request.Query = &gitalypb.GetRepositoryMetadataRequest_Path_{
 			Path: &gitalypb.GetRepositoryMetadataRequest_Path{
-				VirtualStorage: cmd.virtualStorage,
-				RelativePath:   cmd.relativePath,
+				VirtualStorage: virtualStorage,
+				RelativePath:   relativePath,
 			},
 		}
 	default:
 		return errors.New("repository id or virtual storage and relative path required")
 	}
 
-	nodeAddr, err := getNodeAddress(cfg)
+	nodeAddr, err := getNodeAddress(conf)
 	if err != nil {
 		return fmt.Errorf("get node address: %w", err)
 	}
 
-	ctx := context.TODO()
-	conn, err := subCmdDial(ctx, nodeAddr, cfg.Auth.Token, defaultDialTimeout)
+	conn, err := subCmdDial(appCtx.Context, nodeAddr, conf.Auth.Token, defaultDialTimeout)
 	if err != nil {
 		return fmt.Errorf("dial: %w", err)
 	}
 	defer conn.Close()
 
-	metadata, err := gitalypb.NewPraefectInfoServiceClient(conn).GetRepositoryMetadata(ctx, &request)
+	metadata, err := gitalypb.NewPraefectInfoServiceClient(conn).GetRepositoryMetadata(appCtx.Context, &request)
 	if err != nil {
 		return fmt.Errorf("get metadata: %w", err)
 	}
 
-	cmd.println("Repository ID: %d", metadata.RepositoryId)
-	cmd.println("Virtual Storage: %q", metadata.VirtualStorage)
-	cmd.println("Relative Path: %q", metadata.RelativePath)
-	cmd.println("Replica Path: %q", metadata.ReplicaPath)
-	cmd.println("Primary: %q", metadata.Primary)
-	cmd.println("Generation: %d", metadata.Generation)
-	cmd.println("Replicas:")
+	fmt.Fprintf(appCtx.App.Writer, "Repository ID: %d\n", metadata.RepositoryId)
+	fmt.Fprintf(appCtx.App.Writer, "Virtual Storage: %q\n", metadata.VirtualStorage)
+	fmt.Fprintf(appCtx.App.Writer, "Relative Path: %q\n", metadata.RelativePath)
+	fmt.Fprintf(appCtx.App.Writer, "Replica Path: %q\n", metadata.ReplicaPath)
+	fmt.Fprintf(appCtx.App.Writer, "Primary: %q\n", metadata.Primary)
+	fmt.Fprintf(appCtx.App.Writer, "Generation: %d\n", metadata.Generation)
+	fmt.Fprintf(appCtx.App.Writer, "Replicas:\n")
 	for _, replica := range metadata.Replicas {
-		cmd.println("- Storage: %q", replica.Storage)
-		cmd.println("  Assigned: %v", replica.Assigned)
+		fmt.Fprintf(appCtx.App.Writer, "- Storage: %q\n", replica.Storage)
+		fmt.Fprintf(appCtx.App.Writer, "  Assigned: %v\n", replica.Assigned)
 
 		generationText := fmt.Sprintf("%d, fully up to date", replica.Generation)
 		if replica.Generation == -1 {
@@ -104,10 +117,10 @@ func (cmd *metadataSubcommand) Exec(flags *flag.FlagSet, cfg config.Config) erro
 			verifiedAt = replica.VerifiedAt.AsTime().String()
 		}
 
-		cmd.println("  Generation: %s", generationText)
-		cmd.println("  Healthy: %v", replica.Healthy)
-		cmd.println("  Valid Primary: %v", replica.ValidPrimary)
-		cmd.println("  Verified At: %s", verifiedAt)
+		fmt.Fprintf(appCtx.App.Writer, "  Generation: %s\n", generationText)
+		fmt.Fprintf(appCtx.App.Writer, "  Healthy: %v\n", replica.Healthy)
+		fmt.Fprintf(appCtx.App.Writer, "  Valid Primary: %v\n", replica.ValidPrimary)
+		fmt.Fprintf(appCtx.App.Writer, "  Verified At: %s\n", verifiedAt)
 	}
 	return nil
 }
diff --git a/internal/cli/praefect/subcmd_metadata_test.go b/internal/cli/praefect/subcmd_metadata_test.go
index bfce0350d..05db30b7e 100644
--- a/internal/cli/praefect/subcmd_metadata_test.go
+++ b/internal/cli/praefect/subcmd_metadata_test.go
@@ -4,10 +4,12 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
+	"io"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/require"
+	"github.com/urfave/cli/v2"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/service/info"
@@ -22,7 +24,7 @@ func TestMetadataSubcommand(t *testing.T) {
 	ctx := testhelper.Context(t)
 
 	tx := testdb.New(t).Begin(t)
-	defer tx.Rollback(t)
+	t.Cleanup(func() { tx.Rollback(t) })
 
 	testdb.SetHealthyNodes(t, ctx, tx, map[string]map[string][]string{
 		"praefect": {"virtual-storage": {"primary", "secondary-1"}},
@@ -42,7 +44,23 @@ func TestMetadataSubcommand(t *testing.T) {
 	ln, clean := listenAndServe(t, []svcRegistrar{
 		registerPraefectInfoServer(info.NewServer(config.Config{}, rs, nil, nil, nil)),
 	})
-	defer clean()
+	t.Cleanup(clean)
+
+	conf := config.Config{
+		SocketPath: ln.Addr().String(),
+		VirtualStorages: []*config.VirtualStorage{
+			{
+				Name: "vs-1",
+				Nodes: []*config.Node{
+					{
+						Storage: "storage-1",
+						Address: "tcp://1.2.3.4",
+					},
+				},
+			},
+		},
+	}
+	confPath := writeConfigToFile(t, conf)
 
 	for _, tc := range []struct {
 		desc  string
@@ -50,6 +68,11 @@ func TestMetadataSubcommand(t *testing.T) {
 		error error
 	}{
 		{
+			desc:  "positional arguments",
+			args:  []string{"positional-arg"},
+			error: cli.Exit(unexpectedPositionalArgsError{Command: "metadata"}, 1),
+		},
+		{
 			desc:  "missing parameters fails",
 			error: errors.New("repository id or virtual storage and relative path required"),
 		},
@@ -87,14 +110,28 @@ func TestMetadataSubcommand(t *testing.T) {
 			args: []string{"-virtual-storage=virtual-storage", "-relative-path=relative-path"},
 		},
 	} {
+		tc := tc
 		t.Run(tc.desc, func(t *testing.T) {
-			stdout := &bytes.Buffer{}
-			cmd := newMetadataSubcommand(stdout)
+			t.Parallel()
 
-			fs := cmd.FlagSet()
-			require.NoError(t, fs.Parse(tc.args))
-			err := cmd.Exec(fs, config.Config{SocketPath: ln.Addr().String()})
-			testhelper.RequireGrpcError(t, tc.error, err)
+			var stdout bytes.Buffer
+			app := cli.App{
+				Reader:          bytes.NewReader(nil),
+				Writer:          &stdout,
+				ErrWriter:       io.Discard,
+				HideHelpCommand: true,
+				Commands: []*cli.Command{
+					newMetadataCommand(),
+				},
+				Flags: []cli.Flag{
+					&cli.StringFlag{
+						Name:  "config",
+						Value: confPath,
+					},
+				},
+			}
+			err := app.Run(append([]string{progname, "metadata"}, tc.args...))
+			require.Equal(t, tc.error, err)
 			if tc.error != nil {
 				return
 			}