From f416878d8362aa9b8af7fcdb5dce66bdf74992e3 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Tue, 10 Sep 2024 14:05:28 -0400 Subject: [PATCH 1/3] chore: avoid unnecessary json marshal Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- pkg/diff/diff.go | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 2278222c3..0a672d7f6 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -673,22 +673,6 @@ func ThreeWayDiff(orig, config, live *unstructured.Unstructured) (*DiffResult, e return buildDiffResult(predictedLiveBytes, liveBytes), nil } -// stripTypeInformation strips any type information (e.g. float64 vs. int) from the unstructured -// object by remarshalling the object. This is important for diffing since it will cause godiff -// to report a false difference. -func stripTypeInformation(un *unstructured.Unstructured) *unstructured.Unstructured { - unBytes, err := json.Marshal(un) - if err != nil { - panic(err) - } - var newUn unstructured.Unstructured - err = json.Unmarshal(unBytes, &newUn) - if err != nil { - panic(err) - } - return &newUn -} - // removeNamespaceAnnotation remove the namespace and an empty annotation map from the metadata. // The namespace field is present in live (namespaced) objects, but not necessarily present in // config or last-applied. This results in a diff which we don't care about. We delete the two so @@ -1087,11 +1071,20 @@ func toString(val interface{}) string { // Remarshalling also strips any type information (e.g. float64 vs. int) from the unstructured // object. This is important for diffing since it will cause godiff to report a false difference. func remarshal(obj *unstructured.Unstructured, o options) *unstructured.Unstructured { - obj = stripTypeInformation(obj) data, err := json.Marshal(obj) if err != nil { panic(err) } + + // Unmarshal again to strip type information (e.g. float64 vs. int) from the unstructured + // object. This is important for diffing since it will cause godiff to report a false difference. + var newUn unstructured.Unstructured + err = json.Unmarshal(data, &newUn) + if err != nil { + panic(err) + } + obj = &newUn + gvk := obj.GroupVersionKind() item, err := scheme.Scheme.New(obj.GroupVersionKind()) if err != nil { From e9af44735a99b74a3c6cea9947432e234b70c58d Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Tue, 17 Sep 2024 07:47:36 -0400 Subject: [PATCH 2/3] more tests Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- pkg/diff/diff_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/pkg/diff/diff_test.go b/pkg/diff/diff_test.go index 922db590e..9845c1e1e 100644 --- a/pkg/diff/diff_test.go +++ b/pkg/diff/diff_test.go @@ -1150,6 +1150,28 @@ spec: t.Log(requestsAfter) assert.Equal(t, float64(0.2), requestsBefore["cpu"]) assert.Equal(t, "200m", requestsAfter["cpu"]) + + t.Run("from float", func(t *testing.T) { + // use a plain float, not float64 + un.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{})["cpu"] = 0.2 + newUn := remarshal(&un, applyOptions(diffOptionsForTest())) + requestsAfter := newUn.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{}) + assert.Equal(t, "200m", requestsAfter["cpu"]) + }) + + t.Run("from string", func(t *testing.T) { + un.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{})["cpu"] = "200m" + newUn := remarshal(&un, applyOptions(diffOptionsForTest())) + requestsAfter := newUn.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{}) + assert.Equal(t, "200m", requestsAfter["cpu"]) + }) + + t.Run("from invalid", func(t *testing.T) { + un.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{})["cpu"] = "invalid" + newUn := remarshal(&un, applyOptions(diffOptionsForTest())) + requestsAfter := newUn.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{}) + assert.Equal(t, "invalid", requestsAfter["cpu"]) + }) } func ExampleDiff() { From 06e9545de306ea586e788526f4a8a7a1ac6ebdfb Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Tue, 17 Sep 2024 07:57:46 -0400 Subject: [PATCH 3/3] refactor test to satisfy sonarcloud Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- pkg/diff/diff_test.go | 55 +++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/pkg/diff/diff_test.go b/pkg/diff/diff_test.go index 9845c1e1e..fd2fd754e 100644 --- a/pkg/diff/diff_test.go +++ b/pkg/diff/diff_test.go @@ -1127,6 +1127,14 @@ metadata: } func TestRemarshalResources(t *testing.T) { + getRequests := func(un *unstructured.Unstructured) map[string]interface{} { + return un.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{}) + } + + setRequests := func(un *unstructured.Unstructured, requests map[string]interface{}) { + un.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"] = requests + } + manifest := []byte(` apiVersion: v1 kind: Pod @@ -1142,36 +1150,27 @@ spec: `) un := unstructured.Unstructured{} err := yaml.Unmarshal(manifest, &un) - assert.NoError(t, err) - requestsBefore := un.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{}) - t.Log(requestsBefore) - newUn := remarshal(&un, applyOptions(diffOptionsForTest())) - requestsAfter := newUn.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{}) - t.Log(requestsAfter) - assert.Equal(t, float64(0.2), requestsBefore["cpu"]) - assert.Equal(t, "200m", requestsAfter["cpu"]) - - t.Run("from float", func(t *testing.T) { - // use a plain float, not float64 - un.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{})["cpu"] = 0.2 - newUn := remarshal(&un, applyOptions(diffOptionsForTest())) - requestsAfter := newUn.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{}) - assert.Equal(t, "200m", requestsAfter["cpu"]) - }) + require.NoError(t, err) - t.Run("from string", func(t *testing.T) { - un.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{})["cpu"] = "200m" - newUn := remarshal(&un, applyOptions(diffOptionsForTest())) - requestsAfter := newUn.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{}) - assert.Equal(t, "200m", requestsAfter["cpu"]) - }) + testCases := []struct { + name string + cpu any + expectedCPU any + }{ + {"from float", 0.2, "200m"}, + {"from float64", float64(0.2), "200m"}, + {"from string", "0.2", "200m"}, + {"from invalid", "invalid", "invalid"}, + } - t.Run("from invalid", func(t *testing.T) { - un.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{})["cpu"] = "invalid" - newUn := remarshal(&un, applyOptions(diffOptionsForTest())) - requestsAfter := newUn.Object["spec"].(map[string]interface{})["containers"].([]interface{})[0].(map[string]interface{})["resources"].(map[string]interface{})["requests"].(map[string]interface{}) - assert.Equal(t, "invalid", requestsAfter["cpu"]) - }) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + setRequests(&un, map[string]interface{}{"cpu": tc.cpu}) + newUn := remarshal(&un, applyOptions(diffOptionsForTest())) + requestsAfter := getRequests(newUn) + assert.Equal(t, tc.expectedCPU, requestsAfter["cpu"]) + }) + } } func ExampleDiff() {