From 864184f053b507388791fb4cef080a8d2073e926 Mon Sep 17 00:00:00 2001 From: Ruslan Aliev Date: Sat, 3 Apr 2021 22:24:23 -0500 Subject: [PATCH] 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 Closes: #505 --- cmd/config/get_manifest.go | 46 +++----- cmd/config/get_manifest_test.go | 103 +++++++++++------- .../config-cmd-with-help.golden | 2 +- .../get-all-manifests.golden | 15 --- .../get-manifest.golden | 5 - .../no-manifests.golden | 1 - .../config-get-manifest-help.golden} | 4 +- docs/source/cli/airshipctl_config.md | 2 +- .../cli/airshipctl_config_get-manifest.md | 2 +- pkg/config/config.go | 9 ++ pkg/config/config_helper.go | 27 +++++ pkg/config/config_helper_test.go | 65 +++++++++++ pkg/config/config_test.go | 12 ++ 13 files changed, 198 insertions(+), 95 deletions(-) delete mode 100644 cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/get-all-manifests.golden delete mode 100644 cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/get-manifest.golden delete mode 100644 cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/no-manifests.golden rename cmd/config/testdata/{TestGetManifestConfigCmdGoldenOutput/missing.golden => TestNewGetManifestCommandGoldenOutput/config-get-manifest-help.golden} (75%) diff --git a/cmd/config/get_manifest.go b/cmd/config/get_manifest.go index 5b823ca14..c5dddcecd 100644 --- a/cmd/config/get_manifest.go +++ b/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() - 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 config.RunGetManifest(cfgFactory, manifestName, cmd.OutOrStdout()) }, } 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 + } +} diff --git a/cmd/config/get_manifest_test.go b/cmd/config/get_manifest_test.go index 898122c98..0f872b986 100644 --- a/cmd/config/get_manifest_test.go +++ b/cmd/config/get_manifest_test.go @@ -15,50 +15,23 @@ package config_test import ( + "bytes" "fmt" "testing" - cmd "opendev.org/airship/airshipctl/cmd/config" - "opendev.org/airship/airshipctl/pkg/config" + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" + + "opendev.org/airship/airshipctl/cmd/config" "opendev.org/airship/airshipctl/testutil" ) -func TestGetManifestConfigCmd(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 - } - +func TestNewGetManifestCommand(t *testing.T) { cmdTests := []*testutil.CmdTest{ { - Name: "get-manifest", - CmdLine: "fooManifestConfig", - Cmd: cmd.NewGetManifestCommand(settings), - }, - { - 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), + Name: "config-get-manifest-help", + CmdLine: "-h", + Cmd: config.NewGetManifestCommand(nil), }, } @@ -67,10 +40,58 @@ func TestGetManifestConfigCmd(t *testing.T) { } } -func getTestManifest(name string) *config.Manifest { - manifests := &config.Manifest{ - PhaseRepositoryName: name + "_phase_repo", - TargetPath: name + "_target_path", +func TestGetManifestNArgs(t *testing.T) { + tests := []struct { + name string + 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 } diff --git a/cmd/config/testdata/TestConfigGoldenOutput/config-cmd-with-help.golden b/cmd/config/testdata/TestConfigGoldenOutput/config-cmd-with-help.golden index 240214f65..6cd2a7a9d 100644 --- a/cmd/config/testdata/TestConfigGoldenOutput/config-cmd-with-help.golden +++ b/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 diff --git a/cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/get-all-manifests.golden b/cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/get-all-manifests.golden deleted file mode 100644 index 8de29e306..000000000 --- a/cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/get-all-manifests.golden +++ /dev/null @@ -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 - diff --git a/cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/get-manifest.golden b/cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/get-manifest.golden deleted file mode 100644 index 43170e7db..000000000 --- a/cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/get-manifest.golden +++ /dev/null @@ -1,5 +0,0 @@ -inventoryRepositoryName: "" -metadataPath: "" -phaseRepositoryName: foo_phase_repo -targetPath: foo_target_path - diff --git a/cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/no-manifests.golden b/cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/no-manifests.golden deleted file mode 100644 index 84a3bf944..000000000 --- a/cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/no-manifests.golden +++ /dev/null @@ -1 +0,0 @@ -No Manifest found in the configuration. diff --git a/cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/missing.golden b/cmd/config/testdata/TestNewGetManifestCommandGoldenOutput/config-get-manifest-help.golden similarity index 75% rename from cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/missing.golden rename to cmd/config/testdata/TestNewGetManifestCommandGoldenOutput/config-get-manifest-help.golden index 417f1b1bf..7d3eb66a5 100644 --- a/cmd/config/testdata/TestGetManifestConfigCmdGoldenOutput/missing.golden +++ b/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 - diff --git a/docs/source/cli/airshipctl_config.md b/docs/source/cli/airshipctl_config.md index 276be0e3c..67b004cbb 100644 --- a/docs/source/cli/airshipctl_config.md +++ b/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 diff --git a/docs/source/cli/airshipctl_config_get-manifest.md b/docs/source/cli/airshipctl_config_get-manifest.md index ba5062b7a..370ba82f9 100644 --- a/docs/source/cli/airshipctl_config_get-manifest.md +++ b/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 diff --git a/pkg/config/config.go b/pkg/config/config.go index bd72958df..3f1e8c439 100644 --- a/pkg/config/config.go +++ b/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)) diff --git a/pkg/config/config_helper.go b/pkg/config/config_helper.go index 77797acc4..84e944c3a 100644 --- a/pkg/config/config_helper.go +++ b/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 +} diff --git a/pkg/config/config_helper_test.go b/pkg/config/config_helper_test.go index b871f8157..4e823cfc6 100644 --- a/pkg/config/config_helper_test.go +++ b/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()) + } + }) + } +} diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 31382dc74..e9200fad5 100644 --- a/pkg/config/config_test.go +++ b/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)