Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a field to the config file for plugin use. #1652

Merged
merged 3 commits into from
Feb 25, 2019

Conversation

ijc
Copy link
Contributor

@ijc ijc commented Feb 1, 2019

This is a bit manual (as the unit test attests) so we may find we want to add
some helpers/accessors, but this is enough to let plugins use it and to
preserve the information through round-trips.

Signed-off-by: Ian Campbell ijc@docker.com

- What I did

Added a map to the config.json top-level which can be used by plugins.

- How I did it

Added a map of string → json.RawMessage. Using RawMessage allows plugins to late bind on the structure without patching docker/cli.

- How to verify it

I added a unit test, but you could also port your plugin you use it ;-)

- Description for the changelog

N/A -- covered by #1564's entry.

- A picture of a cute animal (not mandatory but encouraged)

@codecov-io
Copy link

codecov-io commented Feb 1, 2019

Codecov Report

Merging #1652 into master will increase coverage by 0.04%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #1652      +/-   ##
==========================================
+ Coverage   56.07%   56.11%   +0.04%     
==========================================
  Files         306      306              
  Lines       20997    21019      +22     
==========================================
+ Hits        11774    11795      +21     
+ Misses       8373     8370       -3     
- Partials      850      854       +4

@thaJeztah
Copy link
Member

thaJeztah commented Feb 1, 2019

@ijc so, this would be configuration options for a plugin, right?

Some quick thoughts;

json.RawMessage would means, that "anything goes", right? So plugins can go crazy if they want to;

{
  "Plugins": {
    "hello": {
      "foobar": {
        "nestedfoobar" : {
          "centre-of-earth": true
        }
      },
      "bla": ["one", "two", "three"]
    }
  }
}

I know at the API level, we have options for plugins, and limit those to
a map[string]string. This can be somewhat limited (plugins may have to use workarounds such as "comma-separated values" and such), but keeps the format simple.

Another thing I'm wondering is;

Do we want plugins to use separate files, so that it's easier to;

  • ship them with a configuration file
  • keep it slightly more "separated" from the CLI

One note to add; we should rethink our configuration file format(s) (in general). JSON is great, but (e.g.) doesn't allow for annotated versions, which sometimes limits its use (i.e., we cannot ship a "sample" configuration file), and JSON is not always that user-friendly

Related: #1622, moby/moby#20151

@ijc
Copy link
Contributor Author

ijc commented Feb 1, 2019

json.RawMessage would means, that "anything goes", right? So plugins can go crazy if they want to;

Yes, that's the trade off between this or plugin authors having to patch docker/cli to add their custom fields. I think we should assume that plugin authors are generally sensible.

Do we want plugins to use separate files, so that it's easier to;

I think having a single file is nicer for users and the integration is a feature. In particular plugins are encouraged to use global config from config.json wherever possible and only use the reserved key where that is not possible or sensible. Having all plugin config (both generic and specific) in a single place make more sense to me (otherwise the split is pretty arbitrary).

There some more stuff on this in the design (#1534) which said explicitly that the config should be in a single file.

@ijc
Copy link
Contributor Author

ijc commented Feb 1, 2019

we should rethink our configuration file format(s)

That seems way out of scope for the particular change here.

@thaJeztah
Copy link
Member

Yes, that's the trade off between this or plugin authors having to patch docker/cli to add their custom fields. I think we should assume that plugin authors are generally sensible.

Would making it a map[string]string (could be either by name spacing configuration options in the key, or providing such a map per plugin) satisfy the requirement? so either;

{
  "Plugins": {
    "myplugin.option1": "value",
    "myplugin.option2": "value",
  }
}

or

{
  "Plugins": {
    "myplugin": {
      "option1": "value",
      "option2": "value"
    }
  }
}

@thaJeztah
Copy link
Member

Having all plugin config (both generic and specific) in a single place make more sense to me (otherwise the split is pretty arbitrary).

Yes, I think a single file could work for now

That seems way out of scope for the particular change here.

The change itself is definitely out of scope, but meant to say; try to keep it format agnostic, as JSON may become something else at some point (IOW, we should try and store the configuration itself in a known format, and not literal JSON)

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ijc
Copy link
Contributor Author

ijc commented Feb 6, 2019

Would making it a map[string]string (could be either by name spacing configuration options in the key, or providing such a map per plugin) satisfy the requirement

It'd work, but would mean plugins having to invent syntax for various structured stuff. So not ideal (but better than nothing). Maybe it's not a huge deal for the sorts of settings we imagine there being. Lists are annoying though since you need a separator, which means you need escaping etc. I suppose helpers might be the answer.

try to keep it format agnostic, as JSON may become something else at some point (IOW, we should try and store the configuration itself in a known format, and not literal JSON)

Perhaps this calls for the helpers/accessors I hypothesised in the commit message getting pull forward, so there would be

func (ConfigFile *c) PluginConfig(pluginname string, config interface{})
func (ConfigFile *c) SetPluginConfig(pluginname string, config interface{})

which abstracts the use of json.Unmarshal (or similar for other formats) away?

The thing passed as config would still potentially need to have the json:... or yaml:... (or toml:..., (or,...)) tags though, so maybe that doesn't help?

@ijc ijc mentioned this pull request Feb 7, 2019
11 tasks
@thaJeztah
Copy link
Member

Argh, thought I replied;

Lists are annoying though since you need a separator, which means you need escaping etc. I suppose helpers might be the answer.

Not sure I grasp this fully; my main thought was to limit the struct for per-plugin configuration to a map[string]string; equivalent to the configurations that are currently used in the tests, but not allowing plugins to expand beyond that for now; so basically, a plugin gets a key/value pairs for configuration (values being strings).

Perhaps this calls for the helpers/accessors I hypothesised in the commit message getting pull forward, so there would be

func (ConfigFile *c) PluginConfig(pluginname string, config interface{})
func (ConfigFile *c) SetPluginConfig(pluginname string, config interface{})

Should the first example return the config?

This could potentially be extended to;

func (ConfigFile *c) PluginConfig(pluginname string, option string)

So, e.g. configfile.PluginConfig("myplugin", "some-option") would return the value for option some-option ("foo" with the configuration file below);

{
  "Plugins": {
    "myplugin": {
      "some-option": "foo"
    }
}

Regarding SetPluginConfig - wondering if the configuration should be updatable by plugins; I guess this is a bit of a grey area that never was fully fledged out (should the config.json be managed by the user, and only read by the CLI, or should the CLI be in charge of updating it? It currently is a bit of a mix, but not well-defined).

@ijc
Copy link
Contributor Author

ijc commented Feb 11, 2019

Lists are annoying though since you need a separator, which means you need escaping etc. I suppose helpers might be the answer.

Not sure I grasp this fully; my main thought was to limit the struct for per-plugin configuration to a map[string]string; equivalent to the configurations that are currently used in the tests, but not allowing plugins to expand beyond that for now; so basically, a plugin gets a key/value pairs for configuration (values being strings).

What I meant was that if a plugin has a list of things to store (["thinga", "thingb", "thingc"]) then with a map[string]string they have to encode that list as a string, perhaps as thinga;thingb;thingc, but then thing* cannot contain a bare ; (or whichever character is chosen), so then you need to support \; to escape or some other way to allow ; in the values.

Should the first example return the config?

Not in what I was prpoposing, since you need to pass in a concrete thing (a struct in my mind) as the interface for the various reflection based marshalling things to have an actual thing to work to. I guess you could do

func (ConfigFile *c) PluginConfig(pluginname string, config reflect.Type) interface{}

but that's pretty awful.

Your proposal is slightly different since it takes a single named option for which it returns the value. That would work too. It's a bit flat and I would expect to see plugins working around this with foo.bar keys or whatever.

Perhaps I'm over thinking this and we should go with a simple option (e.g. map[string]string) until we have more data on what plugins really want to do -- at which point we can evolve things (which should be easier from a simpler starting point).

@silvin-lubecki
Copy link
Contributor

I think we need more feedback from the plugin developers themselves and see if they really need complex configuration struct or juste few feature flags.

My 2cts is they don't really need something complex and just a map[string]string will fit most of their needs, but I may be wrong.

My other 2cts is that changing afterwards from a map[string]string to a map[string]json.RawMessage is possible, the opposite isn't.

@ijc
Copy link
Contributor Author

ijc commented Feb 11, 2019

My other 2cts is that changing afterwards from a map[string]string to a map[string]json.RawMessage is possible, the opposite isn't.

Yep, that was what I was trying to say with "is possible, the opposite isn't".

I will rework this PR to use the map[string]string approach ASAP (which might still be a few days since I've got a few other things on)

@thaJeztah
Copy link
Member

Thanks! Yes, that was basically my train of thought as well; for daemon drivers (logging, storage-drivers) we went for a simple map[string]string (or could be a []string{"key=val","key=val"}) and (so far) that worked. So, yes, if it satisfies the immediate needs, I'd prefer to keep it simple, and extend in future (if there's a need).

@ijc
Copy link
Contributor Author

ijc commented Feb 18, 2019

OK, I switched to a simple map[string]string scheme. I added a couple of simple accessors since there's a bit of logic to cleanup dead entries.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code changes LGTM, thanks!

We'll have to update the example in https://github.com/docker/cli/blob/master/docs/reference/commandline/cli.md#configuration-files

(unfortunately it's already unwieldy, definitely a candidate for a rewrite, but that's another topic)

@ijc
Copy link
Contributor Author

ijc commented Feb 18, 2019

Thanks. I added a few words to that doc

@thaJeztah
Copy link
Member

thanks @ijc

ping @silvin-lubecki still LGTY?

Ian Campbell added 3 commits February 25, 2019 10:38
This is a bit manual (as the unit test attests) so we may find we want to add
some helpers/accessors, but this is enough to let plugins use it and to
preserve the information through round-trips.

Signed-off-by: Ian Campbell <ijc@docker.com>
Make it a simple `map[string]string` for now.

Added a unit test for it.

Signed-off-by: Ian Campbell <ijc@docker.com>
Signed-off-by: Ian Campbell <ijc@docker.com>
@ijc
Copy link
Contributor Author

ijc commented Feb 25, 2019

Rebased to resolve the conflict with #1654's edits to docs/extend/cli_plugins.md.

@silvin-lubecki silvin-lubecki merged commit cdba45b into docker:master Feb 25, 2019
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Feb 25, 2019
@ijc ijc deleted the plugins-config branch February 25, 2019 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants