Repository: kubermatic/machine-controller
Pull request #1165 (2022-01-13 16:57:24)

Omit null value from marshalled JSON for ConfigVarBool
**What this PR does / why we need it**:
Small refinement for the initial PR #1154 that changed `ConfigVarBool`. This slims down the JSON generated by `MarshalJSON` if the value is unset.

Also adds more tests to make sure my changes are capable of handling various input.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Optional Release Note**:
<!--
Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
NONE
```

Signed-off-by: Marvin Beckers <marvin@kubermatic.com>


kubermatic-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubermatic/machine-controller/pull/1165#" title="Author self-approved">embik</a>*

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

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:

- ~~[OWNERS](https://github.com/kubermatic/machine-controller/blob/master/OWNERS)~~ [embik]

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":[]} -->
kubermatic-bot: LGTM label has been added.  <details>Git tree hash: 81c3bedf764b25272a345a50e56a55163513e900</details>
kubermatic-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubermatic/machine-controller/pull/1165#" title="Author self-approved">embik</a>*, *<a href="https://github.com/kubermatic/machine-controller/pull/1165#pullrequestreview-852009879" title="LGTM">moadqassem</a>*

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

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:

- ~~[OWNERS](https://github.com/kubermatic/machine-controller/blob/master/OWNERS)~~ [embik,moadqassem]

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":[]} -->
kubermatic-bot: @embik: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-machine-controller-e2e-azure | 72c0fb501552c8ed66cd128a2d5611418735d30b | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/1165/pull-machine-controller-e2e-azure/1481671750998560768) | true | `/test pull-machine-controller-e2e-azure`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=1165)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
embik: /retest

Patch:
diff --git a/pkg/providerconfig/types/types.go b/pkg/providerconfig/types/types.go
index 186dc826..6bf7063c 100644
--- a/pkg/providerconfig/types/types.go
+++ b/pkg/providerconfig/types/types.go
@@ -250,7 +250,7 @@ func (configVarBool ConfigVarBool) MarshalJSON() ([]byte, error) {
 		if err != nil {
 			return []byte{}, err
 		}
-		return []byte(fmt.Sprintf("%v", string(jsonVal))), nil
+		return jsonVal, nil
 	}
 
 	buffer := bytes.NewBufferString("{")
@@ -274,12 +274,16 @@ func (configVarBool ConfigVarBool) MarshalJSON() ([]byte, error) {
 		buffer.WriteString(fmt.Sprintf(`%s"configMapKeyRef":%s`, leadingComma, jsonVal))
 	}
 
-	jsonVal, err := json.Marshal(configVarBool.Value)
-	if err != nil {
-		return []byte{}, err
+	if configVarBool.Value != nil {
+		jsonVal, err := json.Marshal(configVarBool.Value)
+		if err != nil {
+			return []byte{}, err
+		}
+
+		buffer.WriteString(fmt.Sprintf(`,"value":%v`, string(jsonVal)))
 	}
 
-	buffer.WriteString(fmt.Sprintf(`,"value":%v}`, string(jsonVal)))
+	buffer.WriteString("}")
 
 	return buffer.Bytes(), nil
 }


Test patch:
diff --git a/pkg/providerconfig/types/types_test.go b/pkg/providerconfig/types/types_test.go
index 2e0ac897..1ee10796 100644
--- a/pkg/providerconfig/types/types_test.go
+++ b/pkg/providerconfig/types/types_test.go
@@ -46,21 +46,50 @@ func TestConfigVarStringUnmarshalling(t *testing.T) {
 }
 
 func TestConfigVarBoolUnmarshalling(t *testing.T) {
-	jsonBool := []byte("true")
-	jsonMapBool := []byte(`{"value":true}`)
-
-	expectedResult := ConfigVarBool{Value: pointer.Bool(true)}
-
-	var jsonBoolTarget ConfigVarBool
-	var jsonMapBoolTarget ConfigVarBool
-
-	err := json.Unmarshal(jsonBool, &jsonBoolTarget)
-	if err != nil || !reflect.DeepEqual(expectedResult, jsonBoolTarget) {
-		t.Fatalf("Decoding raw bool into configVarBool failed! Error: '%v'", err)
+	testCases := []struct {
+		jsonString string
+		expected   ConfigVarBool
+	}{
+		{
+			jsonString: "true",
+			expected:   ConfigVarBool{Value: pointer.Bool(true)},
+		},
+		{
+			jsonString: `{"value":true}`,
+			expected:   ConfigVarBool{Value: pointer.Bool(true)},
+		},
+		{
+			jsonString: "null",
+			expected:   ConfigVarBool{},
+		},
+		{
+			jsonString: `{"value":null}`,
+			expected:   ConfigVarBool{},
+		},
+		{
+			jsonString: `{"secretKeyRef":{"namespace":"ns","name":"name","key":"key"}}`,
+			expected:   ConfigVarBool{Value: nil, SecretKeyRef: GlobalSecretKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"}},
+		},
+		{
+			jsonString: `{"value": null, "secretKeyRef":{"namespace":"ns","name":"name","key":"key"}}`,
+			expected:   ConfigVarBool{Value: nil, SecretKeyRef: GlobalSecretKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"}},
+		},
+		{
+			jsonString: `{"value":false, "secretKeyRef":{"namespace":"ns","name":"name","key":"key"}}`,
+			expected:   ConfigVarBool{Value: pointer.Bool(false), SecretKeyRef: GlobalSecretKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"}},
+		},
+		{
+			jsonString: `{"value":true, "secretKeyRef":{"namespace":"ns","name":"name","key":"key"}}`,
+			expected:   ConfigVarBool{Value: pointer.Bool(true), SecretKeyRef: GlobalSecretKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"}},
+		},
 	}
-	err = json.Unmarshal(jsonMapBool, &jsonMapBoolTarget)
-	if err != nil || !reflect.DeepEqual(expectedResult, jsonMapBoolTarget) {
-		t.Fatalf("Decoding map bool into configVarBool failed! Error: '%v'", err)
+
+	for _, testCase := range testCases {
+		var cvb ConfigVarBool
+		err := json.Unmarshal([]byte(testCase.jsonString), &cvb)
+		if err != nil || !reflect.DeepEqual(testCase.expected, cvb) {
+			t.Fatalf("Decoding '%s' into configVarBool failed! Error: '%v'", testCase.jsonString, err)
+		}
 	}
 }
 
@@ -97,13 +126,25 @@ func TestConfigVarBoolMarshalling(t *testing.T) {
 		cvb      ConfigVarBool
 		expected string
 	}{
+		{
+			cvb:      ConfigVarBool{},
+			expected: `null`,
+		},
 		{
 			cvb:      ConfigVarBool{Value: pointer.Bool(true)},
 			expected: `true`,
 		},
 		{
 			cvb:      ConfigVarBool{SecretKeyRef: GlobalSecretKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"}},
-			expected: `{"secretKeyRef":{"namespace":"ns","name":"name","key":"key"},"value":null}`,
+			expected: `{"secretKeyRef":{"namespace":"ns","name":"name","key":"key"}}`,
+		},
+		{
+			cvb:      ConfigVarBool{SecretKeyRef: GlobalSecretKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"}, Value: pointer.Bool(true)},
+			expected: `{"secretKeyRef":{"namespace":"ns","name":"name","key":"key"},"value":true}`,
+		},
+		{
+			cvb:      ConfigVarBool{SecretKeyRef: GlobalSecretKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"}, Value: pointer.Bool(false)},
+			expected: `{"secretKeyRef":{"namespace":"ns","name":"name","key":"key"},"value":false}`,
 		},
 	}
 
@@ -156,6 +197,8 @@ func TestConfigVarStringMarshallingAndUnmarshalling(t *testing.T) {
 func TestConfigVarBoolMarshallingAndUnmarshalling(t *testing.T) {
 
 	testCases := []ConfigVarBool{
+		{},
+		{Value: pointer.Bool(false)},
 		{Value: pointer.Bool(true)},
 		{SecretKeyRef: GlobalSecretKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"}},
 		{Value: pointer.Bool(true), SecretKeyRef: GlobalSecretKeySelector{ObjectReference: v1.ObjectReference{Namespace: "ns", Name: "name"}, Key: "key"}},