Merge branch 'pks-cli-help-subcommand-races' into 'master'

cli: Disable help commands to fix test races

Closes #5104

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

Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

23 files changed, 74 insertions, 38 deletions

diff --git a/cmd/gitaly/main_test.go b/cmd/gitaly/main_test.go
index 276f78fe3..06cc5a807 100644
--- a/cmd/gitaly/main_test.go
+++ b/cmd/gitaly/main_test.go
@@ -30,7 +30,7 @@ func TestGitalyCLI(t *testing.T) {
 		{
 			desc:     "without arguments",
 			exitCode: 2,
-			stdout:   "NAME:\n   gitaly - a git server\n\nUSAGE:\n   gitaly command [command options] [arguments...]\n\nCOMMANDS:\n   serve          launch the server daemon\n   check          verify internal API is accessible\n   configuration  allows to run commands related to the configuration\n   hooks          manage hooks for a git repository\n   help, h        Shows a list of commands or help for one command\n\nOPTIONS:\n   --help, -h     show help\n   --version, -v  print the version\n",
+			stdout:   "NAME:\n   gitaly - a git server\n\nUSAGE:\n   gitaly command [command options] [arguments...]\n\nCOMMANDS:\n   serve          launch the server daemon\n   check          verify internal API is accessible\n   configuration  allows to run commands related to the configuration\n   hooks          manage hooks for a git repository\n\nOPTIONS:\n   --help, -h     show help\n   --version, -v  print the version\n",
 		},
 		{
 			desc:     "with non-existent config",
@@ -42,7 +42,7 @@ func TestGitalyCLI(t *testing.T) {
 			desc:     "check without config",
 			args:     []string{"check"},
 			exitCode: 2,
-			stdout:   "NAME:\n   gitaly check - verify internal API is accessible\n\nUSAGE:\n   gitaly check command [command options] <configfile>\n\nCOMMANDS:\n   help, h  Shows a list of commands or help for one command\n\nOPTIONS:\n   --help, -h  show help\n",
+			stdout:   "NAME:\n   gitaly check - verify internal API is accessible\n\nUSAGE:\n   gitaly check command [command options] <configfile>\n\nOPTIONS:\n   --help, -h  show help\n",
 			stderr:   "invalid argument(s)",
 		},
 		{
diff --git a/internal/cli/gitaly/app.go b/internal/cli/gitaly/app.go
index 232ab2aa3..f11c7ecb5 100644
--- a/internal/cli/gitaly/app.go
+++ b/internal/cli/gitaly/app.go
@@ -24,7 +24,8 @@ func NewApp() *cli.App {
 		// serveAction is also here in the root to keep the CLI backwards compatible
 		// with the previous way to launch Gitaly with just `gitaly <configfile>`. We
 		// may want to deprecate this eventually.
-		Action: serveAction,
+		Action:          serveAction,
+		HideHelpCommand: true,
 		Commands: []*cli.Command{
 			newServeCommand(),
 			newCheckCommand(),
diff --git a/internal/cli/gitaly/check.go b/internal/cli/gitaly/check.go
index 010575575..210c7d68b 100644
--- a/internal/cli/gitaly/check.go
+++ b/internal/cli/gitaly/check.go
@@ -15,10 +15,11 @@ import (
 
 func newCheckCommand() *cli.Command {
 	return &cli.Command{
-		Name:      "check",
-		Usage:     "verify internal API is accessible",
-		ArgsUsage: "<configfile>",
-		Action:    checkAction,
+		Name:            "check",
+		Usage:           "verify internal API is accessible",
+		ArgsUsage:       "<configfile>",
+		Action:          checkAction,
+		HideHelpCommand: true,
 	}
 }
 
diff --git a/internal/cli/gitaly/configuration-validate.go b/internal/cli/gitaly/configuration-validate.go
index bca5d1a9a..2d13c6177 100644
--- a/internal/cli/gitaly/configuration-validate.go
+++ b/internal/cli/gitaly/configuration-validate.go
@@ -11,8 +11,9 @@ const validationErrorCode = 2
 
 func newConfigurationCommand() *cli.Command {
 	return &cli.Command{
-		Name:  "configuration",
-		Usage: "allows to run commands related to the configuration",
+		Name:            "configuration",
+		Usage:           "allows to run commands related to the configuration",
+		HideHelpCommand: true,
 		Subcommands: []*cli.Command{
 			{
 				Name:   "validate",
diff --git a/internal/cli/gitaly/hooks.go b/internal/cli/gitaly/hooks.go
index 243b1aa9f..ef05a03be 100644
--- a/internal/cli/gitaly/hooks.go
+++ b/internal/cli/gitaly/hooks.go
@@ -27,8 +27,9 @@ const (
 
 func newHooksCommand() *cli.Command {
 	return &cli.Command{
-		Name:  "hooks",
-		Usage: "manage hooks for a git repository",
+		Name:            "hooks",
+		Usage:           "manage hooks for a git repository",
+		HideHelpCommand: true,
 		Subcommands: []*cli.Command{
 			{
 				Name:        "set",
diff --git a/internal/cli/gitaly/serve.go b/internal/cli/gitaly/serve.go
index b7abd6654..2dcf3dc00 100644
--- a/internal/cli/gitaly/serve.go
+++ b/internal/cli/gitaly/serve.go
@@ -53,10 +53,11 @@ import (
 
 func newServeCommand() *cli.Command {
 	return &cli.Command{
-		Name:      "serve",
-		Usage:     "launch the server daemon",
-		ArgsUsage: "<configfile>",
-		Action:    serveAction,
+		Name:            "serve",
+		Usage:           "launch the server daemon",
+		ArgsUsage:       "<configfile>",
+		Action:          serveAction,
+		HideHelpCommand: true,
 	}
 }
 
diff --git a/internal/cli/praefect/main.go b/internal/cli/praefect/main.go
index b4e106d97..24bce6613 100644
--- a/internal/cli/praefect/main.go
+++ b/internal/cli/praefect/main.go
@@ -67,7 +67,8 @@ func NewApp() *cli.App {
 		//
 		// The 'DefaultCommand: "serve"' setting can't be used here because it won't be
 		// possible to invoke sub-command not yet registered.
-		Action: serveAction,
+		Action:          serveAction,
+		HideHelpCommand: true,
 		Commands: []*cli.Command{
 			newServeCommand(),
 			newConfigurationCommand(),
diff --git a/internal/cli/praefect/serve.go b/internal/cli/praefect/serve.go
index b9b637f45..1c62d579a 100644
--- a/internal/cli/praefect/serve.go
+++ b/internal/cli/praefect/serve.go
@@ -43,9 +43,10 @@ import (
 
 func newServeCommand() *cli.Command {
 	return &cli.Command{
-		Name:   "serve",
-		Usage:  "launch the server daemon",
-		Action: serveAction,
+		Name:            "serve",
+		Usage:           "launch the server daemon",
+		Action:          serveAction,
+		HideHelpCommand: true,
 	}
 }
 
diff --git a/internal/cli/praefect/subcmd_accept_dataloss.go b/internal/cli/praefect/subcmd_accept_dataloss.go
index 4012af0c1..5374ab9ee 100644
--- a/internal/cli/praefect/subcmd_accept_dataloss.go
+++ b/internal/cli/praefect/subcmd_accept_dataloss.go
@@ -18,6 +18,7 @@ writing again. The current version of the repository on the authoritative storag
 the latest version and replications to other nodes are scheduled in order to bring them consistent
 with the new authoritative version.
 `,
+		HideHelpCommand: true,
 		Flags: []cli.Flag{
 			&cli.StringFlag{
 				Name:     paramVirtualStorage,
diff --git a/internal/cli/praefect/subcmd_accept_dataloss_test.go b/internal/cli/praefect/subcmd_accept_dataloss_test.go
index 8aaa7e266..9af7d89b5 100644
--- a/internal/cli/praefect/subcmd_accept_dataloss_test.go
+++ b/internal/cli/praefect/subcmd_accept_dataloss_test.go
@@ -1,7 +1,9 @@
 package praefect
 
 import (
+	"bytes"
 	"context"
+	"io"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -137,6 +139,10 @@ func TestAcceptDatalossSubcommand(t *testing.T) {
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			app := cli.App{
+				Reader:          bytes.NewReader(nil),
+				Writer:          io.Discard,
+				ErrWriter:       io.Discard,
+				HideHelpCommand: true,
 				Commands: []*cli.Command{
 					newAcceptDatalossCommand(),
 				},
diff --git a/internal/cli/praefect/subcmd_check.go b/internal/cli/praefect/subcmd_check.go
index 19de0802c..436e59ba0 100644
--- a/internal/cli/praefect/subcmd_check.go
+++ b/internal/cli/praefect/subcmd_check.go
@@ -14,9 +14,10 @@ import (
 
 func newCheckCommand(checkFuncs []service.CheckFunc) *cli.Command {
 	return &cli.Command{
-		Name:        "check",
-		Usage:       "runs startup checks",
-		Description: `This command runs startup checks for Praefect.`,
+		Name:            "check",
+		Usage:           "runs startup checks",
+		Description:     `This command runs startup checks for Praefect.`,
+		HideHelpCommand: true,
 		Flags: []cli.Flag{
 			&cli.BoolFlag{
 				Name:  "q",
diff --git a/internal/cli/praefect/subcmd_check_test.go b/internal/cli/praefect/subcmd_check_test.go
index 38af7debb..cec0efe07 100644
--- a/internal/cli/praefect/subcmd_check_test.go
+++ b/internal/cli/praefect/subcmd_check_test.go
@@ -183,7 +183,10 @@ Checking check 3...Failed (warning) error: i failed but not too badly
 		t.Run(tc.desc, func(t *testing.T) {
 			var stdout bytes.Buffer
 			app := cli.App{
-				Writer: &stdout,
+				Reader:          bytes.NewReader(nil),
+				Writer:          &stdout,
+				ErrWriter:       io.Discard,
+				HideHelpCommand: true,
 				Commands: []*cli.Command{
 					newCheckCommand(tc.checks),
 				},
diff --git a/internal/cli/praefect/subcmd_configuration.go b/internal/cli/praefect/subcmd_configuration.go
index 4f35ed6ef..1a767bd45 100644
--- a/internal/cli/praefect/subcmd_configuration.go
+++ b/internal/cli/praefect/subcmd_configuration.go
@@ -6,8 +6,9 @@ const configurationCmdName = "configuration"
 
 func newConfigurationCommand() *cli.Command {
 	return &cli.Command{
-		Name:  configurationCmdName,
-		Usage: "manages configuration",
+		Name:            configurationCmdName,
+		Usage:           "manages configuration",
+		HideHelpCommand: true,
 		Subcommands: []*cli.Command{
 			newConfigurationValidateCommand(),
 		},
diff --git a/internal/cli/praefect/subcmd_configuration_validate.go b/internal/cli/praefect/subcmd_configuration_validate.go
index 2b3da6d07..dd1af5cc8 100644
--- a/internal/cli/praefect/subcmd_configuration_validate.go
+++ b/internal/cli/praefect/subcmd_configuration_validate.go
@@ -17,7 +17,8 @@ func newConfigurationValidateCommand() *cli.Command {
 		Description: "The command accepts configuration in toml format on STDIN. It applies " +
 			"validation rules to the received configuration and returns all the found " +
 			"validation errors in JSON format back on STDOUT.",
-		Action: configurationValidateAction,
+		HideHelpCommand: true,
+		Action:          configurationValidateAction,
 	}
 }
 
diff --git a/internal/cli/praefect/subcmd_configuration_validate_test.go b/internal/cli/praefect/subcmd_configuration_validate_test.go
index 9cb1364b2..bef110b1a 100644
--- a/internal/cli/praefect/subcmd_configuration_validate_test.go
+++ b/internal/cli/praefect/subcmd_configuration_validate_test.go
@@ -71,9 +71,6 @@ USAGE:
 DESCRIPTION:
    The command accepts configuration in toml format on STDIN. It applies validation rules to the received configuration and returns all the found validation errors in JSON format back on STDOUT.
 
-COMMANDS:
-   help, h  Shows a list of commands or help for one command
-
 OPTIONS:
    --help, -h  show help
 `,
diff --git a/internal/cli/praefect/subcmd_dataloss.go b/internal/cli/praefect/subcmd_dataloss.go
index bc618e6aa..ea1c7e011 100644
--- a/internal/cli/praefect/subcmd_dataloss.go
+++ b/internal/cli/praefect/subcmd_dataloss.go
@@ -19,7 +19,8 @@ func newDatalossCommand() *cli.Command {
 			"previous write-enabled primary node. It does so by looking through incomplete\n" +
 			"replication jobs. This is useful for identifying potential data loss from a failover\n" +
 			"event.",
-		Action: datalossAction,
+		HideHelpCommand: true,
+		Action:          datalossAction,
 		Flags: []cli.Flag{
 			&cli.StringFlag{
 				Name: paramVirtualStorage,
diff --git a/internal/cli/praefect/subcmd_dataloss_test.go b/internal/cli/praefect/subcmd_dataloss_test.go
index ee1e9ae34..ed9778f1c 100644
--- a/internal/cli/praefect/subcmd_dataloss_test.go
+++ b/internal/cli/praefect/subcmd_dataloss_test.go
@@ -2,6 +2,7 @@ package praefect
 
 import (
 	"bytes"
+	"io"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -180,7 +181,10 @@ Virtual storage: virtual-storage-2
 
 			var stdout bytes.Buffer
 			app := cli.App{
-				Writer: &stdout,
+				Reader:          bytes.NewReader(nil),
+				Writer:          &stdout,
+				ErrWriter:       io.Discard,
+				HideHelpCommand: true,
 				Commands: []*cli.Command{
 					newDatalossCommand(),
 				},
diff --git a/internal/cli/praefect/subcmd_dial_nodes.go b/internal/cli/praefect/subcmd_dial_nodes.go
index debf1f908..109c7f44c 100644
--- a/internal/cli/praefect/subcmd_dial_nodes.go
+++ b/internal/cli/praefect/subcmd_dial_nodes.go
@@ -15,7 +15,8 @@ func newDialNodesCommand() *cli.Command {
 		Description: "The subcommand \"dial-nodes\" helps diagnose connection problems to Gitaly or\n" +
 			"Praefect. The subcommand works by sourcing the connection information from\n" +
 			"the config file, and then dialing and health checking the remote nodes.",
-		Action: dialNodesAction,
+		HideHelpCommand: true,
+		Action:          dialNodesAction,
 		Flags: []cli.Flag{
 			&cli.DurationFlag{
 				Name:  "timeout",
diff --git a/internal/cli/praefect/subcmd_dial_nodes_test.go b/internal/cli/praefect/subcmd_dial_nodes_test.go
index ffb977f74..adb245f29 100644
--- a/internal/cli/praefect/subcmd_dial_nodes_test.go
+++ b/internal/cli/praefect/subcmd_dial_nodes_test.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"io"
 	"strings"
 	"testing"
 	"time"
@@ -137,7 +138,10 @@ func TestSubCmdDialNodes(t *testing.T) {
 
 			var stdout bytes.Buffer
 			app := cli.App{
-				Writer: &stdout,
+				Reader:          bytes.NewReader(nil),
+				Writer:          &stdout,
+				ErrWriter:       io.Discard,
+				HideHelpCommand: true,
 				Commands: []*cli.Command{
 					newDialNodesCommand(),
 				},
diff --git a/internal/cli/praefect/subcmd_list_storages.go b/internal/cli/praefect/subcmd_list_storages.go
index 0956e522f..41a4f9728 100644
--- a/internal/cli/praefect/subcmd_list_storages.go
+++ b/internal/cli/praefect/subcmd_list_storages.go
@@ -15,7 +15,8 @@ func newListStoragesCommand() *cli.Command {
 		Description: "This command lists virtual storages and their associated storages.\n" +
 			"Passing a virtual-storage argument will print out the storage associated with\n" +
 			"that particular virtual storage.\n",
-		Action: listStoragesAction,
+		HideHelpCommand: true,
+		Action:          listStoragesAction,
 		Flags: []cli.Flag{
 			&cli.StringFlag{
 				Name:  paramVirtualStorage,
diff --git a/internal/cli/praefect/subcmd_list_storages_test.go b/internal/cli/praefect/subcmd_list_storages_test.go
index 61252e33d..a28347fc3 100644
--- a/internal/cli/praefect/subcmd_list_storages_test.go
+++ b/internal/cli/praefect/subcmd_list_storages_test.go
@@ -2,6 +2,7 @@ package praefect
 
 import (
 	"bytes"
+	"io"
 	"testing"
 
 	"github.com/olekukonko/tablewriter"
@@ -119,7 +120,10 @@ func TestListStoragesSubcommand(t *testing.T) {
 	exec := func(confPath string, args []string) (string, error) {
 		var stdout bytes.Buffer
 		app := cli.App{
-			Writer: &stdout,
+			Reader:          bytes.NewReader(nil),
+			Writer:          &stdout,
+			ErrWriter:       io.Discard,
+			HideHelpCommand: true,
 			Commands: []*cli.Command{
 				newListStoragesCommand(),
 			},
diff --git a/internal/cli/praefect/subcmd_list_untracked_repositories.go b/internal/cli/praefect/subcmd_list_untracked_repositories.go
index f432bd929..cf534a293 100644
--- a/internal/cli/praefect/subcmd_list_untracked_repositories.go
+++ b/internal/cli/praefect/subcmd_list_untracked_repositories.go
@@ -30,8 +30,8 @@ func newListUntrackedRepositoriesCommand() *cli.Command {
 			"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,
+		HideHelpCommand: true,
+		Action:          listUntrackedRepositoriesAction,
 		Flags: []cli.Flag{
 			&cli.StringFlag{
 				Name:  "delimiter",
diff --git a/internal/cli/praefect/subcmd_list_untracked_repositories_test.go b/internal/cli/praefect/subcmd_list_untracked_repositories_test.go
index e04624238..430872e75 100644
--- a/internal/cli/praefect/subcmd_list_untracked_repositories_test.go
+++ b/internal/cli/praefect/subcmd_list_untracked_repositories_test.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"io"
 	"os"
 	"strings"
 	"testing"
@@ -88,7 +89,10 @@ func TestListUntrackedRepositoriesCommand(t *testing.T) {
 	newApp := func() (cli.App, *bytes.Buffer) {
 		var stdout bytes.Buffer
 		return cli.App{
-			Writer: &stdout,
+			Reader:          bytes.NewReader(nil),
+			Writer:          &stdout,
+			ErrWriter:       io.Discard,
+			HideHelpCommand: true,
 			Commands: []*cli.Command{
 				newListUntrackedRepositoriesCommand(),
 			},