From 3dd66f7685c997554f97e48d96541102381771f5 Mon Sep 17 00:00:00 2001 From: Ruslan Aliev Date: Fri, 26 Feb 2021 23:22:08 -0600 Subject: [PATCH] Change logic of config set-context cmd * command internal logic was revised and optimized * added ability to delete/reset context fields * unit tests were reorganized, new cases were added Change-Id: Ie35d11405e88fea21abf33cb75f44b03bb4644fd Signed-off-by: Ruslan Aliev Relates-To: #348 --- cmd/config/set_context.go | 78 +++++++----- cmd/config/set_context_test.go | 92 +------------- .../config-cmd-set-context-no-flags.golden | 1 - ...onfig-cmd-set-context-too-many-args.golden | 21 ---- .../config-cmd-set-context-with-help.golden | 7 +- .../set-context.golden | 1 - .../cli/airshipctl_config_set-context.md | 7 +- go.mod | 1 + pkg/config/config.go | 22 ++-- pkg/config/config_helper.go | 92 ++++++++------ pkg/config/config_helper_test.go | 119 +++++++++++++++--- pkg/config/config_test.go | 7 +- 12 files changed, 226 insertions(+), 222 deletions(-) delete mode 100644 cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-no-flags.golden delete mode 100644 cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-many-args.golden delete mode 100644 cmd/config/testdata/TestSetContextGoldenOutput/set-context.golden diff --git a/cmd/config/set_context.go b/cmd/config/set_context.go index a31cfdc5c..3be363516 100644 --- a/cmd/config/set_context.go +++ b/cmd/config/set_context.go @@ -15,9 +15,8 @@ limitations under the License. package config import ( - "fmt" - "github.com/spf13/cobra" + "github.com/spf13/pflag" "opendev.org/airship/airshipctl/pkg/config" ) @@ -37,6 +36,10 @@ airshipctl config set-context \ --current \ --manifest=exampleManifest ` + + setContextManifestFlag = "manifest" + setContextManagementConfigFlag = "management-config" + setContextCurrentFlag = "current" ) // NewSetContextCommand creates a command for creating and modifying contexts @@ -49,51 +52,60 @@ func NewSetContextCommand(cfgFactory config.Factory) *cobra.Command { Long: setContextLong[1:], Example: setContextExample, Args: cobra.MaximumNArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - nFlags := cmd.Flags().NFlag() - if len(args) == 1 { - // context name is made optional with --current flag added - o.Name = args[0] - } - if o.Name != "" && nFlags == 0 { - fmt.Fprintf(cmd.OutOrStdout(), "Context %q not modified. No new options provided.\n", o.Name) - return nil - } - - cfg, err := cfgFactory() - if err != nil { - return err - } - modified, err := config.RunSetContext(o, cfg, true) - - if err != nil { - return err - } - if modified { - fmt.Fprintf(cmd.OutOrStdout(), "Context %q modified.\n", o.Name) - } else { - fmt.Fprintf(cmd.OutOrStdout(), "Context %q created.\n", o.Name) - } - return nil - }, + RunE: setContextRunE(cfgFactory, o), } - addSetContextFlags(o, cmd) + addSetContextFlags(cmd, o) return cmd } -func addSetContextFlags(o *config.ContextOptions, cmd *cobra.Command) { +func addSetContextFlags(cmd *cobra.Command, o *config.ContextOptions) { flags := cmd.Flags() flags.StringVar( &o.Manifest, - "manifest", + setContextManifestFlag, "", "set the manifest for the specified context") + flags.StringVar( + &o.ManagementConfiguration, + setContextManagementConfigFlag, + "", + "set the management config for the specified context") + flags.BoolVar( &o.Current, - "current", + setContextCurrentFlag, false, "update the current context") } + +func setContextRunE(cfgFactory config.Factory, o *config.ContextOptions) func(cmd *cobra.Command, args []string) error { + return func(cmd *cobra.Command, args []string) error { + ctxName := "" + if len(args) == 1 { + ctxName = args[0] + } + + // Go through all the flags that have been set + var opts []config.ContextOption + fn := func(flag *pflag.Flag) { + switch flag.Name { + case setContextManifestFlag: + opts = append(opts, config.SetContextManifest(o.Manifest)) + case setContextManagementConfigFlag: + opts = append(opts, config.SetContextManagementConfig(o.ManagementConfiguration)) + } + } + cmd.Flags().Visit(fn) + + options := &config.RunSetContextOptions{ + CfgFactory: cfgFactory, + CtxName: ctxName, + Current: o.Current, + Writer: cmd.OutOrStdout(), + } + return options.RunSetContext(opts...) + } +} diff --git a/cmd/config/set_context_test.go b/cmd/config/set_context_test.go index 88a191eff..e50b0c798 100644 --- a/cmd/config/set_context_test.go +++ b/cmd/config/set_context_test.go @@ -15,109 +15,21 @@ limitations under the License. package config_test import ( - "fmt" - "strings" "testing" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - cmd "opendev.org/airship/airshipctl/cmd/config" - "opendev.org/airship/airshipctl/pkg/config" + "opendev.org/airship/airshipctl/cmd/config" "opendev.org/airship/airshipctl/testutil" ) -const ( - defaultManifest = "edge_cloud" -) - -type setContextTest struct { - givenConfig *config.Config - cmdTest *testutil.CmdTest - contextName string - manifest string -} - func TestConfigSetContext(t *testing.T) { cmdTests := []*testutil.CmdTest{ { Name: "config-cmd-set-context-with-help", CmdLine: "--help", - Cmd: cmd.NewSetContextCommand(nil), - }, - { - Name: "config-cmd-set-context-no-flags", - CmdLine: "context", - Cmd: cmd.NewSetContextCommand(nil), - }, - { - Name: "config-cmd-set-context-too-many-args", - CmdLine: "arg1 arg2", - Cmd: cmd.NewSetContextCommand(nil), - Error: fmt.Errorf("accepts at most %d arg(s), received %d", 1, 2), + Cmd: config.NewSetContextCommand(nil), }, } - for _, tt := range cmdTests { testutil.RunTest(t, tt) } } - -func TestSetContext(t *testing.T) { - given, cleanupGiven := testutil.InitConfig(t) - defer cleanupGiven(t) - - tests := []struct { - testName string - contextName string - flags []string - givenConfig *config.Config - manifest string - }{ - { - testName: "set-context", - contextName: "dummycontext", - flags: []string{ - "--manifest=" + defaultManifest, - }, - givenConfig: given, - manifest: defaultManifest, - }, - } - - for _, tt := range tests { - tt := tt - cmd := &testutil.CmdTest{ - Name: tt.testName, - CmdLine: fmt.Sprintf("%s %s", tt.contextName, strings.Join(tt.flags, " ")), - } - test := setContextTest{ - givenConfig: tt.givenConfig, - cmdTest: cmd, - contextName: tt.contextName, - manifest: tt.manifest, - } - test.run(t) - } -} - -func (test setContextTest) run(t *testing.T) { - // Get the Environment - settings := func() (*config.Config, error) { - return test.givenConfig, nil - } - - test.cmdTest.Cmd = cmd.NewSetContextCommand(settings) - testutil.RunTest(t, test.cmdTest) - - afterRunConf := test.givenConfig - - // Find the Context Created or Modified - afterRunContext, err := afterRunConf.GetContext(test.contextName) - require.NoError(t, err) - require.NotNil(t, afterRunContext) - - if test.manifest != "" { - assert.EqualValues(t, afterRunContext.Manifest, test.manifest) - } -} diff --git a/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-no-flags.golden b/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-no-flags.golden deleted file mode 100644 index 3880d99cc..000000000 --- a/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-no-flags.golden +++ /dev/null @@ -1 +0,0 @@ -Context "context" not modified. No new options provided. diff --git a/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-many-args.golden b/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-many-args.golden deleted file mode 100644 index a409d896d..000000000 --- a/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-many-args.golden +++ /dev/null @@ -1,21 +0,0 @@ -Error: accepts at most 1 arg(s), received 2 -Usage: - set-context NAME [flags] - -Examples: - -# Create a new context named "exampleContext" -airshipctl config set-context exampleContext \ - --manifest=exampleManifest \ - -# Update the manifest of the current-context -airshipctl config set-context \ - --current \ - --manifest=exampleManifest - - -Flags: - --current update the current context - -h, --help help for set-context - --manifest string set the manifest for the specified context - diff --git a/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-with-help.golden b/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-with-help.golden index 2780e1efd..18ce30d3d 100644 --- a/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-with-help.golden +++ b/cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-with-help.golden @@ -16,6 +16,7 @@ airshipctl config set-context \ Flags: - --current update the current context - -h, --help help for set-context - --manifest string set the manifest for the specified context + --current update the current context + -h, --help help for set-context + --management-config string set the management config for the specified context + --manifest string set the manifest for the specified context diff --git a/cmd/config/testdata/TestSetContextGoldenOutput/set-context.golden b/cmd/config/testdata/TestSetContextGoldenOutput/set-context.golden deleted file mode 100644 index ddc6cc4d3..000000000 --- a/cmd/config/testdata/TestSetContextGoldenOutput/set-context.golden +++ /dev/null @@ -1 +0,0 @@ -Context "dummycontext" created. diff --git a/docs/source/cli/airshipctl_config_set-context.md b/docs/source/cli/airshipctl_config_set-context.md index 098dd0f14..ce18d1afe 100644 --- a/docs/source/cli/airshipctl_config_set-context.md +++ b/docs/source/cli/airshipctl_config_set-context.md @@ -29,9 +29,10 @@ airshipctl config set-context \ ### Options ``` - --current update the current context - -h, --help help for set-context - --manifest string set the manifest for the specified context + --current update the current context + -h, --help help for set-context + --management-config string set the management config for the specified context + --manifest string set the manifest for the specified context ``` ### Options inherited from parent commands diff --git a/go.mod b/go.mod index f3fde238a..e5ed23e51 100644 --- a/go.mod +++ b/go.mod @@ -30,6 +30,7 @@ require ( github.com/opencontainers/image-spec v1.0.1 // indirect github.com/pkg/errors v0.9.1 github.com/spf13/cobra v1.0.0 + github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.6.1 golang.org/x/net v0.0.0-20201021035429-f5854403a974 // indirect golang.org/x/sys v0.0.0-20210119212857-b64e53b001e4 // indirect diff --git a/pkg/config/config.go b/pkg/config/config.go index 658ff08ad..d0c663c8b 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -173,6 +173,11 @@ func (c *Config) EnsureComplete() error { return nil } +// SetFs allows to set custom filesystem used in Config object. Required for unit tests +func (c *Config) SetFs(fsys fs.FileSystem) { + c.fileSystem = fsys +} + // PersistConfig updates the airshipctl config file to match // the current Config object. // If file did not previously exist, the file will be created. @@ -278,24 +283,21 @@ func (c *Config) GetManagementConfiguration(name string) (*ManagementConfigurati // AddContext creates a new context and returns the instance of // newly created context -func (c *Config) AddContext(theContext *ContextOptions) *Context { +func (c *Config) AddContext(ctxName string, opts ...ContextOption) *Context { // Create the new Airship config context nContext := NewContext() - c.Contexts[theContext.Name] = nContext + c.Contexts[ctxName] = nContext // Ok , I have initialized structs for the Context information // We can use Modify to populate the correct information - c.ModifyContext(nContext, theContext) + c.ModifyContext(nContext, opts...) return nContext } -// ModifyContext updates Context object with given given context options -func (c *Config) ModifyContext(context *Context, theContext *ContextOptions) { - if theContext.ManagementConfiguration != "" { - context.ManagementConfiguration = theContext.ManagementConfiguration - } - if theContext.Manifest != "" { - context.Manifest = theContext.Manifest +// ModifyContext updates Context object with given context options +func (c *Config) ModifyContext(context *Context, opts ...ContextOption) { + for _, o := range opts { + o(context) } } diff --git a/pkg/config/config_helper.go b/pkg/config/config_helper.go index 8d5b0af49..0a302062c 100644 --- a/pkg/config/config_helper.go +++ b/pkg/config/config_helper.go @@ -15,58 +15,72 @@ limitations under the License. package config import ( - "errors" + "fmt" + "io" ) +// ContextOption is a function that allows to modify context object +type ContextOption func(ctx *Context) + +// SetContextManifest sets manifest in context object +func SetContextManifest(manifest string) ContextOption { + return func(ctx *Context) { + ctx.Manifest = manifest + } +} + +// SetContextManagementConfig sets management config in context object +func SetContextManagementConfig(managementConfig string) ContextOption { + return func(ctx *Context) { + ctx.ManagementConfiguration = managementConfig + } +} + +// RunSetContextOptions are options required to create/modify airshipctl context +type RunSetContextOptions struct { + CfgFactory Factory + CtxName string + Current bool + Writer io.Writer +} + // RunSetContext validates the given command line options and invokes AddContext/ModifyContext -func RunSetContext(o *ContextOptions, airconfig *Config, writeToStorage bool) (bool, error) { - modified := false - err := o.Validate() +func (o *RunSetContextOptions) RunSetContext(opts ...ContextOption) error { + cfg, err := o.CfgFactory() if err != nil { - return modified, err + return err } + if o.Current { - if airconfig.CurrentContext == "" { - return modified, ErrMissingCurrentContext{} - } - // when --current flag is passed, use current context - o.Name = airconfig.CurrentContext + o.CtxName = cfg.CurrentContext } - context, err := airconfig.GetContext(o.Name) - if err != nil { - var cerr ErrMissingConfig - if !errors.As(err, &cerr) { - // An error occurred, but it wasn't a "missing" config error. - return modified, err - } + if o.CtxName == "" { + return ErrEmptyContextName{} + } - if o.CurrentContext { - return modified, ErrMissingConfig{} - } + infoMsg := fmt.Sprintf("context with name %s", o.CtxName) + context, err := cfg.GetContext(o.CtxName) + if err != nil { // context didn't exist, create it - // ignoring the returned added context - airconfig.AddContext(o) + cfg.AddContext(o.CtxName, opts...) + infoMsg = fmt.Sprintf("%s created\n", infoMsg) } else { - // Found the desired Current Context - // Lets update it and be done. - if o.CurrentContext { - airconfig.CurrentContext = o.Name - } else { - // Context exists, lets update - airconfig.ModifyContext(context, o) - } - modified = true + // Context exists, lets update + cfg.ModifyContext(context, opts...) + infoMsg = fmt.Sprintf("%s modified\n", infoMsg) + } + + // Verify we didn't break anything + if err = cfg.EnsureComplete(); err != nil { + return err + } + + if _, err := o.Writer.Write([]byte(infoMsg)); err != nil { + return err } // Update configuration file just in time persistence approach - if writeToStorage { - if err := airconfig.PersistConfig(true); err != nil { - // Error that it didnt persist the changes - return modified, ErrConfigFailed{} - } - } - - return modified, nil + return cfg.PersistConfig(true) } // RunUseContext validates the given context name and updates it as current context diff --git a/pkg/config/config_helper_test.go b/pkg/config/config_helper_test.go index 8d039954c..c9b920acc 100644 --- a/pkg/config/config_helper_test.go +++ b/pkg/config/config_helper_test.go @@ -15,34 +15,117 @@ limitations under the License. package config_test import ( + "bytes" + "os" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + kustfs "sigs.k8s.io/kustomize/api/filesys" "opendev.org/airship/airshipctl/pkg/config" "opendev.org/airship/airshipctl/testutil" + testfs "opendev.org/airship/airshipctl/testutil/fs" ) +func prepareConfig() func() (*config.Config, error) { + return func() (*config.Config, error) { + cfg := testutil.DummyConfig() + cfg.SetLoadedConfigPath("test") + cfg.SetFs(testfs.MockFileSystem{ + FileSystem: kustfs.MakeFsInMemory(), + MockChmod: func(s string, mode os.FileMode) error { + return nil + }, + MockDir: func(s string) string { + return "." + }, + }) + return cfg, nil + } +} + func TestRunSetContext(t *testing.T) { - t.Run("testAddContext", func(t *testing.T) { - conf := testutil.DummyConfig() - dummyContextOptions := testutil.DummyContextOptions() - dummyContextOptions.Name = "second_context" + ioBuffer := bytes.NewBuffer(nil) + tests := []struct { + name string + options config.RunSetContextOptions + ctxopts []config.ContextOption + expectedOut string + err error + }{ + { + name: "create new context", + options: config.RunSetContextOptions{ + CfgFactory: prepareConfig(), + CtxName: "new_context", + Current: false, + Writer: ioBuffer, + }, + ctxopts: []config.ContextOption{config.SetContextManifest("dummy_manifest"), + config.SetContextManagementConfig("dummy_management_config")}, + err: nil, + expectedOut: "context with name new_context created\n", + }, + { + name: "modify current context", + options: config.RunSetContextOptions{ + CfgFactory: prepareConfig(), + CtxName: "", + Current: true, + Writer: ioBuffer, + }, + ctxopts: []config.ContextOption{config.SetContextManagementConfig("")}, + err: nil, + expectedOut: "context with name dummy_context modified\n", + }, + { + name: "bad config", + options: config.RunSetContextOptions{ + CfgFactory: func() (*config.Config, error) { + return nil, config.ErrMissingConfig{What: "bad config"} + }, + }, + err: config.ErrMissingConfig{What: "bad config"}, + }, + { + name: "no context name provided", + options: config.RunSetContextOptions{ + CfgFactory: prepareConfig(), + CtxName: "", + Current: false, + }, + err: config.ErrEmptyContextName{}, + }, + { + name: "setup invalid manifest", + options: config.RunSetContextOptions{ + CfgFactory: prepareConfig(), + CtxName: "", + Current: true, + Writer: ioBuffer, + }, + ctxopts: []config.ContextOption{config.SetContextManifest("invalid_manifest")}, + err: config.ErrMissingConfig{What: "Current Context (dummy_context) does not identify a defined Manifest"}, + }, + } - modified, err := config.RunSetContext(dummyContextOptions, conf, false) - assert.NoError(t, err) - assert.False(t, modified) - assert.Contains(t, conf.Contexts, "second_context") - }) - - t.Run("testModifyContext", func(t *testing.T) { - conf := testutil.DummyConfig() - dummyContextOptions := testutil.DummyContextOptions() - - modified, err := config.RunSetContext(dummyContextOptions, conf, false) - assert.NoError(t, err) - assert.True(t, modified) - }) + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + ioBuffer.Reset() + err := tt.options.RunSetContext(tt.ctxopts...) + 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()) + } + }) + } } func TestRunUseContext(t *testing.T) { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index c91eeab3b..4f62c2a1d 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -257,7 +257,8 @@ func TestAddContext(t *testing.T) { defer cleanup(t) co := testutil.DummyContextOptions() - context := conf.AddContext(co) + context := conf.AddContext(co.Name, config.SetContextManifest(co.Manifest), + config.SetContextManagementConfig(co.ManagementConfiguration)) assert.EqualValues(t, conf.Contexts[co.Name], context) } @@ -266,10 +267,10 @@ func TestModifyContext(t *testing.T) { defer cleanup(t) co := testutil.DummyContextOptions() - context := conf.AddContext(co) + context := conf.AddContext(co.Name, config.SetContextManifest(co.Manifest)) co.Manifest += stringDelta - conf.ModifyContext(context, co) + conf.ModifyContext(context, config.SetContextManifest(co.Manifest)) assert.EqualValues(t, conf.Contexts[co.Name].Manifest, co.Manifest) assert.EqualValues(t, conf.Contexts[co.Name], context) }