Merge lp:~wallyworld/juju-core/set-env-boolean into lp:~go-bot/juju-core/trunk

[Ian Booth]

Allow strings as bool/int config values

Fix the parsing of config values to allow suitable
string values to be used to set bool and int
attributes. eg "true" mean true, "42" means 42 etc.
This is needed to allow juju set-env to work for those
types of values.

https://codereview.appspot.com/33680043/

Allow strings as bool/int config values

Fix the parsing of config values to allow suitable
string values to be used to set bool and int
attributes. eg "true" mean true, "42" means 42 etc.
This is needed to allow juju set-env to work for those
types of values.

https://codereview.appspot.com/33680043/

[Ian Booth (wallyworld)]

Reviewers: mp+196819_code.launchpad.net,

Message:
Please take a look.

Description:
Allow strings as bool/int config values

Fix the parsing of config values to allow suitable
string values to be used to set bool and int
attributes. eg "true" mean true, "42" means 42 etc.
This is needed to allow juju set-env to work for those
types of values.

https://code.launchpad.net/~wallyworld/juju-core/set-env-boolean/+merge/196819

(do not edit description out of merge proposal)

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

Affected files (+61, -34 lines):
   A [revision details]
   M cmd/juju/environment_test.go
   M schema/schema.go
   M schema/schema_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-20131126164544-1janyqs4x0e2mt9l
+New revision: <email address hidden>

Index: schema/schema.go
=== modified file 'schema/schema.go'
--- schema/schema.go 2013-09-26 23:26:19 +0000
+++ schema/schema.go 2013-11-27 05:21:41 +0000
@@ -102,8 +102,11 @@
  type boolC struct{}

  func (c boolC) Coerce(v interface{}, path []string) (interface{}, error) {
- if v != nil && reflect.TypeOf(v).Kind() == reflect.Bool {
- return v, nil
+ if v != nil {
+ val, err := strconv.ParseBool(fmt.Sprintf("%v", v))
+ if err == nil {
+ return val, nil
+ }
   }
   return nil, error_{"bool", v, path}
  }
@@ -117,19 +120,13 @@
  type intC struct{}

  func (c intC) Coerce(v interface{}, path []string) (interface{}, error) {
- if v == nil {
- return nil, error_{"int", v, path}
- }
- switch reflect.TypeOf(v).Kind() {
- case reflect.Int:
- case reflect.Int8:
- case reflect.Int16:
- case reflect.Int32:
- case reflect.Int64:
- default:
- return nil, error_{"int", v, path}
- }
- return reflect.ValueOf(v).Int(), nil
+ if v != nil {
+ val, err := strconv.ParseInt(fmt.Sprintf("%v", v), 0, 64)
+ if err == nil {
+ return val, nil
+ }
+ }
+ return nil, error_{"int", v, path}
  }

  // ForceInt returns a Checker that accepts any integer or float value, and
@@ -143,14 +140,18 @@
  type forceIntC struct{}

  func (c forceIntC) Coerce(v interface{}, path []string) (interface{},
error) {
- if v == nil {
- return nil, error_{"number", v, path}
- }
- switch reflect.TypeOf(v).Kind() {
- case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32,
reflect.Int64:
- return int(reflect.ValueOf(v).Int()), nil
- case reflect.Float32, reflect.Float64:
- return int(reflect.ValueOf(v).Float()), nil
+ if v != nil {
+ switch reflect.TypeOf(v).Kind() {
+ case reflect.String:
+ intValue, err := strconv.ParseInt(fmt.Sprintf("%v", v), 0, 64)
+ if err == nil {
+ return int(intValue), nil
+ }
+ case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32,
reflect.Int64:
+ return int(reflect.ValueOf(v).Int()), nil
+ case reflect.Float32, reflect.Float64:
+ return int(reflect.ValueOf(v).Float()), nil
+ }
   }
   return nil, error_{"number", v, path}
  }

Index: schema/schema_test.go
=== modified file 'schema/schema_test.go'
--- schema/schema_test.go 2013-09-27 01:50:46 +0000
...

[Roger Peppe (rogpeppe)]

Looks good, thanks, with a few suggestions below.

https://codereview.appspot.com/33680043/diff/1/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode105
schema/schema.go:105: if v != nil {
I think I'd be inclined to do something like this:

func (c boolC) Coerce(v interface{}, path []string) (interface{}, error)
{
     switch vv := reflect.TypeOf(v); vv.Kind() {
     case reflect.Bool:
         return vv.Bool(), nil
     case reflect.String:
         val, err := strconv.ParseBool(vv.String())
         if err == nil {
             return val, nil
         }
     }
     return nil, error{"bool", v, path}
}

the Sprintf seems unnecessary overkill.

https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode123
schema/schema.go:123: if v != nil {
similar here

https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode146
schema/schema.go:146: intValue, err :=
strconv.ParseInt(fmt.Sprintf("%v", v), 0, 64)
Again, no need to use Sprintf. And given that we allow float values, we
should probably allow float string values too.

switch vv := reflect.ValueOf(v); vv.Kind() {
case reflect.String:
    intValue, err := strconv.ParseInt(vv.String(), 0, 64)
    if err == nil {
        return int(intValue), nil
    }
    floatValue, err := strconv.ParseFloat(vv.String(), 0, 64)
    if err = nil {
        return int(floatValue), nil
    }
case reflect.Int, reflect.Int8, etc etc

https://codereview.appspot.com/33680043/diff/1/schema/schema_test.go
File schema/schema_test.go (right):

https://codereview.appspot.com/33680043/diff/1/schema/schema_test.go#newcode92
schema/schema_test.go:92: c.Assert(err, gc.ErrorMatches, `<path>:
expected bool, got int\(42\)`)
could we have a string value that fails too here, please?

https://codereview.appspot.com/33680043/diff/1/schema/schema_test.go#newcode161
schema/schema_test.go:161: c.Assert(err, gc.ErrorMatches, "<path>:
expected number, got nothing")
ditto.
and a test for a string containing a float value.

https://codereview.appspot.com/33680043/

[Ian Booth (wallyworld)]

https://codereview.appspot.com/33680043/diff/1/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode105
schema/schema.go:105: if v != nil {
On 2013/11/27 09:00:08, rog wrote:
> I think I'd be inclined to do something like this:

> func (c boolC) Coerce(v interface{}, path []string) (interface{},
error) {
> switch vv := reflect.TypeOf(v); vv.Kind() {
> case reflect.Bool:
> return vv.Bool(), nil
> case reflect.String:
> val, err := strconv.ParseBool(vv.String())
> if err == nil {
> return val, nil
> }
> }
> return nil, error{"bool", v, path}
> }

> the Sprintf seems unnecessary overkill.

Why all that logic when strconv.ParseBool() does what we need with fewer
lines of code required? I can't see a reason for the reflection etc.

https://codereview.appspot.com/33680043/

[Roger Peppe (rogpeppe)]

On 27 November 2013 09:40, <email address hidden> wrote:
>
> https://codereview.appspot.com/33680043/diff/1/schema/schema.go
> File schema/schema.go (right):
>
> https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode105
> schema/schema.go:105: if v != nil {
> On 2013/11/27 09:00:08, rog wrote:
>>
>> I think I'd be inclined to do something like this:
>
>
>> func (c boolC) Coerce(v interface{}, path []string) (interface{},
>
> error) {
>>
>> switch vv := reflect.TypeOf(v); vv.Kind() {
>> case reflect.Bool:
>> return vv.Bool(), nil
>> case reflect.String:
>> val, err := strconv.ParseBool(vv.String())
>> if err == nil {
>> return val, nil
>> }
>> }
>> return nil, error{"bool", v, path}
>> }
>
>
>> the Sprintf seems unnecessary overkill.
>
>
> Why all that logic when strconv.ParseBool() does what we need with fewer
> lines of code required? I can't see a reason for the reflection etc.

For a start, my suggested code is more efficient and does no
unnecessary allocation, which is a good property to have in
a low level part of the code like this.

Secondly, your code will accept any type at all, as long as Sprintf
happens to marshal it to something that looks like a bool.
I'd much prefer to keep things more limited here, so that we
know exactly what types we accept.

My suggested code might be slightly longer, but it's much more
explicit in its behaviour.

[William Reade (fwereade)]

On 2013/11/27 10:14:39, rog wrote:
> Secondly, your code will accept any type at all, as long as Sprintf
> happens to marshal it to something that looks like a bool.
> I'd much prefer to keep things more limited here, so that we
> know exactly what types we accept.

For me, that's the clincher. I think there's actually a pretty strong
argument to be made for maximally-forgiving checkers in the schema
package, but I'd prefer to keep the change as limited as possible given
that we're rolling it into 1.16 and I don't want unnecessary divergence.

FWIW, roger, I am very much unconvinced by the efficiency argument here
when viewed in the light of the opinions you expressed earlier today re
rpc calls ;p.

So, with the reflect-based version, LGTM.

https://codereview.appspot.com/33680043/

[Go Bot (go-bot)]

The attempt to merge lp:~wallyworld/juju-core/set-env-boolean into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.742s
ok launchpad.net/juju-core/agent/tools 0.226s
ok launchpad.net/juju-core/bzr 6.865s
ok launchpad.net/juju-core/cert 3.843s
ok launchpad.net/juju-core/charm 0.578s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.027s
ok launchpad.net/juju-core/cmd 0.220s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 194.242s
ok launchpad.net/juju-core/cmd/jujud 56.731s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 2.815s
ok launchpad.net/juju-core/constraints 0.028s
ok launchpad.net/juju-core/container 0.034s
ok launchpad.net/juju-core/container/factory 0.074s
ok launchpad.net/juju-core/container/kvm 0.319s
ok launchpad.net/juju-core/container/kvm/mock 0.034s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.343s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
ok launchpad.net/juju-core/downloader 5.312s
ok launchpad.net/juju-core/environs 3.167s
ok launchpad.net/juju-core/environs/bootstrap 4.996s
ok launchpad.net/juju-core/environs/cloudinit 0.544s

----------------------------------------------------------------------
FAIL: config_test.go:581: ConfigSuite.TestConfig

test 0. The minimum good configuration
test 1. Metadata URLs
test 2. Deprecated tools metadata URL used
[LOG] 47.94224 WARNING juju.environs.config Config attribute "tools-url" (tools-metadata-url-value) is deprecated.
The location to find tools is now specified using the "tools-metadata-url" attribute.
Your configuration should be updated to set "tools-metadata-url" as follows
tools-metadata-url: tools-metadata-url-value.
test 3. Deprecated tools metadata URL ignored
[LOG] 47.96187 WARNING juju.environs.config Config attribute "tools-url" (ignore-me) is deprecated and will be ignored since
the new tools URL attribute "tools-metadata-url" has also been used.
The attribute "tools-url" should be removed from your configuration.
test 4. Explicit series
test 5. Implicit series with empty value
test 6. Explicit logging
test 7. Explicit authorized-keys
test 8. Load authorized-keys from path
test 9. CA cert & key from path
test 10. CA cert & key from path; cert attribute set too
test 11. CA cert & key from ~ path
test 12. CA cert and key as attributes
test 13. Mismatched CA cert and key
test 14. Invalid CA cert
test 15. Invalid CA key
test 16. CA cert specified as non-existent file
test 17. CA key specified as non-existent file
test 18. Specified agent version
test 19. Specified development flag
test 20. Specified admin secret
test 21. Invalid development flag
config_test.go:600:
    test.check(c, h)
config_test.go:705:
    c.Check(cfg, gc.IsNil)
... value *config.Config = &config.Config{m:map[string]interface {}{"admin-secret":"", "logging-config":"<root>=DEBUG", "syslog-port":514, "tools-url":"...

[Ian Booth (wallyworld)]

https://codereview.appspot.com/33680043/diff/1/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode105
schema/schema.go:105: if v != nil {
On 2013/11/27 09:00:08, rog wrote:
> I think I'd be inclined to do something like this:

> func (c boolC) Coerce(v interface{}, path []string) (interface{},
error) {
> switch vv := reflect.TypeOf(v); vv.Kind() {
> case reflect.Bool:
> return vv.Bool(), nil
> case reflect.String:
> val, err := strconv.ParseBool(vv.String())
> if err == nil {
> return val, nil
> }
> }
> return nil, error{"bool", v, path}
> }

> the Sprintf seems unnecessary overkill.

Done.

https://codereview.appspot.com/33680043/diff/1/schema/schema.go#newcode123
schema/schema.go:123: if v != nil {
On 2013/11/27 09:00:08, rog wrote:
> similar here

Done.

https://codereview.appspot.com/33680043/

=== modified file 'cmd/juju/environment_test.go'
--- cmd/juju/environment_test.go	2013-09-12 12:38:04 +0000
+++ cmd/juju/environment_test.go	2013-11-28 01:04:32 +0000
@@ -129,6 +129,15 @@
 	c.Assert(stateConfig.DefaultSeries(), gc.Equals, "raring")
 }
 
+func (s *SetEnvironmentSuite) TestChangeBooleanAttribute(c *gc.C) {
+	_, err := testing.RunCommand(c, &SetEnvironmentCommand{}, []string{"ssl-hostname-verification=false"})
+	c.Assert(err, gc.IsNil)
+
+	stateConfig, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	c.Assert(stateConfig.SSLHostnameVerification(), gc.Equals, false)
+}
+
 func (s *SetEnvironmentSuite) TestChangeMultipleValues(c *gc.C) {
 	_, err := testing.RunCommand(c, &SetEnvironmentCommand{}, []string{"default-series=spartan", "broken=nope", "secret=sekrit"})
 	c.Assert(err, gc.IsNil)
@@ -156,13 +165,16 @@
 	"name":          "foo",
 	"type":          "foo",
 	"firewall-mode": "global",
+	"state-port":    "1",
+	"api-port":      "666",
+	"syslog-port":   "42",
 }
 
 func (s *SetEnvironmentSuite) TestImmutableConfigValues(c *gc.C) {
 	for name, value := range immutableConfigTests {
 		param := fmt.Sprintf("%s=%s", name, value)
 		_, err := testing.RunCommand(c, &SetEnvironmentCommand{}, []string{param})
-		errorPattern := fmt.Sprintf("cannot change %s from .* to %q", name, value)
+		errorPattern := fmt.Sprintf("cannot change %s from .* to [\"]?%v[\"]?", name, value)
 		c.Assert(err, gc.ErrorMatches, errorPattern)
 	}
 }

=== modified file 'environs/config/config_test.go'
--- environs/config/config_test.go	2013-11-27 15:26:18 +0000
+++ environs/config/config_test.go	2013-11-28 01:04:32 +0000
@@ -303,9 +303,9 @@
 			"type":            "my-type",
 			"name":            "my-name",
 			"authorized-keys": "my-keys",
-			"development":     "true",
+			"development":     "invalid",
 		},
-		err: `development: expected bool, got string\("true"\)`,
+		err: `development: expected bool, got string\("invalid"\)`,
 	}, {
 		about:       "Invalid agent version",
 		useDefaults: config.UseDefaults,

=== modified file 'schema/schema.go'
--- schema/schema.go	2013-09-26 23:26:19 +0000
+++ schema/schema.go	2013-11-28 01:04:32 +0000
@@ -102,8 +102,16 @@
 type boolC struct{}
 
 func (c boolC) Coerce(v interface{}, path []string) (interface{}, error) {
-	if v != nil && reflect.TypeOf(v).Kind() == reflect.Bool {
-		return v, nil
+	if v != nil {
+		switch reflect.TypeOf(v).Kind() {
+		case reflect.Bool:
+			return v, nil
+		case reflect.String:
+			val, err := strconv.ParseBool(reflect.ValueOf(v).String())
+			if err == nil {
+				return val, nil
+			}
+		}
 	}
 	return nil, error_{"bool", v, path}
 }
@@ -126,6 +134,13 @@
 	case reflect.Int16:
 	case reflect.Int32:
 	case reflect.Int64:
+	case reflect.String:
+		val, err := strconv.ParseInt(reflect.ValueOf(v).String(), 0, 64)
+		if err == nil {
+			return val, nil
+		} else {
+			return nil, error_{"int", v, path}
+		}
 	default:
 		return nil, error_{"int", v, path}
 	}
@@ -143,14 +158,23 @@
 type forceIntC struct{}
 
 func (c forceIntC) Coerce(v interface{}, path []string) (interface{}, error) {
-	if v == nil {
-		return nil, error_{"number", v, path}
-	}
-	switch reflect.TypeOf(v).Kind() {
-	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
-		return int(reflect.ValueOf(v).Int()), nil
-	case reflect.Float32, reflect.Float64:
-		return int(reflect.ValueOf(v).Float()), nil
+	if v != nil {
+		switch vv := reflect.TypeOf(v); vv.Kind() {
+		case reflect.String:
+			vstr := reflect.ValueOf(v).String()
+			intValue, err := strconv.ParseInt(vstr, 0, 64)
+			if err == nil {
+				return int(intValue), nil
+			}
+			floatValue, err := strconv.ParseFloat(vstr, 64)
+			if err == nil {
+				return int(floatValue), nil
+			}
+		case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+			return int(reflect.ValueOf(v).Int()), nil
+		case reflect.Float32, reflect.Float64:
+			return int(reflect.ValueOf(v).Float()), nil
+		}
 	}
 	return nil, error_{"number", v, path}
 }

=== modified file 'schema/schema_test.go'
--- schema/schema_test.go	2013-09-27 01:50:46 +0000
+++ schema/schema_test.go	2013-11-28 01:04:32 +0000
@@ -75,17 +75,21 @@
 func (s *S) TestBool(c *gc.C) {
 	sch := schema.Bool()
 
-	out, err := sch.Coerce(true, aPath)
-	c.Assert(err, gc.IsNil)
-	c.Assert(out, gc.Equals, true)
-
-	out, err = sch.Coerce(false, aPath)
-	c.Assert(err, gc.IsNil)
-	c.Assert(out, gc.Equals, false)
-
-	out, err = sch.Coerce(1, aPath)
+	for _, trueValue := range []interface{}{true, "1", "true", "True", "TRUE"} {
+		out, err := sch.Coerce(trueValue, aPath)
+		c.Assert(err, gc.IsNil)
+		c.Assert(out, gc.Equals, true)
+	}
+
+	for _, falseValue := range []interface{}{false, "0", "false", "False", "FALSE"} {
+		out, err := sch.Coerce(falseValue, aPath)
+		c.Assert(err, gc.IsNil)
+		c.Assert(out, gc.Equals, false)
+	}
+
+	out, err := sch.Coerce(42, aPath)
 	c.Assert(out, gc.IsNil)
-	c.Assert(err, gc.ErrorMatches, `<path>: expected bool, got int\(1\)`)
+	c.Assert(err, gc.ErrorMatches, `<path>: expected bool, got int\(42\)`)
 
 	out, err = sch.Coerce(nil, aPath)
 	c.Assert(out, gc.IsNil)
@@ -103,6 +107,10 @@
 	c.Assert(err, gc.IsNil)
 	c.Assert(out, gc.Equals, int64(42))
 
+	out, err = sch.Coerce("42", aPath)
+	c.Assert(err, gc.IsNil)
+	c.Assert(out, gc.Equals, int64(42))
+
 	out, err = sch.Coerce(true, aPath)
 	c.Assert(out, gc.IsNil)
 	c.Assert(err, gc.ErrorMatches, `<path>: expected int, got bool\(true\)`)
@@ -119,6 +127,14 @@
 	c.Assert(err, gc.IsNil)
 	c.Assert(out, gc.Equals, int(42))
 
+	out, err = sch.Coerce("42", aPath)
+	c.Assert(err, gc.IsNil)
+	c.Assert(out, gc.Equals, int(42))
+
+	out, err = sch.Coerce("42.66", aPath)
+	c.Assert(err, gc.IsNil)
+	c.Assert(out, gc.Equals, int(42))
+
 	out, err = sch.Coerce(int8(42), aPath)
 	c.Assert(err, gc.IsNil)
 	c.Assert(out, gc.Equals, int(42))