Merge lp:~themue/juju-core/037-empty-strings-in-charm-config into lp:~go-bot/juju-core/trunk

[Frank Mueller]

charm: allow empty strings as config values

This is the second CL for lp:1194945. It allows empty strings
as valid values for config options with the type string. Merging
should be done after none has signalled a compatibility problem
as answer to the query mail on the mailing list.

https://codereview.appspot.com/12343045/

[Frank Mueller (themue)]

Reviewers: mp+178318_code.launchpad.net,

Message:
Please take a look.

Description:
charm: allow empty strings as config values

This is the second CL for lp:1194945. It allows empty strings
as valid values for config options with the type string. Merging
should be done after none has signalled a compatibility problem
as answer to the query mail on the mailing list.

https://code.launchpad.net/~themue/juju-core/037-empty-strings-in-charm-config/+merge/178318

Requires:
https://code.launchpad.net/~themue/juju-core/036-set-default/+merge/178279

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/12343045/

Affected files:
   A [revision details]
   M charm/config.go
   M charm/config_test.go
   M cmd/juju/config_test.go
   M state/service_test.go
   M state/statecmd/config_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: charm/config.go
=== modified file 'charm/config.go'
--- charm/config.go 2013-08-02 12:16:22 +0000
+++ charm/config.go 2013-08-02 14:54:09 +0000
@@ -45,7 +45,7 @@
   if checker := optionTypeCheckers[option.Type]; checker != nil {
    if value, err = checker.Coerce(value, nil); err != nil {
     return nil, err
- } else if value == "" {
+ } else if option.Type != "string" && value == "" {
     value = nil
    }
    return value, nil
@@ -64,7 +64,7 @@
  // returns an error if it cannot be parsed to the correct type. Empty
  // string values are returned as nil.
  func (option Option) parse(name, str string) (_ interface{}, err error) {
- if str == "" {
+ if option.Type != "string" && str == "" {
    return nil, nil
   }
   defer option.error(&err, name, str)

Index: charm/config_test.go
=== modified file 'charm/config_test.go'
--- charm/config_test.go 2013-07-19 14:49:06 +0000
+++ charm/config_test.go 2013-08-02 14:54:09 +0000
@@ -112,7 +112,7 @@
   c.Assert(settings, DeepEquals, charm.Settings{
    "title": "something valid",
    "username": nil,
- "outlook": nil,
+ "outlook": "",
   })
  }

@@ -149,10 +149,6 @@
     "reticulate-splines": true,
    },
   }, {
- info: "empty string-typed values become nil",
- input: charm.Settings{"outlook": ""},
- expect: charm.Settings{"outlook": nil},
- }, {
    info: "almost-correctly-typed values are valid",
    input: charm.Settings{
     "skill-level": 123,
@@ -201,6 +197,13 @@
   "reticulate-splines": nil,
  }

+var settingsWithEmptyString = charm.Settings{
+ "outlook": "",
+ "skill-level": nil,
+ "agility-ratio": nil,
+ "reticulate-splines": nil,
+}
+
  var settingsWithValues = charm.Settings{
   "outlook": "whatever",
   "skill-level": int64(123),
@@ -279,14 +282,14 @@
    key: "blah",
    expect: settingsWithNils,
   }, {
- info: "empty strings are considered nil",
+ info: "empty strings are considered nil for non-string types",
    yaml: `blah:
              outlook: ""
           ...

[Dimiter Naydenov (dimitern)]

LGTM

https://codereview.appspot.com/12343045/

[John A Meinel (jameinel)]

LGTM

https://codereview.appspot.com/12343045/

[William Reade (fwereade)]

Sorry, NOT LGTM without explanation. The API has lost/changed
functionality, I think, because we can no longer set string values to
defaults; and we've now got two ways to set defaults for non-string
types. How is all this going to work in the imminent CLI-API world?

https://codereview.appspot.com/12343045/diff/1/charm/config.go
File charm/config.go (right):

https://codereview.appspot.com/12343045/diff/1/charm/config.go#newcode39
charm/config.go:39: // are always considered valid, and empty string
values are converted to nil.
This isn't true any more.

https://codereview.appspot.com/12343045/diff/1/charm/config.go#newcode48
charm/config.go:48: } else if option.Type != "string" && value == "" {
Why are we keeping this behaviour? Having `value=` mean different things
depending on type seems like a bad idea to me. Can't we just delete this
whole special-case block?

https://codereview.appspot.com/12343045/diff/1/charm/config.go#newcode67
charm/config.go:67: if option.Type != "string" && str == "" {
Similarly.

https://codereview.appspot.com/12343045/diff/1/charm/config_test.go
File charm/config_test.go (right):

https://codereview.appspot.com/12343045/diff/1/charm/config_test.go#newcode337
charm/config_test.go:337: info: "empty strings are nil values for
non-string types",
Yeah -- why do we now have two ways of specifying set-to-default? I
don't think this is a good idea *unless* we have to support `value=` for
python compatibility.

https://codereview.appspot.com/12343045/diff/1/state/statecmd/config_test.go
File state/statecmd/config_test.go (right):

https://codereview.appspot.com/12343045/diff/1/state/statecmd/config_test.go#newcode90
state/statecmd/config_test.go:90: "value": "",
So how do we set defaults over the API?

https://codereview.appspot.com/12343045/

Unmerged revisions

1572. By Frank Mueller on 2013-08-02

charm: allowing empty strings as valid config value for string types

1571. By Frank Mueller on 2013-08-02

cmd/set: corrected the reset of an invalid change to be exact like before

1570. By Frank Mueller on 2013-08-02

cmd/set: merged trunk

1569. By Frank Mueller on 2013-08-02

cmd/set: removed setting to empty string until clearance

1568. By Frank Mueller on 2013-08-01

cmd/set: add option --default to set to default value

=== modified file 'charm/config.go'
--- charm/config.go	2013-07-19 14:49:06 +0000
+++ charm/config.go	2013-08-02 15:03:22 +0000
@@ -45,7 +45,7 @@
 	if checker := optionTypeCheckers[option.Type]; checker != nil {
 		if value, err = checker.Coerce(value, nil); err != nil {
 			return nil, err
-		} else if value == "" {
+		} else if option.Type != "string" && value == "" {
 			value = nil
 		}
 		return value, nil
@@ -64,7 +64,7 @@
 // returns an error if it cannot be parsed to the correct type. Empty
 // string values are returned as nil.
 func (option Option) parse(name, str string) (_ interface{}, err error) {
-	if str == "" {
+	if option.Type != "string" && str == "" {
 		return nil, nil
 	}
 	defer option.error(&err, name, str)

=== modified file 'charm/config_test.go'
--- charm/config_test.go	2013-07-19 14:49:06 +0000
+++ charm/config_test.go	2013-08-02 15:03:22 +0000
@@ -112,7 +112,7 @@
 	c.Assert(settings, DeepEquals, charm.Settings{
 		"title":    "something valid",
 		"username": nil,
-		"outlook":  nil,
+		"outlook":  "",
 	})
 }
 
@@ -149,10 +149,6 @@
 			"reticulate-splines": true,
 		},
 	}, {
-		info:   "empty string-typed values become nil",
-		input:  charm.Settings{"outlook": ""},
-		expect: charm.Settings{"outlook": nil},
-	}, {
 		info: "almost-correctly-typed values are valid",
 		input: charm.Settings{
 			"skill-level":   123,
@@ -201,6 +197,13 @@
 	"reticulate-splines": nil,
 }
 
+var settingsWithEmptyString = charm.Settings{
+	"outlook":            "",
+	"skill-level":        nil,
+	"agility-ratio":      nil,
+	"reticulate-splines": nil,
+}
+
 var settingsWithValues = charm.Settings{
 	"outlook":            "whatever",
 	"skill-level":        int64(123),
@@ -279,14 +282,14 @@
 		key:    "blah",
 		expect: settingsWithNils,
 	}, {
-		info: "empty strings are considered nil",
+		info: "empty strings are considered nil for non-string types",
 		yaml: `blah:
             outlook: ""
             skill-level: ""
             agility-ratio: ""
             reticulate-splines: ""`,
 		key:    "blah",
-		expect: settingsWithNils,
+		expect: settingsWithEmptyString,
 	}, {
 		info: "appropriate strings are valid",
 		yaml: `blah:
@@ -331,14 +334,14 @@
 		input:  map[string]string{},
 		expect: charm.Settings{},
 	}, {
-		info: "empty strings are nil values",
+		info: "empty strings are nil values for non-string types",
 		input: map[string]string{
 			"outlook":            "",
 			"skill-level":        "",
 			"agility-ratio":      "",
 			"reticulate-splines": "",
 		},
-		expect: settingsWithNils,
+		expect: settingsWithEmptyString,
 	}, {
 		info: "strings are converted",
 		input: map[string]string{

=== modified file 'cmd/juju/config_test.go'
--- cmd/juju/config_test.go	2013-08-02 15:03:22 +0000
+++ cmd/juju/config_test.go	2013-08-02 15:03:22 +0000
@@ -123,6 +123,13 @@
 		"outlook":  "[e-mail]",
 	},
 }, {
+	about: "set an option to an empty string",
+	args:  []string{"username="},
+	expect: charm.Settings{
+		"username": "",
+		"outlook":  "[e-mail]",
+	},
+}, {
 	about: "set to default value",
 	args:  []string{"--default", "username"},
 	expect: charm.Settings{

=== modified file 'state/service_test.go'
--- state/service_test.go	2013-07-18 16:46:59 +0000
+++ state/service_test.go	2013-08-02 15:03:22 +0000
@@ -344,10 +344,6 @@
 	about:  "unset missing string",
 	update: charm.Settings{"outlook": nil},
 }, {
-	about:   `empty strings unset string values`,
-	initial: charm.Settings{"outlook": "positive"},
-	update:  charm.Settings{"outlook": "", "title": ""},
-}, {
 	about:   "preserve existing value",
 	initial: charm.Settings{"title": "sir"},
 	update:  charm.Settings{"username": "admin001"},

=== modified file 'state/statecmd/config_test.go'
--- state/statecmd/config_test.go	2013-06-11 00:58:34 +0000
+++ state/statecmd/config_test.go	2013-08-02 15:03:22 +0000
@@ -73,7 +73,7 @@
 	about: "deployed service  #2",
 	charm: "dummy",
 	config: map[string]string{
-		// Empty string gives default
+		// Empty string sets to empty string
 		"title": "",
 		// Value when there's a default
 		"username": "foobie",
@@ -87,8 +87,7 @@
 			"title": map[string]interface{}{
 				"description": "A descriptive title used for the service.",
 				"type":        "string",
-				"value":       "My Title",
-				"default":     true,
+				"value":       "",
 			},
 			"outlook": map[string]interface{}{
 				"description": "No default outlook.",