Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make pointer to value if needed #200

Merged
merged 3 commits into from
Jan 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelog/v0.16.1/fix-equality.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
changelog:
- type: FIX
description: >
Fix the case where proto.Message types are passed in by value rather than by pointer.
issueLink: https://github.com/solo-io/skv2/issues/199
2 changes: 1 addition & 1 deletion contrib/codegen/templates/input/input_reconciler.gotmpl
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ type multiCluster{{ $snapshotName }}ReconcilerImpl struct {
base input.InputReconciler
}

// Options for reconcileing a snapshot
// Options for reconciling a snapshot
type {{ $snapshotName }}ReconcileOptions struct {
{{/* generate reconcile options here */}}
{{- range $group := $groups }}
Expand Down
24 changes: 22 additions & 2 deletions pkg/controllerutils/equality.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ func ObjectsEqual(obj1, obj2 runtime.Object) bool {
continue
}

field1 := value1.Field(i).Interface()
field2 := value2.Field(i).Interface()
field1 := mkPointer(value1.Field(i).Interface())
field2 := mkPointer(value2.Field(i).Interface())

// assert DeepEquality any other fields
if !equalityutils.DeepEqual(field1, field2) {
Expand All @@ -61,6 +61,26 @@ func ObjectsEqual(obj1, obj2 runtime.Object) bool {
return true
}

// if i is a pointer, just return the value.
// if i is addressable, return that.
// if i is a struct passed in by value, make a new instance of the type and copy the contents to that and return
// the pointer to that.
func mkPointer(i interface{}) interface{} {
val := reflect.ValueOf(i)
if val.Kind() == reflect.Ptr {
return i
}
if val.CanAddr() {
return val.Addr().Interface()
}
if val.Kind() == reflect.Struct {
nv := reflect.New(reflect.TypeOf(i))
nv.Elem().Set(val)
return nv.Interface()
}
return i
}

// returns true if "relevant" parts of obj1 and obj2 have equal:
// -labels
// -annotations
Expand Down
26 changes: 26 additions & 0 deletions pkg/controllerutils/equality_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,32 @@ var _ = Describe("ObjectsEqual", func() {
equal := ObjectsEqual(obj1, obj2)
Expect(equal).To(BeFalse())
})

It("asserts equality on two proto.Message objects even if they are passed in by value", func() {
obj1 := &things_test_io_v1.Paint{
Spec: things_test_io_v1.PaintSpec{
Color: &things_test_io_v1.PaintColor{
Hue: "red",
},
PaintType: &things_test_io_v1.PaintSpec_Oil{
Oil: nil,
},
},
}
obj1.Spec.ProtoReflect().SetUnknown([]byte(""))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, when the Spec was being passed in by value, then line

protoVal1, isProto := val1.(proto.Message)

would return isProto as false, and use reflect.DeepEqual instead of the correct proto.Equal, which allows us to treat empty .status.atomicInfo fields as the same as nil .status.atomicInfo fields.

obj2 := &things_test_io_v1.Paint{
Spec: things_test_io_v1.PaintSpec{
Color: &things_test_io_v1.PaintColor{
Hue: "red",
},
PaintType: &things_test_io_v1.PaintSpec_Oil{
Oil: nil,
},
},
}
equal := ObjectsEqual(obj1, obj2)
Expect(equal).To(BeTrue())
})
})

var _ = Describe("ObjectStatusesEqual", func() {
Expand Down