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

[Frank Mueller]

charm: reverted empty string branch

Reverted the branch allowing empty string values
in charm configurations due incompatability with
the API.

https://codereview.appspot.com/13349043/

charm: reverted empty string branch

Reverted the branch allowing empty string values
in charm configurations due incompatability with
the API.

https://codereview.appspot.com/13349043/

[Frank Mueller (themue)]

Reviewers: mp+182693_code.launchpad.net,

Message:
Please take a look.

Description:
charm: reverted empty string branch

Reverted the branch allowing empty string values
in charm configurations due incompatability with
the API.

https://code.launchpad.net/~themue/juju-core/041-revert-empty-strings-in-charm-config/+merge/182693

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M charm/config.go
   M charm/config_test.go
   M state/apiserver/client/get_test.go
   M state/service_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: tarmac-20130828123627-r38fc3kmubd7nucc
+New revision: <email address hidden>

Index: charm/config.go
=== modified file 'charm/config.go'
--- charm/config.go 2013-08-26 15:39:55 +0000
+++ charm/config.go 2013-08-28 16:21:50 +0000
@@ -45,6 +45,8 @@
   if checker := optionTypeCheckers[option.Type]; checker != nil {
    if value, err = checker.Coerce(value, nil); err != nil {
     return nil, err
+ } else if value == "" {
+ value = nil
    }
    return value, nil
   }
@@ -62,6 +64,9 @@
  // 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 == "" {
+ return nil, nil
+ }
   defer option.error(&err, name, str)
   switch option.Type {
   case "string":

Index: charm/config_test.go
=== modified file 'charm/config_test.go'
--- charm/config_test.go 2013-08-26 15:39:55 +0000
+++ charm/config_test.go 2013-08-28 16:21:50 +0000
@@ -112,7 +112,7 @@
   c.Assert(settings, gc.DeepEquals, charm.Settings{
    "title": "something valid",
    "username": nil,
- "outlook": "",
+ "outlook": nil,
   })
  }

@@ -149,9 +149,9 @@
     "reticulate-splines": true,
    },
   }, {
- info: "empty string-typed values stay empty",
+ info: "empty string-typed values become nil",
    input: charm.Settings{"outlook": ""},
- expect: charm.Settings{"outlook": ""},
+ expect: charm.Settings{"outlook": nil},
   }, {
    info: "almost-correctly-typed values are valid",
    input: charm.Settings{
@@ -279,14 +279,14 @@
    key: "blah",
    expect: settingsWithNils,
   }, {
- info: "empty strings for non string options are not accepted",
+ info: "empty strings are considered nil",
    yaml: `blah:
              outlook: ""
              skill-level: ""
              agility-ratio: ""
              reticulate-splines: ""`,
- key: "blah",
- err: `option "skill-level" expected int, got ""`,
+ key: "blah",
+ expect: settingsWithNils,
   }, {
    info: "appropriate strings are valid",
    yaml: `blah:
@@ -331,13 +331,14 @@
    input: map[string]string{},
    expect: charm.Settings{},
   }, {
- info: "empty strings for string options are valid",
- input: map[string]string{"outlook": ""},
- expect: charm.Settings{"outlook": ""},
- }, {
- info: "empty strings for non string options are invalid",
- input...

[John A Meinel (jameinel)]

Given it is just reverting a patch you and William agreed should be
pulled out,
LGTM

https://codereview.appspot.com/13349043/

=== modified file 'charm/config.go'
--- charm/config.go	2013-08-26 15:39:55 +0000
+++ charm/config.go	2013-08-28 16:26:36 +0000
@@ -45,6 +45,8 @@
 	if checker := optionTypeCheckers[option.Type]; checker != nil {
 		if value, err = checker.Coerce(value, nil); err != nil {
 			return nil, err
+		} else if value == "" {
+			value = nil
 		}
 		return value, nil
 	}
@@ -62,6 +64,9 @@
 // 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 == "" {
+		return nil, nil
+	}
 	defer option.error(&err, name, str)
 	switch option.Type {
 	case "string":

=== modified file 'charm/config_test.go'
--- charm/config_test.go	2013-08-26 15:39:55 +0000
+++ charm/config_test.go	2013-08-28 16:26:36 +0000
@@ -112,7 +112,7 @@
 	c.Assert(settings, gc.DeepEquals, charm.Settings{
 		"title":    "something valid",
 		"username": nil,
-		"outlook":  "",
+		"outlook":  nil,
 	})
 }
 
@@ -149,9 +149,9 @@
 			"reticulate-splines": true,
 		},
 	}, {
-		info:   "empty string-typed values stay empty",
+		info:   "empty string-typed values become nil",
 		input:  charm.Settings{"outlook": ""},
-		expect: charm.Settings{"outlook": ""},
+		expect: charm.Settings{"outlook": nil},
 	}, {
 		info: "almost-correctly-typed values are valid",
 		input: charm.Settings{
@@ -279,14 +279,14 @@
 		key:    "blah",
 		expect: settingsWithNils,
 	}, {
-		info: "empty strings for non string options are not accepted",
+		info: "empty strings are considered nil",
 		yaml: `blah:
             outlook: ""
             skill-level: ""
             agility-ratio: ""
             reticulate-splines: ""`,
-		key: "blah",
-		err: `option "skill-level" expected int, got ""`,
+		key:    "blah",
+		expect: settingsWithNils,
 	}, {
 		info: "appropriate strings are valid",
 		yaml: `blah:
@@ -331,13 +331,14 @@
 		input:  map[string]string{},
 		expect: charm.Settings{},
 	}, {
-		info:   "empty strings for string options are valid",
-		input:  map[string]string{"outlook": ""},
-		expect: charm.Settings{"outlook": ""},
-	}, {
-		info:  "empty strings for non string options are invalid",
-		input: map[string]string{"skill-level": ""},
-		err:   `option "skill-level" expected int, got ""`,
+		info: "empty strings are nil values",
+		input: map[string]string{
+			"outlook":            "",
+			"skill-level":        "",
+			"agility-ratio":      "",
+			"reticulate-splines": "",
+		},
+		expect: settingsWithNils,
 	}, {
 		info: "strings are converted",
 		input: map[string]string{

=== modified file 'state/apiserver/client/get_test.go'
--- state/apiserver/client/get_test.go	2013-08-28 11:20:26 +0000
+++ state/apiserver/client/get_test.go	2013-08-28 16:26:36 +0000
@@ -57,7 +57,9 @@
 		"title": "Look To Windward",
 		// Same as default.
 		"username": "admin001",
-		// Outlook and skill-level are left unset.
+		// Use default (but there's no charm default)
+		"skill-level": "",
+		// Outlook is left unset.
 	},
 	expect: params.ServiceGetResults{
 		Config: map[string]interface{}{
@@ -89,7 +91,7 @@
 	about: "deployed service  #2",
 	charm: "dummy",
 	config: map[string]string{
-		// Empty string sets empty string
+		// Empty string gives default
 		"title": "",
 		// Value when there's a default
 		"username": "foobie",
@@ -103,7 +105,8 @@
 			"title": map[string]interface{}{
 				"description": "A descriptive title used for the service.",
 				"type":        "string",
-				"value":       "",
+				"value":       "My Title",
+				"default":     true,
 			},
 			"outlook": map[string]interface{}{
 				"description": "No default outlook.",

=== modified file 'state/service_test.go'
--- state/service_test.go	2013-08-27 14:28:19 +0000
+++ state/service_test.go	2013-08-28 16:26:36 +0000
@@ -344,6 +344,10 @@
 	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"},