Refactor config get-manifest command

This commit addresses the bug that user is unable to see retrieved
manifest name or names, also simplifies the internal logic and
reorganizes unit tests.

Change-Id: I89bac0af811f744c209be2aee921033a5e8a4fca
Signed-off-by: Ruslan Aliev <raliev@mirantis.com>
Closes: #505

cmd/config/get_manifest.go

@@ -15,8 +15,6 @@
 package config
 
 import (
-	"fmt"
-
 	"github.com/spf13/cobra"
 
 	"opendev.org/airship/airshipctl/pkg/config"
@@ -39,40 +37,32 @@ airshipctl config get-manifest e2e
 // NewGetManifestCommand creates a command for viewing the manifest information
 // defined in the airshipctl config file.
 func NewGetManifestCommand(cfgFactory config.Factory) *cobra.Command {
-	o := &config.ManifestOptions{}
+	var manifestName string
 	cmd := &cobra.Command{
 		Use:     "get-manifest NAME",
-		Short:   "Get a manifest information from the airshipctl config",
+		Short:   "Get a manifest(s) information from the airshipctl config",
 		Long:    getManifestsLong[1:],
 		Example: getManifestsExample,
-		Args:    cobra.MaximumNArgs(1),
+		Args:    GetManifestNArgs(&manifestName),
 		Aliases: []string{"get-manifests"},
 		RunE: func(cmd *cobra.Command, args []string) error {
-			airconfig, err := cfgFactory()
+			return config.RunGetManifest(cfgFactory, manifestName, cmd.OutOrStdout())
-			if err != nil {
-				return err
-			}
-			if len(args) == 1 {
-				o.Name = args[0]
-				manifest, exists := airconfig.Manifests[o.Name]
-				if !exists {
-					return config.ErrMissingConfig{
-						What: fmt.Sprintf("manifest with name '%s'", o.Name),
-					}
-				}
-				fmt.Fprintln(cmd.OutOrStdout(), manifest)
-			} else {
-				manifests := airconfig.GetManifests()
-				if len(manifests) == 0 {
-					fmt.Fprintln(cmd.OutOrStdout(), "No Manifest found in the configuration.")
-				}
-				for _, manifest := range manifests {
-					fmt.Fprintln(cmd.OutOrStdout(), manifest)
-				}
-			}
-			return nil
 		},
 	}
 
 	return cmd
 }
+
+// GetManifestNArgs is used to process arguments for get-manifest cmd
+func GetManifestNArgs(manifestName *string) cobra.PositionalArgs {
+	return func(cmd *cobra.Command, args []string) error {
+		if cmd.CalledAs() == "get-manifests" {
+			return cobra.ExactArgs(0)(cmd, args)
+		}
+		if err := cobra.ExactArgs(1)(cmd, args); err != nil {
+			return err
+		}
+		*manifestName = args[0]
+		return nil
+	}
+}

cmd/config/get_manifest_test.go

@@ -15,50 +15,23 @@
 package config_test
 
 import (
+	"bytes"
 	"fmt"
 	"testing"
 
-	cmd "opendev.org/airship/airshipctl/cmd/config"
+	"github.com/spf13/cobra"
-	"opendev.org/airship/airshipctl/pkg/config"
+	"github.com/stretchr/testify/require"
+
+	"opendev.org/airship/airshipctl/cmd/config"
 	"opendev.org/airship/airshipctl/testutil"
 )
 
-func TestGetManifestConfigCmd(t *testing.T) {
+func TestNewGetManifestCommand(t *testing.T) {
-	settings := func() (*config.Config, error) {
-		return &config.Config{
-			Manifests: map[string]*config.Manifest{
-				"fooManifestConfig": getTestManifest("foo"),
-				"barManifestConfig": getTestManifest("bar"),
-				"bazManifestConfig": getTestManifest("baz"),
-			},
-		}, nil
-	}
-
-	noConfigSettings := func() (*config.Config, error) {
-		return new(config.Config), nil
-	}
-
 	cmdTests := []*testutil.CmdTest{
 		{
-			Name:    "get-manifest",
+			Name:    "config-get-manifest-help",
-			CmdLine: "fooManifestConfig",
+			CmdLine: "-h",
-			Cmd:     cmd.NewGetManifestCommand(settings),
+			Cmd:     config.NewGetManifestCommand(nil),
-		},
-		{
-			Name:    "get-all-manifests",
-			CmdLine: "",
-			Cmd:     cmd.NewGetManifestCommand(settings),
-		},
-		{
-			Name:    "missing",
-			CmdLine: "manifestMissing",
-			Cmd:     cmd.NewGetManifestCommand(settings),
-			Error:   fmt.Errorf("missing configuration: manifest with name 'manifestMissing'"),
-		},
-		{
-			Name:    "no-manifests",
-			CmdLine: "",
-			Cmd:     cmd.NewGetManifestCommand(noConfigSettings),
 		},
 	}
 
@@ -67,10 +40,58 @@ func TestGetManifestConfigCmd(t *testing.T) {
 	}
 }
 
-func getTestManifest(name string) *config.Manifest {
+func TestGetManifestNArgs(t *testing.T) {
-	manifests := &config.Manifest{
+	tests := []struct {
-		PhaseRepositoryName: name + "_phase_repo",
+		name         string
-		TargetPath:          name + "_target_path",
+		use          string
+		args         []string
+		manifestName string
+		err          error
+	}{
+		{
+			name: "get-manifests no args",
+			use:  "get-manifests",
+			args: []string{},
+			err:  nil,
+		},
+		{
+			name: "get-manifests 1 arg",
+			use:  "get-manifests",
+			args: []string{"arg1"},
+			err:  fmt.Errorf("accepts 0 arg(s), received 1"),
+		},
+		{
+			name: "get-manifest no args",
+			use:  "get-manifest",
+			args: []string{},
+			err:  fmt.Errorf("accepts 1 arg(s), received 0"),
+		},
+		{
+			name: "get-manifest 1 arg",
+			use:  "get-manifest",
+			args: []string{"arg1"},
+			err:  nil,
+		},
+	}
+
+	out := &bytes.Buffer{}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			cmd := &cobra.Command{
+				Use:  tt.use,
+				Args: config.GetManifestNArgs(&tt.manifestName),
+				Run:  func(cmd *cobra.Command, args []string) {},
+			}
+			cmd.SetArgs(tt.args)
+			cmd.SetOut(out)
+			err := cmd.Execute()
+			if tt.err != nil {
+				require.Equal(t, tt.err, err)
+			} else if len(tt.args) == 1 {
+				require.Equal(t, tt.args[0], tt.manifestName)
+			}
+			out.Reset()
+		})
 	}
-	return manifests
 }

cmd/config/testdata/TestConfigGoldenOutput/config-cmd-with-help.golden

@@ -6,7 +6,7 @@ Usage:
 Available Commands:
   get-context           Get context information from the airshipctl config
   get-management-config View a management config or all management configs defined in the airshipctl config
-  get-manifest          Get a manifest information from the airshipctl config
+  get-manifest          Get a manifest(s) information from the airshipctl config
   help                  Help about any command
   init                  Generate initial configuration file for airshipctl
   set-context           Manage contexts

cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/get-all-manifests.golden

@@ -1,15 +0,0 @@
-inventoryRepositoryName: ""
-metadataPath: ""
-phaseRepositoryName: bar_phase_repo
-targetPath: bar_target_path
-
-inventoryRepositoryName: ""
-metadataPath: ""
-phaseRepositoryName: baz_phase_repo
-targetPath: baz_target_path
-
-inventoryRepositoryName: ""
-metadataPath: ""
-phaseRepositoryName: foo_phase_repo
-targetPath: foo_target_path
-

cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/get-manifest.golden

@@ -1,5 +0,0 @@
-inventoryRepositoryName: ""
-metadataPath: ""
-phaseRepositoryName: foo_phase_repo
-targetPath: foo_target_path
-

cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/no-manifests.golden

@@ -1 +0,0 @@
-No Manifest found in the configuration.

cmd/config/testdata/TestNewGetManifestCommandGoldenOutput/config-get-manifest-help.golden

@@ -1,4 +1,5 @@
-Error: missing configuration: manifest with name 'manifestMissing'
+Display a specific manifest information, or all defined manifests if no name is provided.
+
 Usage:
   get-manifest NAME [flags]
 
@@ -16,4 +17,3 @@ airshipctl config get-manifest e2e
 
 Flags:
   -h, --help   help for get-manifest
-

docs/source/cli/airshipctl_config.md

@@ -24,7 +24,7 @@ Manage the airshipctl config file
 * [airshipctl](airshipctl.md)	 - A unified entrypoint to various airship components
 * [airshipctl config get-context](airshipctl_config_get-context.md)	 - Get context information from the airshipctl config
 * [airshipctl config get-management-config](airshipctl_config_get-management-config.md)	 - View a management config or all management configs defined in the airshipctl config
-* [airshipctl config get-manifest](airshipctl_config_get-manifest.md)	 - Get a manifest information from the airshipctl config
+* [airshipctl config get-manifest](airshipctl_config_get-manifest.md)	 - Get a manifest(s) information from the airshipctl config
 * [airshipctl config init](airshipctl_config_init.md)	 - Generate initial configuration file for airshipctl
 * [airshipctl config set-context](airshipctl_config_set-context.md)	 - Manage contexts
 * [airshipctl config set-management-config](airshipctl_config_set-management-config.md)	 - Modify an out-of-band management configuration

docs/source/cli/airshipctl_config_get-manifest.md

@@ -1,6 +1,6 @@
 ## airshipctl config get-manifest
 
-Get a manifest information from the airshipctl config
+Get a manifest(s) information from the airshipctl config
 
 ### Synopsis
 

pkg/config/config.go

@@ -368,6 +368,15 @@ func (c *Config) CurrentContextInventoryRepositoryName() (string, error) {
 	return util.GitDirNameFromURL(repo.URL()), nil
 }
 
+// GetManifest returns a Manifest instance
+func (c *Config) GetManifest(name string) (*Manifest, error) {
+	manifest, exists := c.Manifests[name]
+	if !exists {
+		return nil, ErrMissingConfig{What: fmt.Sprintf("manifest with name '%s'", name)}
+	}
+	return manifest, nil
+}
+
 // GetManifests returns all of the Manifests associated with the Config sorted by name
 func (c *Config) GetManifests() []*Manifest {
 	keys := make([]string, 0, len(c.Manifests))

pkg/config/config_helper.go

@@ -17,6 +17,8 @@ package config
 import (
 	"fmt"
 	"io"
+
+	"sigs.k8s.io/yaml"
 )
 
 // ContextOption is a function that allows to modify context object
@@ -209,3 +211,28 @@ func RunSetManifest(o *ManifestOptions, airconfig *Config, writeToStorage bool)
 
 	return modified, nil
 }
+
+// RunGetManifest prints desired manifest(s) by its name
+func RunGetManifest(cfgFactory Factory, manifestName string, w io.Writer) error {
+	cfg, err := cfgFactory()
+	if err != nil {
+		return err
+	}
+
+	manifestsMap := map[string]*Manifest{}
+	if manifestName != "" {
+		manifestsMap[manifestName], err = cfg.GetManifest(manifestName)
+		if err != nil {
+			return err
+		}
+	} else {
+		manifestsMap = cfg.Manifests
+	}
+
+	data, err := yaml.Marshal(manifestsMap)
+	if err != nil {
+		return err
+	}
+	_, err = w.Write(data)
+	return err
+}

pkg/config/config_helper_test.go

@@ -246,3 +246,68 @@ func TestRunSetManifest(t *testing.T) {
 		assert.Equal(t, "/tmp/default", conf.Manifests["dummy_manifest"].TargetPath)
 	})
 }
+
+func TestRunGetManifest(t *testing.T) {
+	ioBuffer := bytes.NewBuffer(nil)
+	tests := []struct {
+		name         string
+		cfgFactory   config.Factory
+		manifestName string
+		expectedOut  string
+		err          error
+	}{
+		{
+			name: "bad config",
+			cfgFactory: func() (*config.Config, error) {
+				return nil, config.ErrInvalidConfig{}
+			},
+			err: config.ErrInvalidConfig{},
+		},
+		{
+			name:         "no such manifest",
+			cfgFactory:   prepareConfig(),
+			manifestName: "missing_manifest",
+			err:          config.ErrMissingConfig{What: "manifest with name 'missing_manifest'"},
+		},
+		{
+			name:       "get all manifests",
+			cfgFactory: prepareConfig(),
+			err:        nil,
+			expectedOut: `dummy_manifest:
+  inventoryRepositoryName: primary
+  metadataPath: metadata.yaml
+  phaseRepositoryName: primary
+  repositories:
+    primary:
+      auth:
+        sshKey: testdata/test-key.pem
+        type: ssh-key
+      checkout:
+        branch: ""
+        commitHash: ""
+        force: false
+        localBranch: false
+        tag: v1.0.1
+      url: http://dummy.url.com/manifests.git
+  targetPath: /var/tmp/
+`,
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			ioBuffer.Reset()
+			err := config.RunGetManifest(tt.cfgFactory, tt.manifestName, ioBuffer)
+			if tt.err != nil {
+				require.Error(t, err)
+				require.Equal(t, tt.err, err)
+			} else {
+				require.NoError(t, err)
+			}
+			if tt.expectedOut != "" {
+				require.Equal(t, tt.expectedOut, ioBuffer.String())
+			}
+		})
+	}
+}

pkg/config/config_test.go

@@ -478,6 +478,18 @@ func TestManagementConfigurationByNameDoesNotExist(t *testing.T) {
 	assert.Error(t, err)
 }
 
+func TestGetManifest(t *testing.T) {
+	conf, cleanup := testutil.InitConfig(t)
+	defer cleanup(t)
+
+	_, err := conf.GetManifest("dummy_manifest")
+	require.NoError(t, err)
+
+	// Test Wrong Manifest
+	_, err = conf.GetManifest("unknown")
+	assert.Error(t, err)
+}
+
 func TestGetManifests(t *testing.T) {
 	conf, cleanup := testutil.InitConfig(t)
 	defer cleanup(t)