Review feedback

Signed-off-by: Scott Andrews <[email protected]>

internal/resources/resource_with_unexported_fields.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/vmware-labs/reconciler-runtime/apis"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	runtime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -67,7 +68,7 @@ func (r *TestResourceUnexportedFields) validate() field.ErrorList {
 	return errs
 }
 
-func (r *TestResourceUnexportedFields) CopyUnexportedFields() {
+func (r *TestResourceUnexportedFields) ReflectUnexportedFieldsToStatus() {
 	r.Status.unexportedFields = r.Spec.unexportedFields
 }
 
@@ -89,7 +90,7 @@ func (r *TestResourceUnexportedFieldsSpec) SetUnexportedFields(f map[string]stri
 	r.unexportedFields = f
 }
 
-func (r *TestResourceUnexportedFieldsSpec) AddUnexportedFields(key, value string) {
+func (r *TestResourceUnexportedFieldsSpec) AddUnexportedField(key, value string) {
 	if r.unexportedFields == nil {
 		r.unexportedFields = map[string]string{}
 	}
@@ -149,7 +150,7 @@ func (r *TestResourceUnexportedFieldsStatus) SetUnexportedFields(f map[string]st
 	r.unexportedFields = f
 }
 
-func (r *TestResourceUnexportedFieldsStatus) AddUnexportedFields(key, value string) {
+func (r *TestResourceUnexportedFieldsStatus) AddUnexportedField(key, value string) {
 	if r.unexportedFields == nil {
 		r.unexportedFields = map[string]string{}
 	}
@@ -167,4 +168,19 @@ type TestResourceUnexportedFieldsList struct {
 
 func init() {
 	SchemeBuilder.Register(&TestResourceUnexportedFields{}, &TestResourceUnexportedFieldsList{})
+
+	if err := equality.Semantic.AddFuncs(
+		func(a, b TestResourceUnexportedFieldsSpec) bool {
+			return equality.Semantic.DeepEqual(a.Fields, b.Fields) &&
+				equality.Semantic.DeepEqual(a.Template, b.Template) &&
+				equality.Semantic.DeepEqual(a.ErrOnMarshal, b.ErrOnMarshal) &&
+				equality.Semantic.DeepEqual(a.ErrOnUnmarshal, b.ErrOnUnmarshal)
+		},
+		func(a, b TestResourceUnexportedFieldsStatus) bool {
+			return equality.Semantic.DeepEqual(a.Status, b.Status) &&
+				equality.Semantic.DeepEqual(a.Fields, b.Fields)
+		},
+	); err != nil {
+		panic(err)
+	}
 }

reconcilers/child_test.go

@@ -2299,12 +2299,6 @@ func TestChildReconciler_UnexportedFields(t *testing.T) {
 				rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Updated",
 					`Updated TestResourceUnexportedFields %q`, testName),
 			},
-			ExpectResource: resourceReady.
-				SpecDie(func(d *dies.TestResourceSpecDie) {
-					d.AddField("foo", "bar")
-					d.AddField("new", "field")
-				}).
-				DieReleasePtr(),
 			ExpectUpdates: []client.Object{
 				childGiven.
 					SpecDie(func(d *dies.TestResourceUnexportedFieldsSpecDie) {

reconcilers/cmp_test.go

@@ -0,0 +1,83 @@
+/*
+Copyright 2024 VMware, Inc.
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package reconcilers_test
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/vmware-labs/reconciler-runtime/internal/resources"
+	"github.com/vmware-labs/reconciler-runtime/internal/resources/dies"
+	"github.com/vmware-labs/reconciler-runtime/reconcilers"
+)
+
+type TestResourceUnexportedSpec struct {
+	spec resources.TestResourceUnexportedFieldsSpec
+}
+
+func TestIgnoreAllUnexported(t *testing.T) {
+	tests := map[string]struct {
+		a          interface{}
+		b          interface{}
+		shouldDiff bool
+	}{
+		"nil is equivalent": {
+			a:          nil,
+			b:          nil,
+			shouldDiff: false,
+		},
+		"different exported fields have a difference": {
+			a: dies.TestResourceUnexportedFieldsSpecBlank.
+				AddField("name", "hello").
+				DieRelease(),
+			b: dies.TestResourceUnexportedFieldsSpecBlank.
+				AddField("name", "world").
+				DieRelease(),
+			shouldDiff: true,
+		},
+		"different unexported fields do not have a difference": {
+			a: dies.TestResourceUnexportedFieldsSpecBlank.
+				AddUnexportedField("name", "hello").
+				DieRelease(),
+			b: dies.TestResourceUnexportedFieldsSpecBlank.
+				AddUnexportedField("name", "world").
+				DieRelease(),
+			shouldDiff: false,
+		},
+		"different exported fields nested in an unexported field do not have a difference": {
+			a: TestResourceUnexportedSpec{
+				spec: dies.TestResourceUnexportedFieldsSpecBlank.
+					AddField("name", "hello").
+					DieRelease(),
+			},
+			b: TestResourceUnexportedSpec{
+				spec: dies.TestResourceUnexportedFieldsSpecBlank.
+					AddField("name", "world").
+					DieRelease(),
+			},
+			shouldDiff: false,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			if name[0:1] == "#" {
+				t.SkipNow()
+			}
+
+			diff := cmp.Diff(tc.a, tc.b, reconcilers.IgnoreAllUnexported)
+			hasDiff := diff != ""
+			shouldDiff := tc.shouldDiff
+
+			if !hasDiff && shouldDiff {
+				t.Errorf("expected equality, found diff")
+			}
+			if hasDiff && !shouldDiff {
+				t.Errorf("found diff, expected equality: %s", diff)
+			}
+		})
+	}
+}

reconcilers/resource_test.go

@@ -1320,9 +1320,9 @@ func TestResourceReconciler_UnexportedFields(t *testing.T) {
 							if resource.Status.Fields == nil {
 								resource.Status.Fields = map[string]string{}
 							}
-							resource.CopyUnexportedFields()
+							resource.ReflectUnexportedFieldsToStatus()
 							resource.Status.Fields["Reconciler"] = "ran"
-							resource.Status.AddUnexportedFields("Reconciler", "ran")
+							resource.Status.AddUnexportedField("Reconciler", "ran")
 							return nil
 						},
 					}
@@ -1354,40 +1354,30 @@ func TestResourceReconciler_UnexportedFields(t *testing.T) {
 							if resource.Status.Fields == nil {
 								resource.Status.Fields = map[string]string{}
 							}
-							resource.CopyUnexportedFields()
-							resource.Status.AddUnexportedFields("Reconciler", "ran")
+							resource.ReflectUnexportedFieldsToStatus()
+							resource.Status.AddUnexportedField("Reconciler", "ran")
 							return nil
 						},
 					}
 				},
 			},
-			ExpectEvents: []rtesting.Event{
-				rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "StatusUpdated",
-					`Updated status`),
-			},
-			ExpectStatusUpdates: []client.Object{
-				resource.StatusDie(func(d *dies.TestResourceUnexportedFieldsStatusDie) {
-					d.AddUnexportedField("Reconciler", "ran")
-				}),
-			},
 		},
 		"no mutated status": {
 			Request: testRequest,
 			StatusSubResourceTypes: []client.Object{
 				&resources.TestResourceUnexportedFields{},
 			},
 			GivenObjects: []client.Object{
-				resource,
+				resource.StatusDie(func(d *dies.TestResourceUnexportedFieldsStatusDie) {
+					d.AddUnexportedField("Test", "ran")
+					d.AddUnexportedField("Reconciler", "ran")
+				}),
 			},
 			Metadata: map[string]interface{}{
 				"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler[*resources.TestResourceUnexportedFields] {
 					return &reconcilers.SyncReconciler[*resources.TestResourceUnexportedFields]{
 						Sync: func(ctx context.Context, resource *resources.TestResourceUnexportedFields) error {
-							if resource.Status.Fields == nil {
-								resource.Status.Fields = map[string]string{}
-							}
-							resource.CopyUnexportedFields()
-							resource.Spec.Fields["Reconciler"] = "ran"
+							resource.Status.AddUnexportedField("Reconciler", "ran")
 							return nil
 						},
 					}