Repository: tetratelabs/getenvoy
Pull request #227 (2021-05-19 03:12:11)

Removes help subcommand to simplify usage
This removes the "help" subcommand using the below approach. Doing so
keeps coherent advise as we suggest running "-h". It also declutters
markdown and help output.

See https://github.com/spf13/cobra/issues/587#issuecomment-810159087

Signed-off-by: Adrian Cole <adrian@tetrate.io>


Patch:
diff --git a/pkg/cmd/app.go b/pkg/cmd/app.go
index f7607d1..91dc5e4 100644
--- a/pkg/cmd/app.go
+++ b/pkg/cmd/app.go
@@ -52,6 +52,7 @@ func NewApp(o *globals.GlobalOpts) *cobra.Command {
 	rootCmd.AddCommand(NewListCmd(o))
 	rootCmd.AddCommand(NewFetchCmd(o))
 	rootCmd.AddCommand(NewDocCmd())
+	rootCmd.SetHelpCommand(&cobra.Command{Hidden: true}) // only support -h
 
 	rootCmd.PersistentFlags().StringVar(&homeDir, "home-dir", homeDir,
 		"GetEnvoy home directory (location of downloaded artifacts, caches, etc)")


Test patch:
diff --git a/cmd/getenvoy/main_test.go b/cmd/getenvoy/main_test.go
index 9b6eaca..ab7f0c5 100644
--- a/cmd/getenvoy/main_test.go
+++ b/cmd/getenvoy/main_test.go
@@ -51,7 +51,7 @@ func TestRunErrors(t *testing.T) {
 		},
 		{
 			name:           "incorrect global flag value",
-			args:           []string{"getenvoy", "--manifest", ".", "help"},
+			args:           []string{"getenvoy", "--manifest", ".", "list"},
 			expectedStatus: 1,
 			expectedStderr: `"." is not a valid manifest URL
 show usage with: getenvoy -h
diff --git a/pkg/cmd/app_test.go b/pkg/cmd/app_test.go
index 2231fdb..de43c8f 100644
--- a/pkg/cmd/app_test.go
+++ b/pkg/cmd/app_test.go
@@ -29,8 +29,6 @@ import (
 )
 
 func TestGetEnvoyValidateArgs(t *testing.T) {
-	o := &globals.GlobalOpts{}
-
 	tests := []struct {
 		name        string
 		args        []string
@@ -38,7 +36,7 @@ func TestGetEnvoyValidateArgs(t *testing.T) {
 	}{
 		{
 			name:        "--manifest not a URL",
-			args:        []string{"getenvoy", "--manifest", "/not/url", "help"},
+			args:        []string{"getenvoy", "--manifest", "/not/url"},
 			expectedErr: `"/not/url" is not a valid manifest URL`,
 		},
 	}
@@ -47,15 +45,8 @@ func TestGetEnvoyValidateArgs(t *testing.T) {
 		test := test // pin! see https://github.com/kyoh86/scopelint for why
 
 		t.Run(test.name, func(t *testing.T) {
-			c, stdout, stderr := newApp(o)
-			c.SetArgs(test.args[1:])
-			err := c.Execute()
-
-			// Verify the command failed with the expected error
-			require.EqualError(t, err, test.expectedErr, `expected an error running [%v]`, c)
-			// Main handles logging of errors, so we expect nothing in stdout or stderr
-			require.Empty(t, stdout, `expected no stdout running [%v]`, c)
-			require.Empty(t, stderr, `expected no stderr running [%v]`, c)
+			err := runTestCommand(t, &globals.GlobalOpts{}, test.args[1:])
+			require.EqualError(t, err, test.expectedErr)
 		})
 	}
 }
@@ -75,12 +66,12 @@ func TestGetEnvoyHomeDir(t *testing.T) {
 	tests := []testCase{ // we don't test default as that depends on the runtime env
 		{
 			name:     "default is ~/.getenvoy",
-			args:     []string{"getenvoy", "help"},
+			args:     []string{"getenvoy"},
 			expected: filepath.Join(u.HomeDir, ".getenvoy"),
 		},
 		{
 			name: "GETENVOY_HOME env",
-			args: []string{"getenvoy", "help"},
+			args: []string{"getenvoy"},
 			setup: func() func() {
 				return requireSetenv(t, "GETENVOY_HOME", "/from/GETENVOY_HOME/env")
 			},
@@ -88,12 +79,12 @@ func TestGetEnvoyHomeDir(t *testing.T) {
 		},
 		{
 			name:     "--home-dir arg",
-			args:     []string{"getenvoy", "--home-dir", "/from/home-dir/arg", "help"},
+			args:     []string{"getenvoy", "--home-dir", "/from/home-dir/arg"},
 			expected: "/from/home-dir/arg",
 		},
 		{
 			name: "prioritizes --home-dir arg over GETENVOY_HOME env",
-			args: []string{"getenvoy", "--home-dir", "/from/home-dir/arg", "help"},
+			args: []string{"getenvoy", "--home-dir", "/from/home-dir/arg"},
 			setup: func() func() {
 				return requireSetenv(t, "GETENVOY_HOME", "/from/GETENVOY_HOME/env")
 			},
@@ -111,14 +102,9 @@ func TestGetEnvoyHomeDir(t *testing.T) {
 			}
 
 			o := &globals.GlobalOpts{}
-			c, stdout, stderr := newApp(o)
-			c.SetArgs(test.args[1:])
-			err := c.Execute()
-
-			require.NoError(t, err, `expected no error running [%v]`, c)
-			require.NotEmpty(t, stdout.String(), `expected stdout running [%v]`, c)
-			require.Empty(t, stderr.String(), `expected no stderr running [%v]`, c)
+			err := runTestCommand(t, o, test.args[1:])
 
+			require.NoError(t, err)
 			require.Equal(t, test.expected, o.HomeDir)
 		})
 	}
@@ -136,12 +122,12 @@ func TestGetEnvoyManifest(t *testing.T) {
 	tests := []testCase{ // we don't test default as that depends on the runtime env
 		{
 			name:     "default is https://dl.getenvoy.io/public/raw/files/manifest.json",
-			args:     []string{"getenvoy", "help"},
+			args:     []string{"getenvoy"},
 			expected: "https://dl.getenvoy.io/public/raw/files/manifest.json",
 		},
 		{
 			name: "GETENVOY_MANIFEST_URL env",
-			args: []string{"getenvoy", "help"},
+			args: []string{"getenvoy"},
 			setup: func() func() {
 				return requireSetenv(t, "GETENVOY_MANIFEST_URL", "http://GETENVOY_MANIFEST_URL/env")
 			},
@@ -149,12 +135,12 @@ func TestGetEnvoyManifest(t *testing.T) {
 		},
 		{
 			name:     "--manifest arg",
-			args:     []string{"getenvoy", "--manifest", "http://manifest/arg", "help"},
+			args:     []string{"getenvoy", "--manifest", "http://manifest/arg"},
 			expected: "http://manifest/arg",
 		},
 		{
 			name: "prioritizes --manifest arg over GETENVOY_MANIFEST_URL env",
-			args: []string{"getenvoy", "--manifest", "http://manifest/arg", "help"},
+			args: []string{"getenvoy", "--manifest", "http://manifest/arg"},
 			setup: func() func() {
 				return requireSetenv(t, "GETENVOY_MANIFEST_URL", "http://GETENVOY_MANIFEST_URL/env")
 			},
@@ -172,14 +158,9 @@ func TestGetEnvoyManifest(t *testing.T) {
 			}
 
 			o := &globals.GlobalOpts{}
-			c, stdout, stderr := newApp(o)
-			c.SetArgs(test.args[1:])
-			err := c.Execute()
-
-			require.NoError(t, err, `expected no error running [%v]`, c)
-			require.NotEmpty(t, stdout.String(), `expected stdout running [%v]`, c)
-			require.Empty(t, stderr.String(), `expected no stderr running [%v]`, c)
+			err := runTestCommand(t, o, test.args[1:])
 
+			require.NoError(t, err)
 			require.Equal(t, test.expected, o.ManifestURL)
 		})
 	}
@@ -205,3 +186,20 @@ func newApp(o *globals.GlobalOpts) (c *cobra.Command, stdout, stderr *bytes.Buff
 	c.SetErr(stderr)
 	return c, stdout, stderr
 }
+
+func runTestCommand(t *testing.T, o *globals.GlobalOpts, args []string) error {
+	c, stdout, stderr := newApp(o)
+	c.AddCommand(&cobra.Command{
+		Use: "test",
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return nil
+		},
+	})
+	c.SetArgs(append(args, "test"))
+	err := c.Execute()
+
+	// Main handles logging of errors, so we expect nothing in stdout or stderr even in error case
+	require.Empty(t, stdout)
+	require.Empty(t, stderr)
+	return err
+}
diff --git a/pkg/cmd/doc_test.go b/pkg/cmd/doc_test.go
index acb6d5d..5a39325 100644
--- a/pkg/cmd/doc_test.go
+++ b/pkg/cmd/doc_test.go
@@ -43,7 +43,7 @@ func TestGetEnvoyDoc(t *testing.T) {
 			require.NoError(t, err)
 			have, err := os.ReadFile(filepath.Join(tempDir, file))
 			require.NoError(t, err)
-			require.Equal(t, strings.ReplaceAll(string(want), "VERSION", reference.Latest), string(have))
+			require.Equal(t, strings.ReplaceAll(string(want), "ENVOY_VERSION", reference.Latest), string(have))
 		})
 	}
 }
diff --git a/pkg/cmd/help_test.go b/pkg/cmd/help_test.go
index ac9b1dc..d84b15f 100644
--- a/pkg/cmd/help_test.go
+++ b/pkg/cmd/help_test.go
@@ -32,9 +32,9 @@ func TestGetEnvoyHelp(t *testing.T) {
 		command := command
 		t.Run(command, func(t *testing.T) {
 			c, stdout, _ := newApp(&globals.GlobalOpts{})
-			args := []string{"help"}
+			args := []string{"-h"}
 			if command != "" {
-				args = append(args, command)
+				args = []string{command, "-h"}
 			}
 			c.SetArgs(args)
 			require.NoError(t, c.Execute())
@@ -46,7 +46,7 @@ func TestGetEnvoyHelp(t *testing.T) {
 
 			want, err := os.ReadFile(filepath.Join("testdata", expected))
 			require.NoError(t, err)
-			require.Equal(t, strings.ReplaceAll(string(want), "VERSION", reference.Latest), stdout.String())
+			require.Equal(t, strings.ReplaceAll(string(want), "ENVOY_VERSION", reference.Latest), stdout.String())
 		})
 	}
 }
diff --git a/pkg/cmd/testdata/.editorconfig b/pkg/cmd/testdata/.editorconfig
new file mode 100644
index 0000000..20754f1
--- /dev/null
+++ b/pkg/cmd/testdata/.editorconfig
@@ -0,0 +1,4 @@
+# see https://github.com/mvdan/sh#shfmt
+[*.{txt,md}]
+insert_final_newline     = false
+trim_trailing_whitespace = false
diff --git a/pkg/cmd/testdata/getenvoy_fetch.md b/pkg/cmd/testdata/getenvoy_fetch.md
index 9652e51..9edc9e6 100644
--- a/pkg/cmd/testdata/getenvoy_fetch.md
+++ b/pkg/cmd/testdata/getenvoy_fetch.md
@@ -16,10 +16,10 @@ getenvoy fetch <reference> [flags]
 
 ```
 # Fetch using a partial manifest reference to retrieve a build suitable for your operating system.
-getenvoy fetch VERSION
+getenvoy fetch ENVOY_VERSION
 
 # Fetch using a full manifest reference to retrieve a specific build for Linux.
-getenvoy fetch VERSION/linux-glibc
+getenvoy fetch ENVOY_VERSION/linux-glibc
 ```
 
 ### Options
diff --git a/pkg/cmd/testdata/getenvoy_fetch_help.txt b/pkg/cmd/testdata/getenvoy_fetch_help.txt
index bd679b8..bdffbab 100644
--- a/pkg/cmd/testdata/getenvoy_fetch_help.txt
+++ b/pkg/cmd/testdata/getenvoy_fetch_help.txt
@@ -5,10 +5,10 @@ Usage:
 
 Examples:
 # Fetch using a partial manifest reference to retrieve a build suitable for your operating system.
-getenvoy fetch VERSION
+getenvoy fetch ENVOY_VERSION
 
 # Fetch using a full manifest reference to retrieve a specific build for Linux.
-getenvoy fetch VERSION/linux-glibc
+getenvoy fetch ENVOY_VERSION/linux-glibc
 
 Flags:
   -h, --help   help for fetch
diff --git a/pkg/cmd/testdata/getenvoy_help.txt b/pkg/cmd/testdata/getenvoy_help.txt
index b31caf9..53c37eb 100644
--- a/pkg/cmd/testdata/getenvoy_help.txt
+++ b/pkg/cmd/testdata/getenvoy_help.txt
@@ -5,12 +5,12 @@ Usage:
 
 Available Commands:
   fetch       Downloads a version of Envoy. Available builds can be retrieved using `getenvoy list`.
-  help        Help about any command
   list        List available Envoy version references you can run
   run         Runs Envoy and collects process state on exit. Available builds can be retrieved using `getenvoy list`.
 
 Flags:
   -h, --help              help for getenvoy
       --home-dir string   GetEnvoy home directory (location of downloaded artifacts, caches, etc)
+  -v, --version           version for getenvoy
 
 Use "getenvoy [command] --help" for more information about a command.
diff --git a/pkg/cmd/testdata/getenvoy_run.md b/pkg/cmd/testdata/getenvoy_run.md
index 4c7b2e0..307931b 100644
--- a/pkg/cmd/testdata/getenvoy_run.md
+++ b/pkg/cmd/testdata/getenvoy_run.md
@@ -16,10 +16,10 @@ getenvoy run reference [flags] [-- <envoy-args>]
 
 ```
 # Run using a manifest reference.
-getenvoy run VERSION -- --config-path ./bootstrap.yaml
+getenvoy run ENVOY_VERSION -- --config-path ./bootstrap.yaml
 
 # List available Envoy flags.
-getenvoy run VERSION -- --help
+getenvoy run ENVOY_VERSION -- --help
 
 ```
 
diff --git a/pkg/cmd/testdata/getenvoy_run_help.txt b/pkg/cmd/testdata/getenvoy_run_help.txt
index 8c97bf5..5e284ba 100644
--- a/pkg/cmd/testdata/getenvoy_run_help.txt
+++ b/pkg/cmd/testdata/getenvoy_run_help.txt
@@ -5,10 +5,10 @@ Usage:
 
 Examples:
 # Run using a manifest reference.
-getenvoy run VERSION -- --config-path ./bootstrap.yaml
+getenvoy run ENVOY_VERSION -- --config-path ./bootstrap.yaml
 
 # List available Envoy flags.
-getenvoy run VERSION -- --help
+getenvoy run ENVOY_VERSION -- --help
 
 
 Flags: