adds code generation for field comparisons #29

jaypipes · 2021-03-10T00:53:26Z

adds code generation for field comparisons

Adds new code generation functions to pkg/generate/code that output Go
code that compares fields of various types and calls delta.Add() if a
difference is detected.

For fields with underlying scalar types, the generated code looks like
this:

if *a.ko.Spec.Name != *b.ko.Spec.Name) {
        delta.Add("Spec.Name", a.ko.Spec.Name, b.ko.Spec.Name)
}

For non-scalar types, generated code recurses over the fields and/or
calls specialized comparison functions in the runtime library's
pkg/compare module. Here is an example of a complex underlying field
type and what the generated code might look like:

if ackcompare.HasNilDifference(a.ko.Spec.CreateBucketConfiguration, b.ko.Spec.CreateBucketConfiguration) {
        delta.Add("Spec.CreateBucketConfiguration", a.ko.Spec.CreateBucketConfiguration, b.ko.Spec.CreateBucketConfiguration)
} else if a.ko.Spec.CreateBucketConfiguration != nil && b.ko.Spec.CreateBucketConfiguration != nil {
        if ackcompare.HasNilDifference(a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint) {
                delta.Add("Spec.CreateBucketConfiguration.LocationConstraint", a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint)
        } else if a.ko.Spec.CreateBucketConfiguration.LocationConstraint != nil && b.ko.Spec.CreateBucketConfiguration.LocationConstraint {
                if *a.ko.Spec.CreateBucketConfiguration.LocationConstraint != *b.ko.Spec.CreateBucketConfiguration.LocationConstraint {
                        delta.Add("Spec.CreateBucketConfiguration.LocationConstraint", a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint)
                }
        }
}

Adds plumbing to the pkg/resource/depscriptor.go.tpl template that
implements the Delta() method on the
github.com/aws-controllers-k8s/runtime/pkg/types.AWSResourceDescriptor
interface-implementing resource struct.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

vijtrip2

LGTM!

a-hilaly

C'est genial @jaypipes! ça se rapproche!
I left few comments and questions

a-hilaly · 2021-03-10T20:03:29Z

templates/pkg/resource/descriptor.go.tpl

-	opts := []cmp.Option{cmpopts.EquateEmpty()}
-	{{- if .CRD.CompareIgnoredFields }}
-	opts = append(opts, cmpopts.IgnoreFields(*ac.ko,
-		{{- range $fieldPath := .CRD.CompareIgnoredFields }}
-		{{ printf "%q" $fieldPath }},
-		{{- end }}
-	))
-	{{- end }}
-	return cmp.Equal(ac.ko, bc.ko, opts...)


the modules github.com/google/go-cmp/cmp and github.com/google/go-cmp/cmp/cmpopts are no longer used we can remove them from the template (line 8,9)

👍 great catch

a-hilaly · 2021-03-10T21:03:13Z

templates/pkg/resource/delta.go.tpl

+package {{ .CRD.Names.Snake }}
+
+import (
+	ackcompare "github.com/aws/aws-controllers-k8s/pkg/compare"


s/github.com/aws/aws-controllers-k8s/ github.com/aws-controllers-k8s/runtime/

yikes, great catch.

a-hilaly · 2021-03-10T21:37:32Z

templates/pkg/resource/descriptor.go.tpl

+// Delta returns an `ackcompare.Delta` object containing the difference between
+// one `AWSResource` and another.
+func (d *resourceDescriptor) Delta(a, b acktypes.AWSResource) *ackcompare.Delta {
+    return newResourceDelta(a, b)


The generated code doesn't compile since a type is an interface. I'm not very sure, but i think we can use newResourceDelta(a.(*resource), b.(*resource)) (thinking about type assertion panics..)

a-hilaly · 2021-03-10T21:37:36Z

templates/pkg/resource/sdk_update.go.tpl

 	latest *resource,
-	diffReporter *ackcompare.Reporter,
+	delta *ackcompare.Delta,
 ) (*resource, error) {
 {{ $customMethod := .CRD.GetCustomImplementation .CRD.Ops.Update }}
 {{ if $customMethod }}
-	customResp, customRespErr := rm.{{ $customMethod }}(ctx, desired, latest, diffReporter)
+	customResp, customRespErr := rm.{{ $customMethod }}(ctx, desired, latest, delta)
 	if customResp != nil || customRespErr != nil {


We should do the same for the other update methods in manager.go.tpl, sdk_update_set_attributes.go.tpl, sdk_update_not_implemented.go.tpl and sdk_update_custom.go.tpl

a-hilaly · 2021-03-10T21:40:14Z

templates/pkg/resource/delta.go.tpl

+        delta.Add("", a, b)
+        return delta
+    }
+{{ GoCodeCompare .CRD "delta" "a.ko" "b.ko" 1}}


We need a "GoCodeCompare" implementation in pkg/generate/ack/controller.go e.g:

"GoCodeCompare": func(r *ackmodel.CRD, deltaVarName, firstResVarName string, secondResVarName string, indentLevel int) string { return code.CompareResource(r.Config(), r, deltaVarName, firstResVarName, secondResVarName, indentLevel) },

also in pkg/generate/ack/controller.go "delta.go.tpl" should be added the targets list

a-hilaly · 2021-03-10T22:26:24Z

pkg/generate/code/compare_test.go

+	if ackcompare.HasNilDifference(a.ko.Spec.ACL, b.ko.Spec.ACL) {
+		delta.Add("Spec.ACL", a.ko.Spec.ACL, b.ko.Spec.ACL)
+	} else {
+		if *a.ko.Spec.ACL != *b.ko.Spec.ACL {


While testing the s3 controller (generated with the new compare methods) i observed few runtime panics, and the reason is that we don't handle cases where both of a.ko.Spec.* and a.ko.Spec.* are nil pointers. Calling "valueof" of a nil pointer causes panic.

quick fix (Probably there is a better way..): a-hilaly@f787376 :)

Another excellent catch, @a-hilaly! I went with a route that generated:

} else if a.ko.Spec.Name != nil && b.ko.Spec.Name != nil {

instead of an additional indent-level if statement.

Great! 👍

a-hilaly · 2021-03-10T22:42:41Z

pkg/generate/config/field.go

+	IsIgnored bool `json:"is_ignored"`
+	// NilEqualsEmptyString indicates a nil string pointer and empty string
+	// should be considered equal for the purposes of comparison
+	NilEqualsEmptyString bool `json:"nil_equals_empty_string"`


I'm wondering if we should rename this field to NilEqualsZeroValue? i'm thinking about cases where we want to equate 0 and nil, 0.0 and nil, false and nil ... etc

awesome point.

Adds new code generation functions to `pkg/generate/code` that output Go code that compares fields of various types and calls `delta.Add()` if a difference is detected. For fields with underlying scalar types, the generated code looks like this: ```go if *a.ko.Spec.Name != *b.ko.Spec.Name) { delta.Add("Spec.Name", a.ko.Spec.Name, b.ko.Spec.Name) } ``` For non-scalar types, generated code recurses over the fields and/or calls specialized comparison functions in the runtime library's `pkg/compare` module. Here is an example of a complex underlying field type and what the generated code might look like: ```go if ackcompare.HasNilDifference(a.ko.Spec.CreateBucketConfiguration, b.ko.Spec.CreateBucketConfiguration) { delta.Add("Spec.CreateBucketConfiguration", a.ko.Spec.CreateBucketConfiguration, b.ko.Spec.CreateBucketConfiguration) } else { if ackcompare.HasNilDifference(a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint) { delta.Add("Spec.CreateBucketConfiguration.LocationConstraint", a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint) } else { if *a.ko.Spec.CreateBucketConfiguration.LocationConstraint != *b.ko.Spec.CreateBucketConfiguration.LocationConstraint { delta.Add("Spec.CreateBucketConfiguration.LocationConstraint", a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint) } } } ```

Adds plumbing to the `pkg/resource/depscriptor.go.tpl` template that implements the `Delta()` method on the `github.com/aws-controllers-k8s/runtime/pkg/types.AWSResourceDescriptor` interface-implementing `resource` struct. Also adds a test for the compare field ignore functionality.

jaypipes mentioned this pull request Mar 10, 2021

modify reconciler to use new Delta() compare aws-controllers-k8s/runtime#8

Merged

jaypipes requested review from RedbackThomson, a-hilaly, kumargauravsharma, surajkota and vijtrip2 March 10, 2021 00:54

jaypipes changed the title ~~Delta~~ adds code generation for field comparisons Mar 10, 2021

jaypipes requested a review from panazzo March 10, 2021 00:55

jaypipes force-pushed the delta branch from 373b85f to bfe3875 Compare March 10, 2021 01:00

vijtrip2 approved these changes Mar 10, 2021

View reviewed changes

a-hilaly reviewed Mar 10, 2021

View reviewed changes

jaypipes changed the title ~~adds code generation for field comparisons~~ DO NOT MERGE: adds code generation for field comparisons Mar 11, 2021

jaypipes force-pushed the delta branch from bfe3875 to cf30af3 Compare March 11, 2021 19:53

jaypipes changed the title ~~DO NOT MERGE: adds code generation for field comparisons~~ adds code generation for field comparisons Mar 11, 2021

jaypipes force-pushed the delta branch from cf30af3 to 4a42cdf Compare March 11, 2021 20:31

jaypipes force-pushed the delta branch from 4a42cdf to 9900866 Compare March 17, 2021 13:57

jaypipes merged commit bebbdf5 into aws-controllers-k8s:main Mar 17, 2021

a-hilaly mentioned this pull request Mar 22, 2021

Fix delta code generation for nested structs shapes #36

Merged

adds code generation for field comparisons #29

adds code generation for field comparisons #29

Uh oh!

Conversation

jaypipes commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vijtrip2 left a comment

Choose a reason for hiding this comment

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jaypipes commented Mar 10, 2021 •

edited

Loading

a-hilaly Mar 10, 2021 •

edited

Loading