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 <raliev@mirantis.com>
Relates-To: #348

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...)
+	}
+}

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)
-	}
-}

cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-no-flags.golden

@@ -1 +0,0 @@
-Context "context" not modified. No new options provided.

cmd/config/testdata/TestConfigSetContextGoldenOutput/config-cmd-set-context-too-many-args.golden

@@ -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
-

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

cmd/config/testdata/TestSetContextGoldenOutput/set-context.golden

@@ -1 +0,0 @@
-Context "dummycontext" created.

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

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

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)
 	}
 }
 

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

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) {

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)
 }