gitaly-backup: Migrate command to `urfave/cli` framework

6 files changed, 243 insertions, 96 deletions

diff --git a/cmd/gitaly-backup/main.go b/cmd/gitaly-backup/main.go
index 194301d54..171e45476 100644
--- a/cmd/gitaly-backup/main.go
+++ b/cmd/gitaly-backup/main.go
@@ -1,7 +1,13 @@
 package main
 
-import "gitlab.com/gitlab-org/gitaly/v16/internal/cli/gitalybackup"
+import (
+	"os"
+
+	cli "gitlab.com/gitlab-org/gitaly/v16/internal/cli/gitalybackup"
+)
 
 func main() {
-	gitalybackup.Main()
+	if err := cli.NewApp().Run(os.Args); err != nil {
+		os.Exit(1)
+	}
 }
diff --git a/internal/cli/gitalybackup/create.go b/internal/cli/gitalybackup/create.go
index 061be231d..a67cc75da 100644
--- a/internal/cli/gitalybackup/create.go
+++ b/internal/cli/gitalybackup/create.go
@@ -3,12 +3,12 @@ package gitalybackup
 import (
 	"context"
 	"encoding/json"
-	"flag"
 	"fmt"
 	"io"
 	"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,17 +33,89 @@ 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 (cmd *createSubcommand) Run(ctx context.Context, logger log.Logger, stdin io.Reader, stdout io.Writer) error {
+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(cctx.App.Writer, "json", "")
+	if err != nil {
+		fmt.Printf("configuring logger failed: %v", err)
+		return err
+	}
+
+	ctx, err := storage.InjectGitalyServersEnv(context.Background())
+	if err != nil {
+		logger.Error(err.Error())
+		return err
+	}
+
+	subcmd := createSubcommand{}
+
+	subcmd.flags(cctx)
+
+	if err := subcmd.run(ctx, logger, cctx.App.Reader); err != nil {
+		logger.Error(err.Error())
+		return err
+	}
+	return nil
+}
+
+func (cmd *createSubcommand) run(ctx context.Context, logger log.Logger, stdin io.Reader) error {
 	pool := client.NewPool(client.WithDialOptions(client.UnaryInterceptor(), client.StreamInterceptor()))
 	defer func() {
 		_ = pool.Close()
diff --git a/internal/cli/gitalybackup/create_test.go b/internal/cli/gitalybackup/create_test.go
index 2ae10c07a..f4d5c71d8 100644
--- a/internal/cli/gitalybackup/create_test.go
+++ b/internal/cli/gitalybackup/create_test.go
@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
-	"flag"
 	"io"
 	"path/filepath"
 	"strings"
@@ -31,7 +30,7 @@ func TestCreateSubcommand(t *testing.T) {
 		{
 			Name: "when a local backup is created",
 			Flags: func(backupRoot string) []string {
-				return []string{"-path", backupRoot, "-id", "the-new-backup"}
+				return []string{"--path", backupRoot, "--id", "the-new-backup"}
 			},
 			ServerOpts: func(ctx context.Context, backupRoot string) []testserver.GitalyServerOpt {
 				return nil
@@ -41,7 +40,7 @@ func TestCreateSubcommand(t *testing.T) {
 		{
 			Name: "when a server-side backup is created",
 			Flags: func(path string) []string {
-				return []string{"-server-side", "-id", "the-new-backup"}
+				return []string{"--server-side", "--id", "the-new-backup"}
 			},
 			ServerOpts: func(ctx context.Context, backupRoot string) []testserver.GitalyServerOpt {
 				backupSink, err := backup.ResolveSink(ctx, backupRoot)
@@ -60,7 +59,7 @@ func TestCreateSubcommand(t *testing.T) {
 		{
 			Name: "when a server-side incremental backup is created",
 			Flags: func(path string) []string {
-				return []string{"-server-side", "-incremental", "-id", "the-new-backup"}
+				return []string{"--server-side", "--incremental", "--id", "the-new-backup"}
 			},
 			ServerOpts: func(ctx context.Context, backupRoot string) []testserver.GitalyServerOpt {
 				backupSink, err := backup.ResolveSink(ctx, backupRoot)
@@ -112,13 +111,14 @@ func TestCreateSubcommand(t *testing.T) {
 				"relative_path": "invalid",
 			}))
 
-			cmd := createSubcommand{}
-			fs := flag.NewFlagSet("create", flag.ContinueOnError)
-			cmd.Flags(fs)
-			require.NoError(t, fs.Parse(tc.Flags(path)))
+			args := append([]string{progname, "create"}, tc.Flags(path)...)
+			args = append(args, "--layout", "pointer")
+			cmd := NewApp()
+			cmd.Reader = &stdin
+			cmd.Writer = io.Discard
 
 			require.EqualError(t,
-				cmd.Run(ctx, testhelper.SharedLogger(t), &stdin, io.Discard),
+				cmd.RunContext(ctx, args),
 				tc.ExpectedErrMessage)
 
 			for _, repo := range repos {
diff --git a/internal/cli/gitalybackup/main.go b/internal/cli/gitalybackup/main.go
index 3dd3a348a..22825f662 100644
--- a/internal/cli/gitalybackup/main.go
+++ b/internal/cli/gitalybackup/main.go
@@ -1,61 +1,40 @@
 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() {
+	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 gitaly[backup app.
+func NewApp() *cli.App {
+	return &cli.App{
+		Name:    progname,
+		Usage:   "create gitaly backups",
+		Version: version.GetVersionString(binaryName),
+		Commands: []*cli.Command{
+			newCreateCommand(),
+			newRestoreCommand(),
+		},
+		Flags: []cli.Flag{
+			&cli.StringFlag{
+				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..06a20bb6e 100644
--- a/internal/cli/gitalybackup/restore.go
+++ b/internal/cli/gitalybackup/restore.go
@@ -4,12 +4,11 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
-	"flag"
 	"fmt"
 	"io"
 	"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,20 +34,86 @@ 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 (cmd *restoreSubcommand) Run(ctx context.Context, logger log.Logger, stdin io.Reader, stdout io.Writer) error {
+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(cctx.App.Writer, "json", "")
+	if err != nil {
+		fmt.Printf("configuring logger failed: %v", err)
+		return err
+	}
+
+	ctx, err := storage.InjectGitalyServersEnv(cctx.Context)
+	if err != nil {
+		logger.Error(err.Error())
+		return err
+	}
+
+	subcmd := restoreSubcommand{}
+	subcmd.flags(cctx)
+
+	if err := subcmd.run(ctx, logger, cctx.App.Reader); err != nil {
+		logger.Error(err.Error())
+		return err
+	}
+	return nil
+}
+
+func (cmd *restoreSubcommand) run(ctx context.Context, logger log.Logger, stdin io.Reader) error {
 	pool := client.NewPool(client.WithDialOptions(client.UnaryInterceptor(), client.StreamInterceptor()))
 	defer func() {
 		_ = pool.Close()
diff --git a/internal/cli/gitalybackup/restore_test.go b/internal/cli/gitalybackup/restore_test.go
index 77072e9ec..4eae52e4c 100644
--- a/internal/cli/gitalybackup/restore_test.go
+++ b/internal/cli/gitalybackup/restore_test.go
@@ -3,11 +3,12 @@ package gitalybackup
 import (
 	"bytes"
 	"encoding/json"
-	"flag"
 	"fmt"
 	"io"
 	"os"
 	"path/filepath"
+	"runtime"
+	"strconv"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -32,7 +33,6 @@ the database and only then perform the removal.
 
 Issue: https://gitlab.com/gitlab-org/gitaly/-/issues/5269`)
 
-	t.Parallel()
 	ctx := testhelper.Context(t)
 
 	cfg := testcfg.Build(t)
@@ -82,16 +82,29 @@ 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)
+	args := []string{
+		progname,
+		"restore",
+		"--path",
+		path,
+		"--parallel",
+		strconv.Itoa(runtime.NumCPU()),
+		"--parallel-storage",
+		"2",
+		"--layout",
+		"pointer",
+		"--remove-all-repositories",
+		existingRepo.StorageName,
+	}
+	cmd := NewApp()
+	cmd.Reader = &stdin
+	cmd.Writer = io.Discard
 
 	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),
+		cmd.RunContext(ctx, args),
 		"restore: pipeline: 1 failures encountered:\n - invalid: manager: could not dial source: invalid connection string: \"invalid\"\n")
 
 	require.NoDirExists(t, existRepoPath)
@@ -115,7 +128,6 @@ the database and only then perform the removal.
 
 Issue: https://gitlab.com/gitlab-org/gitaly/-/issues/5269`)
 
-	t.Parallel()
 	ctx := testhelper.Context(t)
 
 	path := testhelper.TempDir(t)
@@ -174,16 +186,29 @@ 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)
+	args := []string{
+		progname,
+		"restore",
+		"--parallel",
+		strconv.Itoa(runtime.NumCPU()),
+		"--parallel-storage",
+		"2",
+		"--layout",
+		"pointer",
+		"--remove-all-repositories",
+		existingRepo.StorageName,
+		"--server-side",
+		"true",
+	}
+	cmd := NewApp()
+	cmd.Reader = &stdin
+	cmd.Writer = io.Discard
 
 	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),
+		cmd.RunContext(ctx, args),
 		"restore: pipeline: 1 failures encountered:\n - invalid: server-side restore: could not dial source: invalid connection string: \"invalid\"\n")
 
 	require.NoDirExists(t, existRepoPath)