Refactor reference resolution into Go code #401

RedbackThomson · 2023-02-04T01:31:46Z

Description of changes:
Refactors the resolveReferenceFor<Field> Go template code into direct Go code. This way we can be more intelligent about nested field references and support unit testing its output.

Breaking change:
The previous Go template produced valid, but ultimately faulty, code for references within lists of structs. These are not supported (see aws-controllers-k8s/community#1291). As such, this update produces a panic when it detects those types of references.

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

jljaco · 2023-02-06T22:33:18Z

/cc A-Hilaly

a-hilaly

Great one! Thank you for investing time in this. It's super useful, and makes references testable...
Left a few comments and suggestions bellow

a-hilaly · 2023-02-08T18:16:40Z

go.mod

@@ -16,7 +16,7 @@ require (
 	github.com/pkg/errors v0.9.1
 	github.com/spf13/cobra v1.2.1
 	github.com/stretchr/testify v1.7.1
-	golang.org/x/mod v0.4.2
+	golang.org/x/mod v0.6.0


Was this upgrade required for this patch? same for the upgrade below

I honestly can't tell you where this came from. Go modules are a mystery to me

pkg/generate/code/resource_reference.go

a-hilaly · 2023-02-08T18:33:23Z

pkg/generate/code/resource_reference_test.go

+		`	if ko.Spec.Logging != nil {
+		if ko.Spec.Logging.LoggingEnabled != nil {
+			refVal := ""
+			if ko.Spec.Logging.LoggingEnabled.TargetBucketRef != nil && ko.Spec.Logging.LoggingEnabled.TargetBucketRef.From != nil {


thoughts: The more i see those, the more i think that we can refactor a little bit our code generator to make nil checks generic. Thinking a recursive wrapper function that can be called everywhere

out += wrapNilCheck(fieldPath, generateCode(params....))

That would only really work for structs. As soon as we get into lists and maps then things get iffy. I initially started to do that but it didn't extend cleanly.

pkg/generate/code/resource_reference.go

pkg/generate/code/resource_reference_test.go

pkg/generate/code/resource_reference.go

jljaco · 2023-02-08T20:12:01Z

pkg/generate/code/resource_reference.go

+
+func resolveSingleReference(field *model.Field, sourceVarName string, targetVarName string, shouldAppendTarget bool, indentLevel int) string {
+	out := ""
+	indent := strings.Repeat("\t", indentLevel)


Can you just use https://pkg.go.dev/go/format#Source to deal with all of the formatting of the end result instead of bothering will all of these indentations?

++ Absolutely would love to do this programmatically. If we can do this as a full refactor for all methods in the code-generator.

pkg/generate/code/resource_reference_test.go

Co-authored-by: Amine <hilalyamine@gmail.com>

jaypipes

I think you left one hard-coded string value in the code-generating function :) See comment inline..

jaypipes · 2023-02-09T12:58:55Z

pkg/generate/code/resource_reference.go

+			outPrefix += fmt.Sprintf("%s\t\treturn fmt.Errorf(\"provided resource reference is nil or empty: \\%q\\\")\n", indent, field.ReferenceFieldPath())
+			outPrefix += fmt.Sprintf("%s\t}\n", indent)
+
+			outPrefix += fmt.Sprintf("%s\tif err := getReferencedResourceState_VPCLink(ctx, apiReader, obj, *arr.Name, namespace); err != nil {\n", indent)


Suggested change

outPrefix += fmt.Sprintf("%s\tif err := getReferencedResourceState_VPCLink(ctx, apiReader, obj, *arr.Name, namespace); err != nil {\n", indent)

outPrefix += fmt.Sprintf("%s\tif err := getReferencedResourceState_%s(ctx, apiReader, obj, *arr.Name, namespace); err != nil {\n", indent, r.Names.Camel)

jaypipes · 2023-02-09T12:59:57Z

pkg/generate/code/resource_reference_test.go

+		if arr == nil || arr.Name == nil || *arr.Name == "" {
+			return fmt.Errorf("provided resource reference is nil or empty: \"SecurityGroupRefs"\")
+		}
+		if err := getReferencedResourceState_VPCLink(ctx, apiReader, obj, *arr.Name, namespace); err != nil {


:) This test worked because it matched the hard-coded VPCLink you left in the generated code!

Don't you love test driven development

a-hilaly

Neat! Good stuff 👌

a-hilaly · 2023-02-09T18:19:07Z

pkg/generate/code/resource_reference.go

+			outPrefix += fmt.Sprintf("%s\t}\n", indent)
+			outPrefix += fmt.Sprintf("%s\t%s = append(%s, obj.%s)\n", indent, targetVarName, targetVarName, field.FieldConfig.References.Path)
+			outPrefix += fmt.Sprintf("%s}\n", indent)
+		case ("map"):


Didn't know parenthesis here are possible

ack-prow · 2023-02-09T18:21:49Z

@RedbackThomson: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ecr-controller-test	`dde4df3`	link	false	`/test ecr-controller-test`
dynamodb-controller-test	`dde4df3`	link	false	`/test dynamodb-controller-test`
s3-controller-test	`dde4df3`	link	false	`/test s3-controller-test`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

a-hilaly · 2023-02-09T18:22:27Z

/lgtm

ack-prow · 2023-02-09T18:22:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, RedbackThomson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [A-Hilaly,RedbackThomson]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…8s#401)" This reverts commit 8dc09b7.

Continuation of #401 Refactors the resolveReferenceFor<Field> Go template code into direct Go code. This way we can be more intelligent about nested field references and support unit testing its output. Breaking change: The previous Go template produced valid, but ultimately faulty, code for references within lists of structs. These are not supported (see aws-controllers-k8s/community#1291). As such, this update produces a panic when it detects those types of references. Co-authored-by: Amine <hilalyamine@gmail.com>

ack-prow bot requested review from jljaco and vijtrip2 February 4, 2023 01:31

ack-prow bot added the approved label Feb 4, 2023

jljaco requested a review from a-hilaly February 6, 2023 22:33

a-hilaly reviewed Feb 8, 2023

View reviewed changes

jaypipes reviewed Feb 8, 2023

View reviewed changes

pkg/generate/code/resource_reference.go Outdated Show resolved Hide resolved

pkg/generate/code/resource_reference.go Outdated Show resolved Hide resolved

jljaco reviewed Feb 8, 2023

View reviewed changes

Nicholas Thomson added 4 commits February 8, 2023 23:59

Naively refactored Go templates into Go code

60e6268

Refactored into iterative method

2a88d23

Fix nested struct detection

be6c652

Use template file for get reference value

798b8a5

RedbackThomson force-pushed the refactor-references branch from d84ea0b to 798b8a5 Compare February 8, 2023 23:59

RedbackThomson and others added 4 commits February 8, 2023 16:00

Update pkg/generate/code/resource_reference.go

52bdff9

Co-authored-by: Amine <hilalyamine@gmail.com>

Improve error condition messages

2936bef

Modify conditionals to use switch cases

f9c485b

Remove CRD as method input

c314ce1

RedbackThomson requested review from jljaco, jaypipes and a-hilaly and removed request for jaypipes and vijtrip2 February 9, 2023 00:16

jaypipes requested changes Feb 9, 2023

View reviewed changes

Fix hardcoded function name

dde4df3

a-hilaly approved these changes Feb 9, 2023

View reviewed changes

ack-prow bot assigned a-hilaly Feb 9, 2023

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2023

ack-prow bot merged commit 8dc09b7 into aws-controllers-k8s:main Feb 9, 2023

This was referenced Feb 9, 2023

Correct number of parameters to GoCodeResolveReference #412

Merged

Fixes for resource reference resolution templates #413

Closed

a-hilaly added a commit to a-hilaly/ack-code-generator that referenced this pull request Feb 9, 2023

Revert "Refactor reference resolution into Go code (aws-controllers-k…

b868a36

…8s#401)" This reverts commit 8dc09b7.

a-hilaly mentioned this pull request Feb 9, 2023

Revert resource reference resolution #414

Merged

RedbackThomson mentioned this pull request Feb 13, 2023

Refactor reference resolution into Go code #417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor reference resolution into Go code #401

Refactor reference resolution into Go code #401

RedbackThomson commented Feb 4, 2023

jljaco commented Feb 6, 2023

a-hilaly left a comment •

edited

Loading

a-hilaly Feb 8, 2023

RedbackThomson Feb 8, 2023

a-hilaly Feb 8, 2023

RedbackThomson Feb 9, 2023

jljaco Feb 8, 2023

RedbackThomson Feb 9, 2023

jaypipes left a comment

jaypipes Feb 9, 2023

jaypipes Feb 9, 2023

RedbackThomson Feb 9, 2023

RedbackThomson Feb 9, 2023

a-hilaly left a comment

a-hilaly Feb 9, 2023

ack-prow bot commented Feb 9, 2023

a-hilaly commented Feb 9, 2023

ack-prow bot commented Feb 9, 2023

	outPrefix += fmt.Sprintf("%s\tif err := getReferencedResourceState_VPCLink(ctx, apiReader, obj, *arr.Name, namespace); err != nil {\n", indent)
	outPrefix += fmt.Sprintf("%s\tif err := getReferencedResourceState_%s(ctx, apiReader, obj, *arr.Name, namespace); err != nil {\n", indent, r.Names.Camel)

Refactor reference resolution into Go code #401

Refactor reference resolution into Go code #401

Conversation

RedbackThomson commented Feb 4, 2023

jljaco commented Feb 6, 2023

a-hilaly left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaypipes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-hilaly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ack-prow bot commented Feb 9, 2023

a-hilaly commented Feb 9, 2023

ack-prow bot commented Feb 9, 2023

a-hilaly left a comment •

edited

Loading