Skip to content

Commit db2aa58

Browse files
jaypipesjaypipes
authored
use reflect to compare map/slice struct (#157)
Although it's not what I wanted and isn't the most efficient thing we could do, I'm implementing the slice of struct and map of struct comparisons using `reflect.DeepEqual`. Long-term, I'd like to enable our generator-config CompareConfig for nested struct fields, and to do that we cannot use `reflect.DeepEqual`, however for the time-being, since slice and map of struct is a hole in our delta logic, I'm pushing up this workaround. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent eac8d8a commit db2aa58

File tree

3 files changed

+76
-23
lines changed

3 files changed

+76
-23
lines changed

‎pkg/generate/code/compare.go‎

Lines changed: 57 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,44 @@ import (
3636
// constructed from the Read operation, is compared to the Resource we got from
3737
// the Kubernetes API server's event bus. The code that is returned from this
3838
// function is the code that compares those two Resources.
39+
//
40+
// The Go code we return depends on the Go type of the various fields for the
41+
// resource being compared.
42+
//
43+
// For *scalar* Go types, the output Go code looks like this:
44+
//
45+
// if ackcompare.HasNilDifference(a.ko.Spec.GrantFullControl, b.ko.Spec.GrantFullControl) {
46+
// delta.Add("Spec.GrantFullControl", a.ko.Spec.GrantFullControl, b.ko.Spec.GrantFullControl)
47+
// } else if a.ko.Spec.GrantFullControl != nil && b.ko.Spec.GrantFullControl != nil {
48+
// if *a.ko.Spec.GrantFullControl != *b.ko.Spec.GrantFullControl {
49+
// delta.Add("Spec.GrantFullControl", a.ko.Spec.GrantFullControl, b.ko.Spec.GrantFullControl)
50+
// }
51+
// }
52+
//
53+
// For *struct* Go types, the output Go code looks like this (note that it is a
54+
// simple recursive-descent output of all the struct's fields...):
55+
//
56+
// if ackcompare.HasNilDifference(a.ko.Spec.CreateBucketConfiguration, b.ko.Spec.CreateBucketConfiguration) {
57+
// delta.Add("Spec.CreateBucketConfiguration", a.ko.Spec.CreateBucketConfiguration, b.ko.Spec.CreateBucketConfiguration)
58+
// } else if a.ko.Spec.CreateBucketConfiguration != nil && b.ko.Spec.CreateBucketConfiguration != nil {
59+
// if ackcompare.HasNilDifference(a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint) {
60+
// delta.Add("Spec.CreateBucketConfiguration.LocationConstraint", a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint)
61+
// } else if a.ko.Spec.CreateBucketConfiguration.LocationConstraint != nil && b.ko.Spec.CreateBucketConfiguration.LocationConstraint != nil {
62+
// if *a.ko.Spec.CreateBucketConfiguration.LocationConstraint != *b.ko.Spec.CreateBucketConfiguration.LocationConstraint {
63+
// delta.Add("Spec.CreateBucketConfiguration.LocationConstraint", a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint)
64+
// }
65+
// }
66+
// }
67+
//
68+
// For *slice of strings* Go types, the output Go code looks like this:
69+
//
70+
// if ackcompare.HasNilDifference(a.ko.Spec.AllowedPublishers, b.ko.Spec.AllowedPublishers) {
71+
// delta.Add("Spec.AllowedPublishers", a.ko.Spec.AllowedPublishers, b.ko.Spec.AllowedPublishers)
72+
// } else if a.ko.Spec.AllowedPublishers != nil && b.ko.Spec.AllowedPublishers != nil {
73+
// if !ackcompare.SliceStringPEqual(a.ko.Spec.AllowedPublishers.SigningProfileVersionARNs, b.ko.Spec.AllowedPublishers.SigningProfileVersionARNs) {
74+
// delta.Add("Spec.AllowedPublishers.SigningProfileVersionARNs", a.ko.Spec.AllowedPublishers.SigningProfileVersionARNs, b.ko.Spec.AllowedPublishers.SigningProfileVersionARNs)
75+
// }
76+
// }
3977
func CompareResource(
4078
cfg *ackgenconfig.Config,
4179
r *model.CRD,
@@ -113,8 +151,6 @@ func CompareResource(
113151
nilCode, firstResAdaptedVarName, secondResAdaptedVarName,
114152
)
115153
indentLevel++
116-
} else {
117-
out += "\n"
118154
}
119155

120156
switch memberShape.Type {
@@ -363,10 +399,15 @@ func compareMap(
363399
indent, firstResVarName, secondResVarName,
364400
)
365401
case "structure":
366-
// TODO(jaypipes): Implement this by walking the keys and struct values
367-
// and comparing each struct individually, building up the fieldPath
368-
// appropriately...
369-
return ""
402+
// NOTE(jaypipes): Using reflect here is really punting. We should
403+
// implement this in a cleaner, more efficient fashion by walking the
404+
// keys and struct values and comparing each struct individually,
405+
// building up the fieldPath appropriately and calling into a
406+
// struct-specific comparator function...
407+
out += fmt.Sprintf(
408+
"%sif !reflect.DeepEqual(%s, %s) {\n",
409+
indent, firstResVarName, secondResVarName,
410+
)
370411
default:
371412
panic("Unsupported shape type in generate.code.compareMap: " + shape.Type)
372413
}
@@ -431,10 +472,16 @@ func compareSlice(
431472
indent, firstResVarName, secondResVarName,
432473
)
433474
case "structure":
434-
// TODO(jaypipes): Implement this by walking the slice of struct values
435-
// and comparing each struct individually, building up the fieldPath
436-
// appropriately...
437-
return ""
475+
// NOTE(jaypipes): Using reflect here is really punting. We should
476+
// implement this in a cleaner, more efficient fashion by walking the
477+
// struct values and comparing each struct individually, building up
478+
// the fieldPath appropriately and calling into a struct-specific
479+
// comparator function...the tricky part of this is figuring out how to
480+
// sort the slice of structs...
481+
out += fmt.Sprintf(
482+
"%sif !reflect.DeepEqual(%s, %s) {\n",
483+
indent, firstResVarName, secondResVarName,
484+
)
438485
default:
439486
panic("Unsupported shape type in generate.code.compareSlice: " + shape.Type)
440487
}
@@ -531,8 +578,6 @@ func compareStruct(
531578
nilCode, firstResAdaptedVarName, secondResAdaptedVarName,
532579
)
533580
indentLevel++
534-
} else {
535-
out += "\n"
536581
}
537582
switch memberShape.Type {
538583
case "structure":

‎pkg/generate/code/compare_test.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,9 @@ func TestCompareResource_S3_Bucket(t *testing.T) {
9494
delta.Add("Spec.Logging.LoggingEnabled.TargetBucket", a.ko.Spec.Logging.LoggingEnabled.TargetBucket, b.ko.Spec.Logging.LoggingEnabled.TargetBucket)
9595
}
9696
}
97-
97+
if !reflect.DeepEqual(a.ko.Spec.Logging.LoggingEnabled.TargetGrants, b.ko.Spec.Logging.LoggingEnabled.TargetGrants) {
98+
delta.Add("Spec.Logging.LoggingEnabled.TargetGrants", a.ko.Spec.Logging.LoggingEnabled.TargetGrants, b.ko.Spec.Logging.LoggingEnabled.TargetGrants)
99+
}
98100
if ackcompare.HasNilDifference(a.ko.Spec.Logging.LoggingEnabled.TargetPrefix, b.ko.Spec.Logging.LoggingEnabled.TargetPrefix) {
99101
delta.Add("Spec.Logging.LoggingEnabled.TargetPrefix", a.ko.Spec.Logging.LoggingEnabled.TargetPrefix, b.ko.Spec.Logging.LoggingEnabled.TargetPrefix)
100102
} else if a.ko.Spec.Logging.LoggingEnabled.TargetPrefix != nil && b.ko.Spec.Logging.LoggingEnabled.TargetPrefix != nil {
@@ -140,7 +142,6 @@ func TestCompareResource_Lambda_CodeSigningConfig(t *testing.T) {
140142
if ackcompare.HasNilDifference(a.ko.Spec.AllowedPublishers, b.ko.Spec.AllowedPublishers) {
141143
delta.Add("Spec.AllowedPublishers", a.ko.Spec.AllowedPublishers, b.ko.Spec.AllowedPublishers)
142144
} else if a.ko.Spec.AllowedPublishers != nil && b.ko.Spec.AllowedPublishers != nil {
143-
144145
if !ackcompare.SliceStringPEqual(a.ko.Spec.AllowedPublishers.SigningProfileVersionARNs, b.ko.Spec.AllowedPublishers.SigningProfileVersionARNs) {
145146
delta.Add("Spec.AllowedPublishers.SigningProfileVersionARNs", a.ko.Spec.AllowedPublishers.SigningProfileVersionARNs, b.ko.Spec.AllowedPublishers.SigningProfileVersionARNs)
146147
}

‎templates/pkg/resource/delta.go.tpl‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,28 @@
33
package {{ .CRD.Names.Snake }}
44

55
import (
6+
"reflect"
7+
68
ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
79
)
810

11+
// Hack to avoid import errors during build...
12+
var (
13+
_ = &reflect.Method{}
14+
)
15+
916
// newResourceDelta returns a new `ackcompare.Delta` used to compare two
1017
// resources
1118
func newResourceDelta(
12-
a *resource,
13-
b *resource,
19+
a *resource,
20+
b *resource,
1421
) *ackcompare.Delta {
15-
delta := ackcompare.NewDelta()
16-
if ((a == nil && b != nil) ||
17-
(a != nil && b == nil)) {
18-
delta.Add("", a, b)
19-
return delta
20-
}
22+
delta := ackcompare.NewDelta()
23+
if ((a == nil && b != nil) ||
24+
(a != nil && b == nil)) {
25+
delta.Add("", a, b)
26+
return delta
27+
}
2128

2229
{{- if $hookCode := Hook .CRD "delta_pre_compare" }}
2330
{{ $hookCode }}
@@ -26,5 +33,5 @@ func newResourceDelta(
2633
{{- if $hookCode := Hook .CRD "delta_post_compare" }}
2734
{{ $hookCode }}
2835
{{- end }}
29-
return delta
36+
return delta
3037
}

0 commit comments

Comments
 (0)