From 20439aa6628e9da3fbc19c1d0527de19728ed97a Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Mon, 18 Feb 2019 11:49:41 +0000 Subject: [PATCH] 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 --- cli-plugins/examples/helloworld/main.go | 13 ++- cli/config/configfile/file.go | 90 +++++++++++++------ cli/config/configfile/file_test.go | 77 ++++++++-------- .../testdata/plugin-config-2.golden | 12 +++ .../configfile/testdata/plugin-config.golden | 2 +- e2e/cli-plugins/config_test.go | 34 +++++++ e2e/cli-plugins/help_test.go | 2 +- e2e/cli-plugins/run_test.go | 28 +++--- .../testdata/docker-help-helloworld.golden | 2 +- e2e/cli-plugins/util_test.go | 10 ++- 10 files changed, 185 insertions(+), 85 deletions(-) create mode 100644 cli/config/configfile/testdata/plugin-config-2.golden create mode 100644 e2e/cli-plugins/config_test.go diff --git a/cli-plugins/examples/helloworld/main.go b/cli-plugins/examples/helloworld/main.go index cbe015937f..e501fc37d4 100644 --- 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 index 338c8db780..c8d601162f 100644 --- 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 index badfa047d3..dc00572677 100644 --- 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) - assert.NilError(t, err) - assert.DeepEqual(t, p1, 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, "") - var p2bis PluginConfig2 - assert.Assert(t, is.Contains(configFile2.Plugins, "plugin2")) - err = json.Unmarshal(configFile2.Plugins["plugin2"], &p2bis) + // 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, 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") } diff --git a/cli/config/configfile/testdata/plugin-config-2.golden b/cli/config/configfile/testdata/plugin-config-2.golden new file mode 100644 index 0000000000..f7834bb0a3 --- /dev/null +++ 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" + } + } +} \ No newline at end of file diff --git a/cli/config/configfile/testdata/plugin-config.golden b/cli/config/configfile/testdata/plugin-config.golden index 45c5d11e6d..19141644e7 100644 --- 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 new file mode 100644 index 0000000000..e99ffc6bb4 --- /dev/null +++ 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 index 6ecdd4c7f0..7024e36bcd 100644 --- 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")) diff --git a/e2e/cli-plugins/run_test.go b/e2e/cli-plugins/run_test.go index e12c51025d..22fa474a69 100644 --- a/e2e/cli-plugins/run_test.go +++ b/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")) diff --git a/e2e/cli-plugins/testdata/docker-help-helloworld.golden b/e2e/cli-plugins/testdata/docker-help-helloworld.golden index 6ff36bc64e..089bee2caf 100644 --- a/e2e/cli-plugins/testdata/docker-help-helloworld.golden +++ b/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 diff --git a/e2e/cli-plugins/util_test.go b/e2e/cli-plugins/util_test.go index 0ab1aa9566..dca0bd8f18 100644 --- a/e2e/cli-plugins/util_test.go +++ b/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 }