Simplify cli plugin config file entry

Make it a simple `map[string]string` for now. Added a unit test for it. Signed-off-by: Ian Campbell <ijc@docker.com>
docker · Feb 25, 2019 · 20439aa · 20439aa
1 parent 4eb642b
commit 20439aa
Show file tree

Hide file tree

Showing 10 changed files with 184 additions and 84 deletions.
diff --git a/cli-plugins/examples/helloworld/main.go b/cli-plugins/examples/helloworld/main.go
@@ -41,12 +41,21 @@ func main() {
 			// the path where a plugin overrides this
 			// hook.
 			PersistentPreRunE: plugin.PersistentPreRunE,
-			Run: func(cmd *cobra.Command, args []string) {
+			RunE: func(cmd *cobra.Command, args []string) error {
+				if who == "" {
+					who, _ = dockerCli.ConfigFile().PluginConfig("helloworld", "who")
+				}
+				if who == "" {
+					who = "World"
+				}
+
 				fmt.Fprintf(dockerCli.Out(), "Hello %s!\n", who)
+				dockerCli.ConfigFile().SetPluginConfig("helloworld", "lastwho", who)
+				return dockerCli.ConfigFile().Save()
 			},
 		}
 		flags := cmd.Flags()
-		flags.StringVar(&who, "who", "World", "Who are we addressing?")
+		flags.StringVar(&who, "who", "", "Who are we addressing?")
 
 		cmd.AddCommand(goodbye, apiversion)
 		return cmd

diff --git a/cli/config/configfile/file.go b/cli/config/configfile/file.go
@@ -24,32 +24,32 @@ const (
 
 // ConfigFile ~/.docker/config.json file info
 type ConfigFile struct {
-	AuthConfigs          map[string]types.AuthConfig `json:"auths"`
-	HTTPHeaders          map[string]string           `json:"HttpHeaders,omitempty"`
-	PsFormat             string                      `json:"psFormat,omitempty"`
-	ImagesFormat         string                      `json:"imagesFormat,omitempty"`
-	NetworksFormat       string                      `json:"networksFormat,omitempty"`
-	PluginsFormat        string                      `json:"pluginsFormat,omitempty"`
-	VolumesFormat        string                      `json:"volumesFormat,omitempty"`
-	StatsFormat          string                      `json:"statsFormat,omitempty"`
-	DetachKeys           string                      `json:"detachKeys,omitempty"`
-	CredentialsStore     string                      `json:"credsStore,omitempty"`
-	CredentialHelpers    map[string]string           `json:"credHelpers,omitempty"`
-	Filename             string                      `json:"-"` // Note: for internal use only
-	ServiceInspectFormat string                      `json:"serviceInspectFormat,omitempty"`
-	ServicesFormat       string                      `json:"servicesFormat,omitempty"`
-	TasksFormat          string                      `json:"tasksFormat,omitempty"`
-	SecretFormat         string                      `json:"secretFormat,omitempty"`
-	ConfigFormat         string                      `json:"configFormat,omitempty"`
-	NodesFormat          string                      `json:"nodesFormat,omitempty"`
-	PruneFilters         []string                    `json:"pruneFilters,omitempty"`
-	Proxies              map[string]ProxyConfig      `json:"proxies,omitempty"`
-	Experimental         string                      `json:"experimental,omitempty"`
-	StackOrchestrator    string                      `json:"stackOrchestrator,omitempty"`
-	Kubernetes           *KubernetesConfig           `json:"kubernetes,omitempty"`
-	CurrentContext       string                      `json:"currentContext,omitempty"`
-	CLIPluginsExtraDirs  []string                    `json:"cliPluginsExtraDirs,omitempty"`
-	Plugins              map[string]json.RawMessage  `json:"plugins,omitempty"`
+	AuthConfigs          map[string]types.AuthConfig  `json:"auths"`
+	HTTPHeaders          map[string]string            `json:"HttpHeaders,omitempty"`
+	PsFormat             string                       `json:"psFormat,omitempty"`
+	ImagesFormat         string                       `json:"imagesFormat,omitempty"`
+	NetworksFormat       string                       `json:"networksFormat,omitempty"`
+	PluginsFormat        string                       `json:"pluginsFormat,omitempty"`
+	VolumesFormat        string                       `json:"volumesFormat,omitempty"`
+	StatsFormat          string                       `json:"statsFormat,omitempty"`
+	DetachKeys           string                       `json:"detachKeys,omitempty"`
+	CredentialsStore     string                       `json:"credsStore,omitempty"`
+	CredentialHelpers    map[string]string            `json:"credHelpers,omitempty"`
+	Filename             string                       `json:"-"` // Note: for internal use only
+	ServiceInspectFormat string                       `json:"serviceInspectFormat,omitempty"`
+	ServicesFormat       string                       `json:"servicesFormat,omitempty"`
+	TasksFormat          string                       `json:"tasksFormat,omitempty"`
+	SecretFormat         string                       `json:"secretFormat,omitempty"`
+	ConfigFormat         string                       `json:"configFormat,omitempty"`
+	NodesFormat          string                       `json:"nodesFormat,omitempty"`
+	PruneFilters         []string                     `json:"pruneFilters,omitempty"`
+	Proxies              map[string]ProxyConfig       `json:"proxies,omitempty"`
+	Experimental         string                       `json:"experimental,omitempty"`
+	StackOrchestrator    string                       `json:"stackOrchestrator,omitempty"`
+	Kubernetes           *KubernetesConfig            `json:"kubernetes,omitempty"`
+	CurrentContext       string                       `json:"currentContext,omitempty"`
+	CLIPluginsExtraDirs  []string                     `json:"cliPluginsExtraDirs,omitempty"`
+	Plugins              map[string]map[string]string `json:"plugins,omitempty"`
 }
 
 // ProxyConfig contains proxy configuration settings
@@ -71,7 +71,7 @@ func New(fn string) *ConfigFile {
 		AuthConfigs: make(map[string]types.AuthConfig),
 		HTTPHeaders: make(map[string]string),
 		Filename:    fn,
-		Plugins:     make(map[string]json.RawMessage),
+		Plugins:     make(map[string]map[string]string),
 	}
 }
 
@@ -332,6 +332,42 @@ func (configFile *ConfigFile) GetFilename() string {
 	return configFile.Filename
 }
 
+// PluginConfig retrieves the requested option for the given plugin.
+func (configFile *ConfigFile) PluginConfig(pluginname, option string) (string, bool) {
+	if configFile.Plugins == nil {
+		return "", false
+	}
+	pluginConfig, ok := configFile.Plugins[pluginname]
+	if !ok {
+		return "", false
+	}
+	value, ok := pluginConfig[option]
+	return value, ok
+}
+
+// SetPluginConfig sets the option to the given value for the given
+// plugin. Passing a value of "" will remove the option. If removing
+// the final config item for a given plugin then also cleans up the
+// overall plugin entry.
+func (configFile *ConfigFile) SetPluginConfig(pluginname, option, value string) {
+	if configFile.Plugins == nil {
+		configFile.Plugins = make(map[string]map[string]string)
+	}
+	pluginConfig, ok := configFile.Plugins[pluginname]
+	if !ok {
+		pluginConfig = make(map[string]string)
+		configFile.Plugins[pluginname] = pluginConfig
+	}
+	if value != "" {
+		pluginConfig[option] = value
+	} else {
+		delete(pluginConfig, option)
+	}
+	if len(pluginConfig) == 0 {
+		delete(configFile.Plugins, pluginname)
+	}
+}
+
 func checkKubernetesConfiguration(kubeConfig *KubernetesConfig) error {
 	if kubeConfig == nil {
 		return nil

diff --git a/cli/config/configfile/file_test.go b/cli/config/configfile/file_test.go
@@ -2,7 +2,6 @@ package configfile
 
 import (
 	"bytes"
-	"encoding/json"
 	"io/ioutil"
 	"os"
 	"testing"
@@ -437,31 +436,13 @@ func TestPluginConfig(t *testing.T) {
 	configFile := New("test-plugin")
 	defer os.Remove("test-plugin")
 
-	type PluginConfig1 struct {
-		Data1 string `json:"data1"`
-		Data2 int    `json:"data2"`
-	}
-	type PluginConfig2 struct {
-		Data3 string `json:"data3"`
-	}
-	p1 := PluginConfig1{
-		Data1: "some string",
-		Data2: 42,
-	}
-	p2 := PluginConfig2{
-		Data3: "some other string",
-	}
-
-	plugin1, err := json.MarshalIndent(p1, "", "\t")
-	assert.NilError(t, err)
-	configFile.Plugins["plugin1"] = plugin1
-
-	plugin2, err := json.MarshalIndent(p2, "", "\t")
-	assert.NilError(t, err)
-	configFile.Plugins["plugin2"] = plugin2
+	// Populate some initial values
+	configFile.SetPluginConfig("plugin1", "data1", "some string")
+	configFile.SetPluginConfig("plugin1", "data2", "42")
+	configFile.SetPluginConfig("plugin2", "data3", "some other string")
 
 	// Save a config file with some plugin config
-	err = configFile.Save()
+	err := configFile.Save()
 	assert.NilError(t, err)
 
 	// Read it back and check it has the expected content
@@ -471,25 +452,47 @@ func TestPluginConfig(t *testing.T) {
 
 	// Load it, resave and check again that the content is
 	// preserved through a load/save cycle.
-	configFile2 := New("test-plugin2")
+	configFile = New("test-plugin2")
 	defer os.Remove("test-plugin2")
-	assert.NilError(t, configFile2.LoadFromReader(bytes.NewReader(cfg)))
-	err = configFile2.Save()
+	assert.NilError(t, configFile.LoadFromReader(bytes.NewReader(cfg)))
+	err = configFile.Save()
 	assert.NilError(t, err)
 	cfg, err = ioutil.ReadFile("test-plugin2")
 	assert.NilError(t, err)
 	golden.Assert(t, string(cfg), "plugin-config.golden")
 
-	// Check that the contents was retained properly
-	var p1bis PluginConfig1
-	assert.Assert(t, is.Contains(configFile2.Plugins, "plugin1"))
-	err = json.Unmarshal(configFile2.Plugins["plugin1"], &p1bis)
+	// Check that the contents was reloaded properly
+	v, ok := configFile.PluginConfig("plugin1", "data1")
+	assert.Assert(t, ok)
+	assert.Equal(t, v, "some string")
+	v, ok = configFile.PluginConfig("plugin1", "data2")
+	assert.Assert(t, ok)
+	assert.Equal(t, v, "42")
+	v, ok = configFile.PluginConfig("plugin1", "data3")
+	assert.Assert(t, !ok)
+	assert.Equal(t, v, "")
+	v, ok = configFile.PluginConfig("plugin2", "data3")
+	assert.Assert(t, ok)
+	assert.Equal(t, v, "some other string")
+	v, ok = configFile.PluginConfig("plugin2", "data4")
+	assert.Assert(t, !ok)
+	assert.Equal(t, v, "")
+	v, ok = configFile.PluginConfig("plugin3", "data5")
+	assert.Assert(t, !ok)
+	assert.Equal(t, v, "")
+
+	// Add, remove and modify
+	configFile.SetPluginConfig("plugin1", "data1", "some replacement string") // replacing a key
+	configFile.SetPluginConfig("plugin1", "data2", "")                        // deleting a key
+	configFile.SetPluginConfig("plugin1", "data3", "some additional string")  // new key
+	configFile.SetPluginConfig("plugin2", "data3", "")                        // delete the whole plugin, since this was the only data
+	configFile.SetPluginConfig("plugin3", "data5", "a new plugin")            // add a new plugin
+
+	err = configFile.Save()
 	assert.NilError(t, err)
-	assert.DeepEqual(t, p1, p1bis)
 
-	var p2bis PluginConfig2
-	assert.Assert(t, is.Contains(configFile2.Plugins, "plugin2"))
-	err = json.Unmarshal(configFile2.Plugins["plugin2"], &p2bis)
+	// Read it back and check it has the expected content again
+	cfg, err = ioutil.ReadFile("test-plugin2")
 	assert.NilError(t, err)
-	assert.DeepEqual(t, p2, p2bis)
+	golden.Assert(t, string(cfg), "plugin-config-2.golden")
 }
diff --git a/cli/config/configfile/testdata/plugin-config-2.golden b/cli/config/configfile/testdata/plugin-config-2.golden
@@ -0,0 +1,12 @@
+{
+	"auths": {},
+	"plugins": {
+		"plugin1": {
+			"data1": "some replacement string",
+			"data3": "some additional string"
+		},
+		"plugin3": {
+			"data5": "a new plugin"
+		}
+	}
+}
diff --git a/cli/config/configfile/testdata/plugin-config.golden b/cli/config/configfile/testdata/plugin-config.golden
@@ -3,7 +3,7 @@
 	"plugins": {
 		"plugin1": {
 			"data1": "some string",
-			"data2": 42
+			"data2": "42"
 		},
 		"plugin2": {
 			"data3": "some other string"

diff --git a/e2e/cli-plugins/config_test.go b/e2e/cli-plugins/config_test.go
@@ -0,0 +1,34 @@
+package cliplugins
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/docker/cli/cli/config"
+	"gotest.tools/assert"
+	"gotest.tools/icmd"
+)
+
+func TestConfig(t *testing.T) {
+	run, cfg, cleanup := prepare(t)
+	defer cleanup()
+
+	cfg.SetPluginConfig("helloworld", "who", "Cambridge")
+	err := cfg.Save()
+	assert.NilError(t, err)
+
+	res := icmd.RunCmd(run("helloworld"))
+	res.Assert(t, icmd.Expected{
+		ExitCode: 0,
+		Out:      "Hello Cambridge!",
+	})
+
+	cfg2, err := config.Load(filepath.Dir(cfg.GetFilename()))
+	assert.NilError(t, err)
+	assert.DeepEqual(t, cfg2.Plugins, map[string]map[string]string{
+		"helloworld": {
+			"who":     "Cambridge",
+			"lastwho": "Cambridge",
+		},
+	})
+}
diff --git a/e2e/cli-plugins/help_test.go b/e2e/cli-plugins/help_test.go
@@ -13,7 +13,7 @@ import (
 
 // TestGlobalHelp ensures correct behaviour when running `docker help`
 func TestGlobalHelp(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("help"))