Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add support for --config key=value for juju deploy #8206
Conversation
|
!!build!! |
|
!!build!! |
| + return errors.Trace(err) | ||
| + } | ||
| + if len(files) > 1 { | ||
| + return errors.Errorf("only a single config YAMl file can be specified, got %d", len(files)) |
| + // So we need to combine the two here to have only one. | ||
| + if apiRoot.BestFacadeVersion("Application") < 6 && len(appConfig) > 0 { | ||
| + var configFromFile map[string]map[interface{}]interface{} | ||
| + err := yaml.Unmarshal(configYAML, &configFromFile) |
axw
Dec 12, 2017
Member
configYAML is (was) intentionally not decoded on the client side
I'm not sure if there's a safe way to do this with gopkg.in/yaml. Looks like it'll use encoding.TextUnmarshaler, so probably that way. If it were json, I'd suggest unmarshalling into a map[string]json.RawMessage, combining those, and then marshalling them again.
IMO, just raise an error here and people can upgrade to a newer controller. Or at least defer this work.
wallyworld
Dec 12, 2017
Owner
Why do we not decode client side? We're not interpreting anything or coercing into values. We're simply adding extra values.
axw
Dec 12, 2017
Member
Why do we not decode client side?
Because we don't have the schema on the client side. We don't know which type to use. 42 could be a unmarshaled into any of string, int, or float. By unmarshaling into map[string]interface{}, you get the default-according-to-gopkg.in/yaml.
wallyworld
Dec 12, 2017
Owner
Right, but it goes across the wire as a string when we marshal back to yaml. So "42" will round trip back to "42" and will be parsed using the schema on the server side.
axw
Dec 12, 2017
Member
I chose a bad example, substitute in 42.0. That will be unmarshaled as float64 42. Float64 42 marshals as "42" and not "42.0".
If the charm setting was in fact a float, then that'd be fine. But if it's a string, you've dropped the ".0", which may not be fine.
wallyworld
Dec 12, 2017
Owner
I changed the type we unmarshal into to be:
map[string]charm.Settings
This preserves the string values as written into the YAML, ie "123.0" remains as "123.0" during the round trip as it is unmarshalled into a string value
axw
Dec 13, 2017
Member
charm.Settings is just a map[string]interface{}
I just tested this:
val := charm.Settings{}
yaml.Unmarshal([]byte("x: 42.0"), &val)
out, _ := yaml.Marshal(val)
fmt.Printf("%s\n", out)and the output was x: 42
wallyworld
Dec 13, 2017
Owner
I had it as map[string]string and mistakenly though charm.Settings was also map[string]string.
I changed it back.
val := map[string]string{}
yaml.Unmarshal([]byte("x: 42.0"), &val)
out, _ := yaml.Marshal(val)
fmt.Printf("%s\n", out)
prints x: "42.0"
| + var configFromFile map[string]map[interface{}]interface{} | ||
| + err := yaml.Unmarshal(configYAML, &configFromFile) | ||
| + if err != nil { | ||
| + return errors.Annotate(err, "badly formatted YAMl config file") |
| + charmSettings[k] = v | ||
| + } | ||
| + configFromFile[applicationName] = charmSettings | ||
| + configYAML, err = yaml.Marshal(configFromFile) |
wallyworld
Dec 12, 2017
Owner
it doesn't matter because if there's a configYAML, it will mask any config map entries
| @@ -136,3 +139,70 @@ func (m stringMap) String() string { | ||
| } | ||
| return strings.Join(pairs, ";") | ||
| } | ||
| + | ||
| +// configFlag records k=v attributes from command arguments |
axw
Dec 12, 2017
Member
most of this is the same as common.ConfigFlag - why not just modify that a bit? AFAICS, all you need is to add methods for accessing the k=v attrs and the filenames
wallyworld
Dec 12, 2017
Owner
I didn't want to break encapsulation. Either way works for me. I changed to use common.ConfigFlag
| + if fields[1] == "" { | ||
| + value = "" | ||
| + } else { | ||
| + if err := yaml.Unmarshal([]byte(fields[1]), &value); err != nil { |
axw
Dec 12, 2017
Member
I don't think we should be parsing the value as YAML for application config. Can we make that bit configurable please, and disable it for the "juju config" command?
| + } | ||
| + appConfig := make(map[string]string) | ||
| + for k, v := range attr { | ||
| + appConfig[k] = fmt.Sprintf("%v", v) |
axw
Dec 13, 2017
Member
should be safe to just v.(string), since you've said to preserve string values
| + preserveType bool | ||
| +} | ||
| + | ||
| +// SetPreserveType sets whether name values should be |
axw
Dec 13, 2017
Member
Based on how preserveType is used, I don't understand this at all. Is it meant to be preserving the string type? Or inferring the type based on the value's contents (i.e. decoding as YAML)?
Perhaps be a bit more literal, and make this
// SetDecodeYAMLValues controls whether or not config values
// are decoded as YAML into their default types, or to store the
// original string values. The default is to decode as YAML.
func (*ConfigFlag) SetDecodeYAMLValues(bool)
wallyworld
Dec 13, 2017
Owner
Changed to "SetPreserveStringValue" and expanded the comment to hopefully clarify.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
jujubot
merged commit fdcd718
into
juju:develop
Dec 13, 2017
1 check passed
added a commit
to wallyworld/juju
that referenced
this pull request
Dec 15, 2017
added a commit
that referenced
this pull request
Dec 15, 2017
wallyworld commentedDec 12, 2017
•
Edited 1 time
-
wallyworld
Dec 12, 2017
Description of change
juju deploy help claims that it supports
$ juju deploy mysql --config foo=bar
But it didn't. It only supported --config path/to/yaml
(where the yaml file wasn't a key values but a map kyed on charm name).
Add support for --config key=value along with a yaml file.
If more than one yaml file is specified, that's an error.
If both config name values and a yaml file are specified, the config name values take precedence over the values in the yaml file.
QA steps
bootstrap
deploy a charm and specify key=value and ensure these are used
Documentation changes
None because juju didn't behave like the doc said it should.