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>

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

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"`
+	AuthConfigs          map[string]types.AuthConfig  `json:"auths"`
-	HTTPHeaders          map[string]string           `json:"HttpHeaders,omitempty"`
+	HTTPHeaders          map[string]string            `json:"HttpHeaders,omitempty"`
-	PsFormat             string                      `json:"psFormat,omitempty"`
+	PsFormat             string                       `json:"psFormat,omitempty"`
-	ImagesFormat         string                      `json:"imagesFormat,omitempty"`
+	ImagesFormat         string                       `json:"imagesFormat,omitempty"`
-	NetworksFormat       string                      `json:"networksFormat,omitempty"`
+	NetworksFormat       string                       `json:"networksFormat,omitempty"`
-	PluginsFormat        string                      `json:"pluginsFormat,omitempty"`
+	PluginsFormat        string                       `json:"pluginsFormat,omitempty"`
-	VolumesFormat        string                      `json:"volumesFormat,omitempty"`
+	VolumesFormat        string                       `json:"volumesFormat,omitempty"`
-	StatsFormat          string                      `json:"statsFormat,omitempty"`
+	StatsFormat          string                       `json:"statsFormat,omitempty"`
-	DetachKeys           string                      `json:"detachKeys,omitempty"`
+	DetachKeys           string                       `json:"detachKeys,omitempty"`
-	CredentialsStore     string                      `json:"credsStore,omitempty"`
+	CredentialsStore     string                       `json:"credsStore,omitempty"`
-	CredentialHelpers    map[string]string           `json:"credHelpers,omitempty"`
+	CredentialHelpers    map[string]string            `json:"credHelpers,omitempty"`
-	Filename             string                      `json:"-"` // Note: for internal use only
+	Filename             string                       `json:"-"` // Note: for internal use only
-	ServiceInspectFormat string                      `json:"serviceInspectFormat,omitempty"`
+	ServiceInspectFormat string                       `json:"serviceInspectFormat,omitempty"`
-	ServicesFormat       string                      `json:"servicesFormat,omitempty"`
+	ServicesFormat       string                       `json:"servicesFormat,omitempty"`
-	TasksFormat          string                      `json:"tasksFormat,omitempty"`
+	TasksFormat          string                       `json:"tasksFormat,omitempty"`
-	SecretFormat         string                      `json:"secretFormat,omitempty"`
+	SecretFormat         string                       `json:"secretFormat,omitempty"`
-	ConfigFormat         string                      `json:"configFormat,omitempty"`
+	ConfigFormat         string                       `json:"configFormat,omitempty"`
-	NodesFormat          string                      `json:"nodesFormat,omitempty"`
+	NodesFormat          string                       `json:"nodesFormat,omitempty"`
-	PruneFilters         []string                    `json:"pruneFilters,omitempty"`
+	PruneFilters         []string                     `json:"pruneFilters,omitempty"`
-	Proxies              map[string]ProxyConfig      `json:"proxies,omitempty"`
+	Proxies              map[string]ProxyConfig       `json:"proxies,omitempty"`
-	Experimental         string                      `json:"experimental,omitempty"`
+	Experimental         string                       `json:"experimental,omitempty"`
-	StackOrchestrator    string                      `json:"stackOrchestrator,omitempty"`
+	StackOrchestrator    string                       `json:"stackOrchestrator,omitempty"`
-	Kubernetes           *KubernetesConfig           `json:"kubernetes,omitempty"`
+	Kubernetes           *KubernetesConfig            `json:"kubernetes,omitempty"`
-	CurrentContext       string                      `json:"currentContext,omitempty"`
+	CurrentContext       string                       `json:"currentContext,omitempty"`
-	CLIPluginsExtraDirs  []string                    `json:"cliPluginsExtraDirs,omitempty"`
+	CLIPluginsExtraDirs  []string                     `json:"cliPluginsExtraDirs,omitempty"`
-	Plugins              map[string]json.RawMessage  `json:"plugins,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

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 {
+	// Populate some initial values
-		Data1 string `json:"data1"`
+	configFile.SetPluginConfig("plugin1", "data1", "some string")
-		Data2 int    `json:"data2"`
+	configFile.SetPluginConfig("plugin1", "data2", "42")
-	}
+	configFile.SetPluginConfig("plugin2", "data3", "some other string")
-	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
 
 	// 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)))
+	assert.NilError(t, configFile.LoadFromReader(bytes.NewReader(cfg)))
-	err = configFile2.Save()
+	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
+	// Check that the contents was reloaded properly
-	var p1bis PluginConfig1
+	v, ok := configFile.PluginConfig("plugin1", "data1")
-	assert.Assert(t, is.Contains(configFile2.Plugins, "plugin1"))
+	assert.Assert(t, ok)
-	err = json.Unmarshal(configFile2.Plugins["plugin1"], &p1bis)
+	assert.Equal(t, v, "some string")
-	assert.NilError(t, err)
+	v, ok = configFile.PluginConfig("plugin1", "data2")
-	assert.DeepEqual(t, p1, p1bis)
+	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, "")
 
-	var p2bis PluginConfig2
+	// Add, remove and modify
-	assert.Assert(t, is.Contains(configFile2.Plugins, "plugin2"))
+	configFile.SetPluginConfig("plugin1", "data1", "some replacement string") // replacing a key
-	err = json.Unmarshal(configFile2.Plugins["plugin2"], &p2bis)
+	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, p2, p2bis)
+
+	// Read it back and check it has the expected content again
+	cfg, err = ioutil.ReadFile("test-plugin2")
+	assert.NilError(t, err)
+	golden.Assert(t, string(cfg), "plugin-config-2.golden")
 }

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"
+		}
+	}
+}

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"

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",
+		},
+	})
+}

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

e2e/cli-plugins/run_test.go

@@ -11,7 +11,7 @@ import (
 
 // TestRunNonexisting ensures correct behaviour when running a nonexistent plugin.
 func TestRunNonexisting(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("nonexistent"))
@@ -24,7 +24,7 @@ func TestRunNonexisting(t *testing.T) {
 
 // TestHelpNonexisting ensures correct behaviour when invoking help on a nonexistent plugin.
 func TestHelpNonexisting(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("help", "nonexistent"))
@@ -38,7 +38,7 @@ func TestHelpNonexisting(t *testing.T) {
 // TestNonexistingHelp ensures correct behaviour when invoking a
 // nonexistent plugin with `--help`.
 func TestNonexistingHelp(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("nonexistent", "--help"))
@@ -53,7 +53,7 @@ func TestNonexistingHelp(t *testing.T) {
 
 // TestRunBad ensures correct behaviour when running an existent but invalid plugin
 func TestRunBad(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("badmeta"))
@@ -66,7 +66,7 @@ func TestRunBad(t *testing.T) {
 
 // TestHelpBad ensures correct behaviour when invoking help on a existent but invalid plugin.
 func TestHelpBad(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("help", "badmeta"))
@@ -80,7 +80,7 @@ func TestHelpBad(t *testing.T) {
 // TestBadHelp ensures correct behaviour when invoking an
 // existent but invalid plugin with `--help`.
 func TestBadHelp(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("badmeta", "--help"))
@@ -95,7 +95,7 @@ func TestBadHelp(t *testing.T) {
 
 // TestRunGood ensures correct behaviour when running a valid plugin
 func TestRunGood(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("helloworld"))
@@ -109,7 +109,7 @@ func TestRunGood(t *testing.T) {
 // valid plugin. A global argument is included to ensure it does not
 // interfere.
 func TestHelpGood(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("-D", "help", "helloworld"))
@@ -122,7 +122,7 @@ func TestHelpGood(t *testing.T) {
 // with `--help`. A global argument is used to ensure it does not
 // interfere.
 func TestGoodHelp(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("-D", "helloworld", "--help"))
@@ -134,7 +134,7 @@ func TestGoodHelp(t *testing.T) {
 
 // TestRunGoodSubcommand ensures correct behaviour when running a valid plugin with a subcommand
 func TestRunGoodSubcommand(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("helloworld", "goodbye"))
@@ -146,7 +146,7 @@ func TestRunGoodSubcommand(t *testing.T) {
 
 // TestRunGoodArgument ensures correct behaviour when running a valid plugin with an `--argument`.
 func TestRunGoodArgument(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("helloworld", "--who", "Cleveland"))
@@ -160,7 +160,7 @@ func TestRunGoodArgument(t *testing.T) {
 // valid plugin subcommand. A global argument is included to ensure it does not
 // interfere.
 func TestHelpGoodSubcommand(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("-D", "help", "helloworld", "goodbye"))
@@ -173,7 +173,7 @@ func TestHelpGoodSubcommand(t *testing.T) {
 // with a subcommand and `--help`. A global argument is used to ensure it does not
 // interfere.
 func TestGoodSubcommandHelp(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("-D", "helloworld", "goodbye", "--help"))
@@ -186,7 +186,7 @@ func TestGoodSubcommandHelp(t *testing.T) {
 // TestCliInitialized tests the code paths which ensure that the Cli
 // object is initialized even if the plugin uses PersistentRunE
 func TestCliInitialized(t *testing.T) {
-	run, cleanup := prepare(t)
+	run, _, cleanup := prepare(t)
 	defer cleanup()
 
 	res := icmd.RunCmd(run("helloworld", "apiversion"))

e2e/cli-plugins/testdata/docker-help-helloworld.golden

@@ -4,7 +4,7 @@ Usage:	docker helloworld [OPTIONS] COMMAND
 A basic Hello World plugin for tests
 
 Options:
-      --who string   Who are we addressing? (default "World")
+      --who string   Who are we addressing?
 
 Commands:
   apiversion  Print the API version of the server

e2e/cli-plugins/util_test.go

@@ -5,11 +5,14 @@ import (
 	"os"
 	"testing"
 
+	"github.com/docker/cli/cli/config"
+	"github.com/docker/cli/cli/config/configfile"
+	"gotest.tools/assert"
 	"gotest.tools/fs"
 	"gotest.tools/icmd"
 )
 
-func prepare(t *testing.T) (func(args ...string) icmd.Cmd, func()) {
+func prepare(t *testing.T) (func(args ...string) icmd.Cmd, *configfile.ConfigFile, func()) {
 	cfg := fs.NewDir(t, "plugin-test",
 		fs.WithFile("config.json", fmt.Sprintf(`{"cliPluginsExtraDirs": [%q]}`, os.Getenv("DOCKER_CLI_E2E_PLUGINS_EXTRA_DIRS"))),
 	)
@@ -19,6 +22,9 @@ func prepare(t *testing.T) (func(args ...string) icmd.Cmd, func()) {
 	cleanup := func() {
 		cfg.Remove()
 	}
-	return run, cleanup
+	cfgfile, err := config.Load(cfg.Path())
+	assert.NilError(t, err)
+
+	return run, cfgfile, cleanup
 
 }