Allow calling `GoCodeSetResourceForStruct` for non-CRD SDK shapes #387

a-hilaly · 2023-01-04T20:15:16Z

Issue: aws-controllers-k8s/community#1609

Previously, the code-generator was unable to let us use GoCodeSetResourceForStruct for fields that are not part of a CRD top level fields. This caused issues when trying to generate custom code that transforms an eventbridge v1alpha1.Target to an svcsdk.Target.

This patch fixes the issue by allowing users to call GoCodeSetResourceForStruct for any field, not just CRD top level fields. In addition, unit tests have been added to ensure the correct behavior.

This patch is needed to allow us to properly execute the template written here https://github.com/embano1/eventbridge-controller/blob/aminemicha/templates/hooks/rule/sdk_file_end.go.tpl

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

RedbackThomson · 2023-01-04T20:39:03Z

I don't think I understand how this solves the problem? This seems to be throwing away error handling, but I don't see how it's able to access fields that aren't in the spec or status? It still uses f.Fields?

a-hilaly · 2023-01-04T20:57:24Z

I don't think I understand how this solves the problem? This seems to be throwing away error handling, but I don't see how it's able to access fields that aren't in the spec or status? It still uses f.Fields?

Because the statement bellow will not return a positive ok unless if one targetFieldName is one of the spec/status fields.

        f, ok := r.Fields[targetFieldName]
	if !ok {
		return ""
	}

Reading the code again i just realized that i need to add the setCfg.ignore part.. adding that in a different commit

pkg/generate/code/set_resource_test.go

jaypipes · 2023-01-05T16:59:27Z

pkg/generate/code/set_resource_test.go

+	assert.Equal(
+		expected,
+		code.SetResourceForStruct(
+			crd.Config(), crd, "", "krTarget", krTargetShape, nil, "sdkTarget", sdkTargetShape, "", model.OpTypeList, 1,


All of the other calls in this test file use "resp" and "ko" for the SDK and Kubernetes object variable names, respectively. Can we match that?

The thing is that Target is not a CRD nor a CRD Field - There is no need to use resp and ko as they really represent what the function is used for

jaypipes · 2023-01-05T17:01:28Z

pkg/generate/code/set_resource_test.go

+		krTarget.ARN = sdkTarget.Arn
+	}
+	if sdkTarget.BatchParameters != nil {
+		krTargetf1 := &svcapitypes.BatchParameters{}


Normally, this would just be:

f1 := &svcapitypes.BatchParameters{}

not

krTargetf1 := &svcapitypes.BatchParameters{}

I think you are incorrectly specifying the target variable name...

The thing is if i change krTarget to an empty string, L3298 will become .ARN = resp.Arn which is not correct

a-hilaly · 2023-01-11T11:29:24Z

/retest

embano1 · 2023-01-17T09:26:05Z

Pinging @a-hilaly for who can review and LGTM since this is blocking the ACK EventBridge controller 🙏🏻

RedbackThomson · 2023-01-18T20:04:49Z

Setting the test naming conventions aside, I am happy with this as a simple feature/fix.
/approve

jljaco · 2023-01-20T15:51:22Z

/approve

jaypipes · 2023-01-20T18:39:43Z

pkg/generate/code/set_resource_test.go

+		ko.DeadLetterConfig = kof2
+	}
+	if resp.EcsParameters != nil {
+		kof3 := &svcapitypes.EcsParameters{}


Hmm, this should be ECSParameters... we need to add ECS to the initialisms.

jaypipes · 2023-01-20T18:40:26Z

pkg/generate/code/set_resource_test.go

+			kof3.CapacityProviderStrategy = kof3f0
+		}
+		if resp.EcsParameters.EnableECSManagedTags != nil {
+			kof3.EnableECSManagedTags = resp.EcsParameters.EnableECSManagedTags


Interesting that they used "ECS" instead of "Ecs" in this field name...

jaypipes · 2023-01-20T18:52:06Z

pkg/generate/code/set_resource_test.go

+	assert.Equal(
+		expected,
+		code.SetResourceForStruct(
+			crd.Config(), crd, "", "ko", koShape, nil, "resp", targetShape, "", model.OpTypeList, 1,


Turns out that the targetField parameter isn't ever used by the SetResourceForStruct function :( Only the targetFieldPath parameter is ever used.

My request is that you:

remove the unused targetField parameter (since it is just confusing) from the SetResourceForStruct function

Pass fX for the targetVarName parameter (normally this would be something like "f12" if SetResourceForStruct is being called recursively from other functions in pkg/generate/code/set_resource.go or it would be the name of the field if SetResourceForStruct was called from a top-level template in sdk_file_end hooks...)

Previously, the code-generator was unable to use `GoCodeSetResourceForStruct` for fields that are not part of a CRD spec or status. This caused issues when trying to generate custom code that transforms an eventbridge `v1alpha1.Target` to an `svcsdk.Target`. This patch fixes the issue by allowing users to call `GoCodeSetResourceForStruct` for any field, not just CRD top level fields. In addition, unit tests have been added to ensure the correct behavior. Signed-off-by: Amine Hilaly <[email protected]>

jaypipes

Much better. Thank you sir! :)

a-hilaly · 2023-01-24T21:47:40Z

/retest

a-hilaly · 2023-01-24T21:51:59Z

/retest

a-hilaly · 2023-01-24T22:30:32Z

/retest

a-hilaly · 2023-01-24T22:47:42Z

/test all

a-hilaly · 2023-01-25T01:02:52Z

/retest

a-hilaly · 2023-01-25T01:55:31Z

/test all

a-hilaly · 2023-01-25T13:57:47Z

/test all

jljaco · 2023-01-25T19:32:17Z

/lgtm

ack-bot · 2023-01-25T19:32:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes, jljaco, 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,jljaco]

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

Add EventBridge API model definitions and docs

97912dd

ack-bot requested review from jljaco and vijtrip2 January 4, 2023 20:15

ack-bot added the approved label Jan 4, 2023

a-hilaly changed the title ~~Fix GoCodeSetResourceForStruct for non-CRD fields~~ Allow calling GoCodeSetResourceForStruct for non-CRD fields Jan 4, 2023

a-hilaly changed the title ~~Allow calling GoCodeSetResourceForStruct for non-CRD fields~~ Allow calling GoCodeSetResourceForStruct for non-CRD shapes Jan 4, 2023

a-hilaly changed the title ~~Allow calling GoCodeSetResourceForStruct for non-CRD shapes~~ Allow calling GoCodeSetResourceForStruct for non-CRD SDK shapes Jan 4, 2023

a-hilaly added the kind/enhancement Categorizes issue or PR as related to existing feature enhancements. label Jan 4, 2023

jaypipes reviewed Jan 5, 2023

View reviewed changes

a-hilaly force-pushed the gocode-setresource branch from 63caf60 to f09171b Compare January 5, 2023 20:57

jljaco requested review from RedbackThomson and jaypipes and removed request for jaypipes January 18, 2023 19:24

a-hilaly force-pushed the gocode-setresource branch from f09171b to 215b1b9 Compare January 20, 2023 15:52

jaypipes requested changes Jan 20, 2023

View reviewed changes

a-hilaly force-pushed the gocode-setresource branch from 215b1b9 to 1ab64e5 Compare January 23, 2023 17:33

jaypipes approved these changes Jan 23, 2023

View reviewed changes

a-hilaly mentioned this pull request Jan 23, 2023

E2E Tests Prow Jobs Failing aws-controllers-k8s/community#1640

Closed

ack-bot assigned jljaco Jan 25, 2023

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2023

ack-bot merged commit 5204077 into aws-controllers-k8s:main Jan 25, 2023

Allow calling GoCodeSetResourceForStruct for non-CRD SDK shapes #387

Allow calling GoCodeSetResourceForStruct for non-CRD SDK shapes #387

Uh oh!

Conversation

a-hilaly commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RedbackThomson commented Jan 4, 2023

Uh oh!

a-hilaly commented Jan 4, 2023

Uh oh!

Uh oh!

jaypipes Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

a-hilaly Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

jaypipes Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

a-hilaly Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Jan 11, 2023

Uh oh!

embano1 commented Jan 17, 2023

Uh oh!

RedbackThomson commented Jan 18, 2023

Uh oh!

jljaco commented Jan 20, 2023

Uh oh!

jaypipes Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

jaypipes Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

jaypipes Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Jan 24, 2023

Uh oh!

a-hilaly commented Jan 24, 2023

Uh oh!

a-hilaly commented Jan 24, 2023

Uh oh!

a-hilaly commented Jan 24, 2023

Uh oh!

a-hilaly commented Jan 25, 2023

Uh oh!

a-hilaly commented Jan 25, 2023

Uh oh!

a-hilaly commented Jan 25, 2023

Uh oh!

jljaco commented Jan 25, 2023

Uh oh!

ack-bot commented Jan 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Allow calling `GoCodeSetResourceForStruct` for non-CRD SDK shapes #387

Allow calling `GoCodeSetResourceForStruct` for non-CRD SDK shapes #387

a-hilaly commented Jan 4, 2023 •

edited

Loading