Add support for --config key=value for juju deploy #8206

wallyworld · 2017-12-12T05:28:15Z

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.

wallyworld · 2017-12-12T07:29:56Z

!!build!!

wallyworld · 2017-12-12T09:45:28Z

!!build!!

axw · 2017-12-12T09:28:03Z

cmd/juju/application/deploy.go

+		return errors.Trace(err)
+	}
+	if len(files) > 1 {
+		return errors.Errorf("only a single config YAMl file can be specified, got %d", len(files))


axw · 2017-12-12T09:32:11Z

cmd/juju/application/deploy.go

+		var configFromFile map[string]map[interface{}]interface{}
+		err := yaml.Unmarshal(configYAML, &configFromFile)
+		if err != nil {
+			return errors.Annotate(err, "badly formatted YAMl config file")


axw · 2017-12-12T09:37:05Z

cmd/juju/application/deploy.go

+	// 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)


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.

Why do we not decode client side? We're not interpreting anything or coercing into values. We're simply adding extra values.

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.

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.

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.

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

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

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"

axw · 2017-12-12T09:38:20Z

cmd/juju/application/deploy.go

+			charmSettings[k] = v
+		}
+		configFromFile[applicationName] = charmSettings
+		configYAML, err = yaml.Marshal(configFromFile)


shouldn't you be setting appConfig to nil somewhere here?

it doesn't matter because if there's a configYAML, it will mask any config map entries

true, but it would be nicer - no unnecessary data on the wire

axw · 2017-12-12T09:43:00Z

cmd/juju/application/flags.go

@@ -136,3 +139,70 @@ func (m stringMap) String() string {
 	}
 	return strings.Join(pairs, ";")
 }
+
+// configFlag records k=v attributes from command arguments


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

I didn't want to break encapsulation. Either way works for me. I changed to use common.ConfigFlag

axw · 2017-12-12T09:45:34Z

cmd/juju/application/flags.go

+	if fields[1] == "" {
+		value = ""
+	} else {
+		if err := yaml.Unmarshal([]byte(fields[1]), &value); err != nil {


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?

axw · 2017-12-13T01:14:54Z

cmd/juju/common/flags.go

+	preserveType bool
+}
+
+// SetPreserveType sets whether name values should be


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)

Changed to "SetPreserveStringValue" and expanded the comment to hopefully clarify.

axw · 2017-12-13T01:16:46Z

cmd/juju/application/deploy.go

+	}
+	appConfig := make(map[string]string)
+	for k, v := range attr {
+		appConfig[k] = fmt.Sprintf("%v", v)


should be safe to just v.(string), since you've said to preserve string values

wallyworld · 2017-12-13T01:39:49Z

$$merge$$

jujubot · 2017-12-13T01:40:09Z

Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju

jujubot · 2017-12-13T02:00:36Z

Build failed: Tests failed
build url: http://ci.jujucharms.com/job/github-merge-juju/670

wallyworld · 2017-12-13T02:14:27Z

$$merge$$

jujubot · 2017-12-13T02:16:07Z

Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju

Add support for --config key=value for juju deploy 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. bootstrap deploy a charm and specify key=value and ensure these are used None because juju didn't behave like the doc said it should.

Backport 2.4 fixes ## Description of change Backport 2 fixes: #8206 which adds support for juju deploy --config name=value #8227 which adds relation status on import if they are missing

wallyworld force-pushed the deploy-config branch 2 times, most recently from fc6aa0b to a21147b Compare December 12, 2017 07:13

wallyworld force-pushed the deploy-config branch 2 times, most recently from 7487bb1 to d5e2d19 Compare December 12, 2017 08:01

axw suggested changes Dec 12, 2017

View reviewed changes

wallyworld force-pushed the deploy-config branch 4 times, most recently from 635365a to b972898 Compare December 13, 2017 00:38

axw approved these changes Dec 13, 2017

View reviewed changes

wallyworld force-pushed the deploy-config branch from b972898 to a856a67 Compare December 13, 2017 01:39

Add support for --config key=value for juju deploy

b7bdf97

wallyworld force-pushed the deploy-config branch from a856a67 to b7bdf97 Compare December 13, 2017 02:14

jujubot merged commit fdcd718 into juju:develop Dec 13, 2017

wallyworld mentioned this pull request Dec 15, 2017

Backport 2.4 fixes #8230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for --config key=value for juju deploy #8206

Add support for --config key=value for juju deploy #8206

wallyworld commented Dec 12, 2017 •

edited

Loading

wallyworld commented Dec 12, 2017

wallyworld commented Dec 12, 2017

axw Dec 12, 2017

wallyworld Dec 12, 2017

axw Dec 12, 2017

wallyworld Dec 12, 2017

axw Dec 12, 2017

wallyworld Dec 12, 2017

axw Dec 12, 2017

wallyworld Dec 12, 2017

axw Dec 12, 2017

wallyworld Dec 12, 2017

axw Dec 13, 2017

wallyworld Dec 13, 2017

axw Dec 12, 2017

wallyworld Dec 12, 2017

axw Dec 12, 2017

wallyworld Dec 12, 2017

axw Dec 12, 2017

wallyworld Dec 12, 2017

axw Dec 12, 2017

wallyworld Dec 12, 2017

axw Dec 13, 2017

wallyworld Dec 13, 2017

axw Dec 13, 2017

wallyworld Dec 13, 2017

wallyworld commented Dec 13, 2017

jujubot commented Dec 13, 2017

jujubot commented Dec 13, 2017

wallyworld commented Dec 13, 2017

jujubot commented Dec 13, 2017

Add support for --config key=value for juju deploy #8206

Add support for --config key=value for juju deploy #8206

Conversation

wallyworld commented Dec 12, 2017 • edited Loading

Description of change

QA steps

Documentation changes

wallyworld commented Dec 12, 2017

wallyworld commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallyworld commented Dec 13, 2017

jujubot commented Dec 13, 2017

jujubot commented Dec 13, 2017

wallyworld commented Dec 13, 2017

jujubot commented Dec 13, 2017

wallyworld commented Dec 12, 2017 •

edited

Loading