Merge lp:~themue/juju-core/032-config-default-empty-string into lp:~go-bot/juju-core/trunk

[Frank Mueller]

charm: allow empty string as default value

The current solution is incompatible to pyjuju,
where empty strings as default values are allowed.
Added this as an exceptional case to the charm
config.

https://codereview.appspot.com/10682043/

charm: allow empty string as default value

The current solution is incompatible to pyjuju,
where empty strings as default values are allowed.
Added this as an exceptional case to the charm
config.

https://codereview.appspot.com/10682043/

[Frank Mueller (themue)]

Reviewers: mp+171779_code.launchpad.net,

Message:
Please take a look.

Description:
charm: allow empty string as default value

The current solution is incompatible to pyjuju,
where empty strings as default values are allowed.
Added this as an exceptional case to the charm
config.

https://code.launchpad.net/~themue/juju-core/032-config-default-empty-string/+merge/171779

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M charm/config.go
   M charm/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: tarmac-20130627084404-bq6fxhiknqub7hna
+New revision: <email address hidden>

Index: charm/config.go
=== modified file 'charm/config.go'
--- charm/config.go 2013-06-11 14:55:46 +0000
+++ charm/config.go 2013-06-27 11:50:08 +0000
@@ -110,10 +110,13 @@
    default:
     return nil, fmt.Errorf("invalid config: option %q has unknown type %q",
name, option.Type)
    }
- def := option.Default
- if option.Default, err = option.validate(name, def); err != nil {
- option.error(&err, name, def)
- return nil, fmt.Errorf("invalid config default: %v", err)
+ if !(option.Type == "string" && option.Default == "") {
+ // Empty default string is an exceptional case due to compatability
reasons.
+ def := option.Default
+ if option.Default, err = option.validate(name, def); err != nil {
+ option.error(&err, name, def)
+ return nil, fmt.Errorf("invalid config default: %v", err)
+ }
    }
    config.Options[name] = option
   }

Index: charm/config_test.go
=== modified file 'charm/config_test.go'
--- charm/config_test.go 2013-06-17 13:12:56 +0000
+++ charm/config_test.go 2013-06-27 11:50:08 +0000
@@ -26,6 +26,9 @@
      default: My Title
      description: A descriptive title used for the service.
      type: string
+ subtitle:
+ default: ""
+ description: An optional subtitle used for the service.
    outlook:
      description: No default outlook.
      # type defaults to string in python
@@ -53,6 +56,11 @@
     Description: "A descriptive title used for the service.",
     Type: "string",
    },
+ "subtitle": {
+ Default: "",
+ Description: "An optional subtitle used for the service.",
+ Type: "string",
+ },
    "username": {
     Default: "admin001",
     Description: "The name of the initial account (given admin
permissions).",
@@ -80,6 +88,7 @@
  func (s *ConfigSuite) TestDefaultSettings(c *C) {
   c.Assert(s.config.DefaultSettings(), DeepEquals, charm.Settings{
    "title": "My Title",
+ "subtitle": "",
    "username": "admin001",
    "outlook": nil,
    "skill-level": nil,
@@ -91,6 +100,7 @@
  func (s *ConfigSuite) TestFilterSettings(c *C) {
   settings := s.config.FilterSettings(charm.Settings{
    "title": "something valid",
+ "subtitle": "",
    "username": nil,
    "unknown": "whatever",
    "outlook":...

[Dimiter Naydenov (dimitern)]

A few thoughts

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

https://codereview.appspot.com/10682043/diff/1/charm/config.go#newcode114
charm/config.go:114: // Empty default string is an exceptional case due
to compatability reasons.
s/compatability/compatibility/ reasons with pyjuju.

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

https://codereview.appspot.com/10682043/diff/1/charm/config_test.go#newcode113
charm/config_test.go:113: "subtitle": nil,
shouldn't this be "" ?

https://codereview.appspot.com/10682043/

[William Reade (fwereade)]

LGTM with quibbles.

https://codereview.appspot.com/10682043/diff/1/charm/config.go
File charm/config.go (left):

https://codereview.appspot.com/10682043/diff/1/charm/config.go#oldcode117
charm/config.go:117: }
I would personally find something like this a bit easier to follow:

def := option.Default
if def == "" && option.Type == "string" {
     // Skip normal validation for compatibility with python.
} else if option.Default, err := option.validate(name, def); err != nil
{
     option.error(&err, name, def)
     return nil, fmt.Errorf("invalid config default: %v", err)
}

but YMMV, I guess, and there's no point bikeshedding this too hard,
because we'll have to deal with ""-everywhere before too long.

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

https://codereview.appspot.com/10682043/diff/1/charm/config_test.go#newcode103
charm/config_test.go:103: "subtitle": "",
Just skip this. You haven't changed the behaviour of FilterSettings, and
you're just duplicating a case that's already tested by "outlook".

https://codereview.appspot.com/10682043/diff/1/charm/config_test.go#newcode113
charm/config_test.go:113: "subtitle": nil,
On 2013/06/27 15:01:20, dimitern wrote:
> shouldn't this be "" ?

Nope -- all we've changed is ReadConfig, the usual validation stays the
same as before. Probably not for long, but one bug at a time...

https://codereview.appspot.com/10682043/

[Frank Mueller (themue)]

Please take a look.

https://codereview.appspot.com/10682043/diff/1/charm/config.go
File charm/config.go (left):

https://codereview.appspot.com/10682043/diff/1/charm/config.go#oldcode117
charm/config.go:117: }
On 2013/06/28 09:32:11, fwereade wrote:
> I would personally find something like this a bit easier to follow:

> def := option.Default
> if def == "" && option.Type == "string" {
> // Skip normal validation for compatibility with python.
> } else if option.Default, err := option.validate(name, def); err !=
nil {
> option.error(&err, name, def)
> return nil, fmt.Errorf("invalid config default: %v", err)
> }

> but YMMV, I guess, and there's no point bikeshedding this too hard,
because
> we'll have to deal with ""-everywhere before too long.

Done.

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

https://codereview.appspot.com/10682043/diff/1/charm/config.go#newcode114
charm/config.go:114: // Empty default string is an exceptional case due
to compatability reasons.
On 2013/06/27 15:01:20, dimitern wrote:
> s/compatability/compatibility/ reasons with pyjuju.

Changed with Williams proposal.

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

https://codereview.appspot.com/10682043/diff/1/charm/config_test.go#newcode103
charm/config_test.go:103: "subtitle": "",
On 2013/06/28 09:32:11, fwereade wrote:
> Just skip this. You haven't changed the behaviour of FilterSettings,
and you're
> just duplicating a case that's already tested by "outlook".

Done.

https://codereview.appspot.com/10682043/

[Dimiter Naydenov (dimitern)]

LGTM

https://codereview.appspot.com/10682043/diff/8001/charm/config.go
File charm/config.go (right):

https://codereview.appspot.com/10682043/diff/8001/charm/config.go#newcode114
charm/config.go:114: if def == "" && option.Type == "string" {
much nicer, thanks!

https://codereview.appspot.com/10682043/

=== modified file 'charm/config.go'
--- charm/config.go	2013-06-11 14:55:46 +0000
+++ charm/config.go	2013-06-28 13:17:23 +0000
@@ -111,7 +111,9 @@
 			return nil, fmt.Errorf("invalid config: option %q has unknown type %q", name, option.Type)
 		}
 		def := option.Default
-		if option.Default, err = option.validate(name, def); err != nil {
+		if def == "" && option.Type == "string" {
+			// Skip normal validation for compatibility with pyjuju.
+		} else if option.Default, err = option.validate(name, def); err != nil {
 			option.error(&err, name, def)
 			return nil, fmt.Errorf("invalid config default: %v", err)
 		}

=== modified file 'charm/config_test.go'
--- charm/config_test.go	2013-06-17 13:12:56 +0000
+++ charm/config_test.go	2013-06-28 13:17:23 +0000
@@ -26,6 +26,9 @@
     default: My Title
     description: A descriptive title used for the service.
     type: string
+  subtitle:
+    default: ""
+    description: An optional subtitle used for the service.
   outlook:
     description: No default outlook.
     # type defaults to string in python
@@ -53,6 +56,11 @@
 			Description: "A descriptive title used for the service.",
 			Type:        "string",
 		},
+		"subtitle": {
+			Default:     "",
+			Description: "An optional subtitle used for the service.",
+			Type:        "string",
+		},
 		"username": {
 			Default:     "admin001",
 			Description: "The name of the initial account (given admin permissions).",
@@ -80,6 +88,7 @@
 func (s *ConfigSuite) TestDefaultSettings(c *C) {
 	c.Assert(s.config.DefaultSettings(), DeepEquals, charm.Settings{
 		"title":              "My Title",
+		"subtitle":           "",
 		"username":           "admin001",
 		"outlook":            nil,
 		"skill-level":        nil,
@@ -376,6 +385,7 @@
 
 	assertDefault("boolean", "true", true)
 	assertDefault("string", "golden grahams", "golden grahams")
+	assertDefault("string", `""`, "")
 	assertDefault("float", "2.2e11", 2.2e11)
 	assertDefault("int", "99", int64(99))