Repository: knative/pkg
Pull request #1651 (2020-08-25 20:10:43)

Switch bool parsing to the real parser
We parse junk as false, but that's not really correct.
So be more precise and more aggressive in validation.

/assign mattmoor @yanweiguo @julz 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1651#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[configmap/OWNERS](https://github.com/knative/pkg/blob/master/configmap/OWNERS)~~ [vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
julz: I guess this is _technically_ a breaking change to the configmaps (since you could theoretically have a falsey value that was valid before but that strconv doesn't like). You'll get a nice error in that case though and it's a pretty degenerate case so 🤷 seems fine to me.

/lgtm
/hold so @mattmoor can look
vagababov: Yeah, I think it's more correct this way, since somebody might have had a breaking config and not even aware of it, just because _false_ was what made sense.
vagababov: 🕐 
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1651#pullrequestreview-475091073" title="Approved">mattmoor</a>*, *<a href="https://github.com/knative/pkg/pull/1651#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[configmap/OWNERS](https://github.com/knative/pkg/blob/master/configmap/OWNERS)~~ [mattmoor,vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/configmap/parse.go b/configmap/parse.go
index eba04a11c..d4fa5e875 100644
--- a/configmap/parse.go
+++ b/configmap/parse.go
@@ -45,7 +45,9 @@ func AsString(key string, target *string) ParseFunc {
 func AsBool(key string, target *bool) ParseFunc {
 	return func(data map[string]string) error {
 		if raw, ok := data[key]; ok {
-			*target = strings.EqualFold(raw, "true")
+			val, err := strconv.ParseBool(raw)
+			*target = val // If err != nil — this is always false.
+			return err
 		}
 		return nil
 	}


Test patch:
diff --git a/configmap/parse_test.go b/configmap/parse_test.go
index 8bf6cb98e..c7706c87e 100644
--- a/configmap/parse_test.go
+++ b/configmap/parse_test.go
@@ -27,15 +27,15 @@ import (
 )
 
 type testConfig struct {
-	str string
-	boo bool
-	i32 int32
-	i64 int64
-	u32 uint32
-	f64 float64
-	dur time.Duration
-	set sets.String
-	qua *resource.Quantity
+	str    string
+	toggle bool
+	i32    int32
+	i64    int64
+	u32    uint32
+	f64    float64
+	dur    time.Duration
+	set    sets.String
+	qua    *resource.Quantity
 
 	nsn  types.NamespacedName
 	onsn *types.NamespacedName
@@ -66,15 +66,15 @@ func TestParse(t *testing.T) {
 			"test-optional-namespaced-name": "some-other-namespace/some-other-name",
 		},
 		want: testConfig{
-			str: "foo.bar",
-			boo: true,
-			i32: 1,
-			i64: 2,
-			u32: 3,
-			f64: 1.0,
-			dur: time.Minute,
-			set: sets.NewString("a", "b", "c"),
-			qua: &fiveHundredM,
+			str:    "foo.bar",
+			toggle: true,
+			i32:    1,
+			i64:    2,
+			u32:    3,
+			f64:    1.0,
+			dur:    time.Minute,
+			set:    sets.NewString("a", "b", "c"),
+			qua:    &fiveHundredM,
 			nsn: types.NamespacedName{
 				Name:      "some-name",
 				Namespace: "some-namespace",
@@ -87,31 +87,29 @@ func TestParse(t *testing.T) {
 	}, {
 		name: "respect defaults",
 		conf: testConfig{
-			str: "foo.bar",
-			boo: true,
-			i32: 1,
-			i64: 2,
-			f64: 1.0,
-			dur: time.Minute,
-			qua: &fiveHundredM,
+			str:    "foo.bar",
+			toggle: true,
+			i32:    1,
+			i64:    2,
+			f64:    1.0,
+			dur:    time.Minute,
+			qua:    &fiveHundredM,
 		},
 		want: testConfig{
-			str: "foo.bar",
-			boo: true,
-			i32: 1,
-			i64: 2,
-			f64: 1.0,
-			dur: time.Minute,
-			qua: &fiveHundredM,
+			str:    "foo.bar",
+			toggle: true,
+			i32:    1,
+			i64:    2,
+			f64:    1.0,
+			dur:    time.Minute,
+			qua:    &fiveHundredM,
 		},
 	}, {
-		name: "bool defaults to false",
+		name: "junk bool fails",
 		data: map[string]string{
 			"test-bool": "foo",
 		},
-		want: testConfig{
-			boo: false,
-		},
+		expectErr: true,
 	}, {
 		name: "int32 error",
 		data: map[string]string{
@@ -166,7 +164,7 @@ func TestParse(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			if err := Parse(test.data,
 				AsString("test-string", &test.conf.str),
-				AsBool("test-bool", &test.conf.boo),
+				AsBool("test-bool", &test.conf.toggle),
 				AsInt32("test-int32", &test.conf.i32),
 				AsInt64("test-int64", &test.conf.i64),
 				AsUint32("test-uint32", &test.conf.u32),