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

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: ""
           ...

LGTM

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

LGTM

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

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/