Code review comment for lp:~themue/juju-core/041-revert-empty-strings-in-charm-config

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: 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{

Index: state/service_test.go
=== 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:21:50 +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"},

Index: state/apiserver/client/get_test.go
=== 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:21:50 +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.",