gitaly-backup: Start switch to the new cli implementation.gg-gitaly-backup-cli

gitaly-backup has two subcommands which have not ben migrated yet.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/5430

6 files changed, 232 insertions, 86 deletions

diff --git a/cmd/gitaly-backup/main.go b/cmd/gitaly-backup/main.go
index 194301d54..203eb59cc 100644
--- a/cmd/gitaly-backup/main.go
+++ b/cmd/gitaly-backup/main.go
@@ -1,7 +1,14 @@
 package main
 
-import "gitlab.com/gitlab-org/gitaly/v16/internal/cli/gitalybackup"
+import (
+	"log"
+	"os"
+
+	cli "gitlab.com/gitlab-org/gitaly/v16/internal/cli/gitalybackup"
+)
 
 func main() {
-	gitalybackup.Main()
+	if err := cli.NewApp().Run(os.Args); err != nil {
+		log.Fatal(err)
+	}
 }
diff --git a/internal/cli/gitalybackup/create.go b/internal/cli/gitalybackup/create.go
index 061be231d..cd0a0531d 100644
--- a/internal/cli/gitalybackup/create.go
+++ b/internal/cli/gitalybackup/create.go
@@ -3,12 +3,13 @@ package gitalybackup
 import (
 	"context"
 	"encoding/json"
-	"flag"
 	"fmt"
 	"io"
+	"os"
 	"runtime"
 	"time"
 
+	cli "github.com/urfave/cli/v2"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/backup"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/grpc/client"
@@ -33,14 +34,86 @@ type createSubcommand struct {
 	serverSide      bool
 }
 
-func (cmd *createSubcommand) Flags(fs *flag.FlagSet) {
-	fs.StringVar(&cmd.backupPath, "path", "", "repository backup path")
-	fs.IntVar(&cmd.parallel, "parallel", runtime.NumCPU(), "maximum number of parallel backups")
-	fs.IntVar(&cmd.parallelStorage, "parallel-storage", 2, "maximum number of parallel backups per storage. Note: actual parallelism when combined with `-parallel` depends on the order the repositories are received.")
-	fs.StringVar(&cmd.layout, "layout", "pointer", "how backup files are located. Either pointer or legacy.")
-	fs.BoolVar(&cmd.incremental, "incremental", false, "creates an incremental backup if possible.")
-	fs.StringVar(&cmd.backupID, "id", time.Now().UTC().Format("20060102150405"), "the backup ID used when creating a full backup.")
-	fs.BoolVar(&cmd.serverSide, "server-side", false, "use server-side backups. Note: The feature is not ready for production use.")
+func (cmd *createSubcommand) Flags(ctx *cli.Context) {
+	cmd.backupPath = ctx.String("path")
+	cmd.parallel = ctx.Int("parallel")
+	cmd.parallelStorage = ctx.Int("parallel-storage")
+	cmd.layout = ctx.String("layout")
+	cmd.incremental = ctx.Bool("incremental")
+	cmd.backupID = ctx.String("id")
+	cmd.serverSide = ctx.Bool("server-side")
+}
+
+func createFlags() []cli.Flag {
+	return []cli.Flag{
+		&cli.StringFlag{
+			Name:  "path",
+			Usage: "repository backup path",
+		},
+		&cli.IntFlag{
+			Name:  "parallel",
+			Usage: "maximum number of parallel backups",
+			Value: runtime.NumCPU(),
+		},
+		&cli.IntFlag{
+			Name:  "parallel-storage",
+			Usage: "maximum number of parallel backups per storage. Note: actual parallelism when combined with `-parallel` depends on the order the repositories are received.",
+			Value: 2,
+		},
+		&cli.StringFlag{
+			Name:  "layout",
+			Usage: "how backup files are located. Either pointer or legacy.",
+			Value: "pointer",
+		},
+		&cli.BoolFlag{
+			Name:  "incremental",
+			Usage: "creates an incremental backup if possible.",
+			Value: false,
+		},
+		&cli.StringFlag{
+			Name:  "id",
+			Usage: "the backup ID used when creating a full backup.",
+			Value: time.Now().UTC().Format("20060102150405"),
+		},
+		&cli.BoolFlag{
+			Name:  "server-side",
+			Usage: "use server-side backups. Note: The feature is not ready for production use.",
+			Value: false,
+		},
+	}
+}
+
+func newCreateCommand() *cli.Command {
+	return &cli.Command{
+		Name:   "create",
+		Usage:  "Create backup file",
+		Action: createAction,
+		Flags:  createFlags(),
+	}
+}
+
+func createAction(cctx *cli.Context) error {
+	logger, err := log.Configure(os.Stdout, "json", "")
+	if err != nil {
+		fmt.Printf("configuring logger failed: %v", err)
+		os.Exit(1)
+	}
+
+	ctx, err := storage.InjectGitalyServersEnv(context.Background())
+	if err != nil {
+		logger.Error(err.Error())
+		os.Exit(1)
+	}
+
+	subcmd := createSubcommand{}
+
+	subcmd.Flags(cctx)
+
+	if err := subcmd.Run(ctx, logger, os.Stdin, os.Stdout); err != nil {
+		logger.Error(err.Error())
+		os.Exit(1)
+	}
+	return nil
 }
 
 func (cmd *createSubcommand) Run(ctx context.Context, logger log.Logger, stdin io.Reader, stdout io.Writer) error {
diff --git a/internal/cli/gitalybackup/create_test.go b/internal/cli/gitalybackup/create_test.go
index 2ae6c5653..a95e16a9a 100644
--- a/internal/cli/gitalybackup/create_test.go
+++ b/internal/cli/gitalybackup/create_test.go
@@ -3,9 +3,9 @@ package gitalybackup
 import (
 	"bytes"
 	"encoding/json"
-	"flag"
 	"io"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -56,10 +56,12 @@ func TestCreateSubcommand(t *testing.T) {
 
 	cmd := createSubcommand{backupPath: path}
 
-	fs := flag.NewFlagSet("create", flag.ContinueOnError)
-	cmd.Flags(fs)
+	cmd.backupPath = path
+	cmd.parallel = runtime.NumCPU()
+	cmd.parallelStorage = 2
+	cmd.layout = "pointer"
+	cmd.backupID = "the-new-backup"
 
-	require.NoError(t, fs.Parse([]string{"-path", path, "-id", "the-new-backup"}))
 	require.EqualError(t,
 		cmd.Run(ctx, testhelper.SharedLogger(t), &stdin, io.Discard),
 		"create: pipeline: 1 failures encountered:\n - invalid: manager: could not dial source: invalid connection string: \"invalid\"\n")
@@ -113,10 +115,12 @@ func TestCreateSubcommand_serverSide(t *testing.T) {
 	}))
 
 	cmd := createSubcommand{}
-	fs := flag.NewFlagSet("create", flag.ContinueOnError)
-	cmd.Flags(fs)
+	cmd.parallel = runtime.NumCPU()
+	cmd.parallelStorage = 2
+	cmd.layout = "pointer"
+	cmd.backupID = "the-new-backup"
+	cmd.serverSide = true
 
-	require.NoError(t, fs.Parse([]string{"-server-side", "-id", "the-new-backup"}))
 	require.EqualError(t,
 		cmd.Run(ctx, testhelper.SharedLogger(t), &stdin, io.Discard),
 		"create: pipeline: 1 failures encountered:\n - invalid: server-side create: could not dial source: invalid connection string: \"invalid\"\n")
diff --git a/internal/cli/gitalybackup/main.go b/internal/cli/gitalybackup/main.go
index 3dd3a348a..d6acd0985 100644
--- a/internal/cli/gitalybackup/main.go
+++ b/internal/cli/gitalybackup/main.go
@@ -1,61 +1,52 @@
 package gitalybackup
 
 import (
-	"context"
-	"flag"
 	"fmt"
-	"io"
-	"os"
 
-	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
+	"github.com/urfave/cli/v2"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/version"
 )
 
-type subcmd interface {
-	Flags(*flag.FlagSet)
-	Run(ctx context.Context, logger log.Logger, stdin io.Reader, stdout io.Writer) error
-}
-
-var subcommands = map[string]subcmd{
-	"create":  &createSubcommand{},
-	"restore": &restoreSubcommand{},
-}
-
-// Main is an entry point of the gitaly-backup binary.
-func Main() {
-	logger, err := log.Configure(os.Stdout, "json", "")
-	if err != nil {
-		fmt.Printf("configuring logger failed: %v", err)
-		os.Exit(1)
-	}
-
-	flags := flag.NewFlagSet("gitaly-backup", flag.ExitOnError)
-	_ = flags.Parse(os.Args)
-
-	if flags.NArg() < 2 {
-		logger.Error("missing subcommand")
-		os.Exit(1)
-	}
-
-	subcmdName := flags.Arg(1)
-	subcmd, ok := subcommands[subcmdName]
-	if !ok {
-		logger.Error(fmt.Sprintf("unknown subcommand: %q", flags.Arg(1)))
-		os.Exit(1)
+func init() {
+	// Override the version printer so the output format matches what Praefect
+	// used before the introduction of the CLI toolkit.
+	cli.VersionPrinter = func(ctx *cli.Context) {
+		fmt.Fprintln(ctx.App.Writer, version.GetVersionString(binaryName))
 	}
+}
 
-	subcmdFlags := flag.NewFlagSet(subcmdName, flag.ExitOnError)
-	subcmd.Flags(subcmdFlags)
-	_ = subcmdFlags.Parse(flags.Args()[2:])
+const (
+	progname = "gitaly-backup"
 
-	ctx, err := storage.InjectGitalyServersEnv(context.Background())
-	if err != nil {
-		logger.Error(err.Error())
-		os.Exit(1)
-	}
+	pathFlagName = "path"
+	binaryName   = "Gitaly Backup"
+)
 
-	if err := subcmd.Run(ctx, logger, os.Stdin, os.Stdout); err != nil {
-		logger.Error(err.Error())
-		os.Exit(1)
+// NewApp returns a new praefect app.
+func NewApp() *cli.App {
+	return &cli.App{
+		Name:    progname,
+		Usage:   "create gitaly backups",
+		Version: version.GetVersionString(binaryName),
+		// serveAction is also here in the root to keep the CLI backwards compatible with
+		// the previous way to launch Praefect with just `praefect -config FILE`.
+		// We may want to deprecate this eventually.
+		//
+		// The 'DefaultCommand: "serve"' setting can't be used here because it won't be
+		// possible to invoke sub-command not yet registered.
+		// Action: serveAction,
+		Commands: []*cli.Command{
+			newCreateCommand(),
+			newRestoreCommand(),
+		},
+		Flags: []cli.Flag{
+			&cli.StringFlag{
+				// We can't mark it required, because it is not for all sub-commands.
+				// We need it as it is used by majority of the sub-commands and
+				// because of the existing format of commands invocation.
+				Name:  pathFlagName,
+				Usage: "Directory where the backup files will be created/restored.",
+			},
+		},
 	}
 }
diff --git a/internal/cli/gitalybackup/restore.go b/internal/cli/gitalybackup/restore.go
index 9bff6fe4f..628227341 100644
--- a/internal/cli/gitalybackup/restore.go
+++ b/internal/cli/gitalybackup/restore.go
@@ -4,12 +4,12 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
-	"flag"
 	"fmt"
 	"io"
+	"os"
 	"runtime"
-	"strings"
 
+	cli "github.com/urfave/cli/v2"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/backup"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/grpc/client"
@@ -35,17 +35,84 @@ type restoreSubcommand struct {
 	serverSide            bool
 }
 
-func (cmd *restoreSubcommand) Flags(fs *flag.FlagSet) {
-	fs.StringVar(&cmd.backupPath, "path", "", "repository backup path")
-	fs.IntVar(&cmd.parallel, "parallel", runtime.NumCPU(), "maximum number of parallel restores")
-	fs.IntVar(&cmd.parallelStorage, "parallel-storage", 2, "maximum number of parallel restores per storage. Note: actual parallelism when combined with `-parallel` depends on the order the repositories are received.")
-	fs.StringVar(&cmd.layout, "layout", "pointer", "how backup files are located. Either pointer or legacy.")
-	fs.Func("remove-all-repositories", "comma-separated list of storage names to have all repositories removed from before restoring.", func(removeAll string) error {
-		cmd.removeAllRepositories = strings.Split(removeAll, ",")
-		return nil
-	})
-	fs.StringVar(&cmd.backupID, "id", "", "ID of full backup to restore. If not specified, the latest backup is restored.")
-	fs.BoolVar(&cmd.serverSide, "server-side", false, "use server-side backups. Note: The feature is not ready for production use.")
+func (cmd *restoreSubcommand) Flags(ctx *cli.Context) {
+	cmd.backupPath = ctx.String("path")
+	cmd.parallel = ctx.Int("parallel")
+	cmd.parallelStorage = ctx.Int("parallel-storage")
+	cmd.layout = ctx.String("layout")
+	cmd.removeAllRepositories = ctx.StringSlice("remove-all-repositories")
+	cmd.backupID = ctx.String("id")
+	cmd.serverSide = ctx.Bool("server-side")
+}
+
+func restoreFlags() []cli.Flag {
+	return []cli.Flag{
+		&cli.StringFlag{
+			Name:  "path",
+			Usage: "repository backup path",
+		},
+		&cli.IntFlag{
+			Name:  "parallel",
+			Usage: "maximum number of parallel backups",
+			Value: runtime.NumCPU(),
+		},
+		&cli.IntFlag{
+			Name:  "parallel-storage",
+			Usage: "maximum number of parallel backups per storage. Note: actual parallelism when combined with `-parallel` depends on the order the repositories are received.",
+			Value: 2,
+		},
+		&cli.StringFlag{
+			Name:  "layout",
+			Usage: "how backup files are located. Either pointer or legacy.",
+			Value: "pointer",
+		},
+		&cli.StringSliceFlag{
+			Name:  "remove-all-repositories",
+			Usage: "comma-separated list of storage names to have all repositories removed from before restoring.",
+		},
+		&cli.StringFlag{
+			Name:  "id",
+			Usage: "ID of full backup to restore. If not specified, the latest backup is restored.",
+		},
+		&cli.BoolFlag{
+			Name:  "server-side",
+			Usage: "use server-side backups. Note: The feature is not ready for production use.",
+			Value: false,
+		},
+	}
+}
+
+func newRestoreCommand() *cli.Command {
+	return &cli.Command{
+		Name:   "restore",
+		Usage:  "Restore backup file",
+		Action: restoreAction,
+		Flags:  restoreFlags(),
+	}
+}
+
+func restoreAction(cctx *cli.Context) error {
+	logger, err := log.Configure(os.Stdout, "json", "")
+	if err != nil {
+		fmt.Printf("configuring logger failed: %v", err)
+		os.Exit(1)
+	}
+
+	ctx, err := storage.InjectGitalyServersEnv(context.Background())
+	if err != nil {
+		logger.Error(err.Error())
+		os.Exit(1)
+	}
+
+	subcmd := restoreSubcommand{}
+	subcmd.Flags(cctx)
+	fmt.Print(cctx)
+
+	if err := subcmd.Run(ctx, logger, os.Stdin, os.Stdout); err != nil {
+		logger.Error(err.Error())
+		os.Exit(1)
+	}
+	return nil
 }
 
 func (cmd *restoreSubcommand) Run(ctx context.Context, logger log.Logger, stdin io.Reader, stdout io.Writer) error {
diff --git a/internal/cli/gitalybackup/restore_test.go b/internal/cli/gitalybackup/restore_test.go
index 77072e9ec..95dc11328 100644
--- a/internal/cli/gitalybackup/restore_test.go
+++ b/internal/cli/gitalybackup/restore_test.go
@@ -3,11 +3,11 @@ package gitalybackup
 import (
 	"bytes"
 	"encoding/json"
-	"flag"
 	"fmt"
 	"io"
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -84,12 +84,14 @@ Issue: https://gitlab.com/gitlab-org/gitaly/-/issues/5269`)
 	ctx = testhelper.MergeIncomingMetadata(ctx, testcfg.GitalyServersMetadataFromCfg(t, cfg))
 	cmd := restoreSubcommand{}
 
-	fs := flag.NewFlagSet("restore", flag.ContinueOnError)
-	cmd.Flags(fs)
+	cmd.backupPath = path
+	cmd.parallel = runtime.NumCPU()
+	cmd.parallelStorage = 2
+	cmd.layout = "pointer"
+	cmd.removeAllRepositories = append(cmd.removeAllRepositories, existingRepo.StorageName)
 
 	require.DirExists(t, existRepoPath)
 
-	require.NoError(t, fs.Parse([]string{"-path", path, "-remove-all-repositories", existingRepo.StorageName}))
 	require.EqualError(t,
 		cmd.Run(ctx, testhelper.SharedLogger(t), &stdin, io.Discard),
 		"restore: pipeline: 1 failures encountered:\n - invalid: manager: could not dial source: invalid connection string: \"invalid\"\n")
@@ -176,12 +178,14 @@ Issue: https://gitlab.com/gitlab-org/gitaly/-/issues/5269`)
 	ctx = testhelper.MergeIncomingMetadata(ctx, testcfg.GitalyServersMetadataFromCfg(t, cfg))
 	cmd := restoreSubcommand{}
 
-	fs := flag.NewFlagSet("restore", flag.ContinueOnError)
-	cmd.Flags(fs)
+	cmd.parallel = runtime.NumCPU()
+	cmd.parallelStorage = 2
+	cmd.layout = "pointer"
+	cmd.removeAllRepositories = append(cmd.removeAllRepositories, existingRepo.StorageName)
+	cmd.serverSide = true
 
 	require.DirExists(t, existRepoPath)
 
-	require.NoError(t, fs.Parse([]string{"-server-side", "-remove-all-repositories", existingRepo.StorageName}))
 	require.EqualError(t,
 		cmd.Run(ctx, testhelper.SharedLogger(t), &stdin, io.Discard),
 		"restore: pipeline: 1 failures encountered:\n - invalid: server-side restore: could not dial source: invalid connection string: \"invalid\"\n")