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

Support lists of structs containing refs and ClearResolvedReferences #435

Merged
merged 14 commits into from
Apr 17, 2023

Conversation

RedbackThomson
Copy link
Contributor

@RedbackThomson RedbackThomson commented Apr 6, 2023

Implements aws-controllers-k8s/runtime#121

Description of changes:
This PR abstracts out the process of iterating through all references into a separate method called iterReferenceValues. iterReferenceValues produces Go code that drills down into the spec for every ref, iterating through any slices as necessary. Once it reaches the ref object, it calls a callback which can be used by other methods for logic pertaining to accessing the refs.

This change allows ResolveReferencesForField to access all refs within lists of structs (or even within lists of lists of structs, etc.). iterReferenceValues is also used to implement ClearResolvedReferencesForField, which simply sets the concrete value to nil if it detects a non-nil value in the ref field (or len > 0 for lists of refs).

This PR also removes hasNonNilReferences. hasNonNilReferences was being used to indicate whether there were any references inside the resource, which required another set of iterating through all ref fields. Instead, each resolveReferencesFor* returns a boolean which indicates whether it found a reference during its iteration.

Below is a Gist that shows the output of running the code-generator on the current EC2 generator.yaml:
https://gist.github.com/RedbackThomson/8e11cbbe96065a4eb812c387665c747d

For testing:
ec2 and lambda already have a series of tests for references. Both of these tests are passing, but they were also passing before this PR. I have a branch of the ec2-controller with additional tests using RouteTables, which have lists of structs containing refs, and all of those are passing as well.

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

@ack-prow ack-prow bot requested review from jaypipes and vijtrip2 April 6, 2023 23:30
@ack-prow ack-prow bot added the approved label Apr 6, 2023
@jaypipes jaypipes requested review from a-hilaly and jljaco and removed request for vijtrip2 April 7, 2023 13:55
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

Awesome, nice work on this mate 👍
Left few comments. I will get back with a more constructive review once i test it this patch with one of the controllers!

pkg/generate/code/resource_reference_test.go Outdated Show resolved Hide resolved
pkg/generate/code/resource_reference_test.go Show resolved Hide resolved
Comment on lines +39 to +44
` if ko.Spec.APIRef != nil && ko.Spec.APIID != nil {
return ackerr.ResourceReferenceAndIDNotSupportedFor("APIID", "APIRef")
}
if ko.Spec.APIRef == nil && ko.Spec.APIID == nil {
return ackerr.ResourceReferenceOrIDRequiredFor("APIID", "APIRef")
}
Copy link
Member

Choose a reason for hiding this comment

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

One of the good use case of https://github.com/aws-controllers-k8s/pkg/pull/14/files#diff-4b1976072e607d66abb6e181c6699c9be57d46ada282e77ee704c6cdbebb17e7R36-R48 i believe. I know it's a pain to check for the zero values as well. But users could provide apiID: "" ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lo also supports IsEmpty

@RedbackThomson
Copy link
Contributor Author

The caveat to these changes that is still not fixed:

These changes allow for resolving references within lists of structs, but reference fields are removed (replaced with their concrete values) from the spec of the resources at the end of the create resource reconciliation. This is because sdkFind inevitably calls Describe* and the response of this call is to place the values into spec. Any part of the describe response that contains a list, sdkFind creates an entirely new list from the output of the describe and replaces the field in the spec with this. For example, in ec2-controller RouteTable, Spec.Routes comes from the elem.Routes of the DescribeRouteTable call - https://github.com/aws-controllers-k8s/ec2-controller/blob/main/pkg/resource/route_table/sdk.go#L149. Any references that may have been in Spec.Routes are now overwritten and cannot contain the original references.

Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

👍

@RedbackThomson
Copy link
Contributor Author

/test s3-controller-test

@a-hilaly
Copy link
Member

/test apigatewayv2-controller-test

Copy link
Contributor

@jljaco jljaco left a comment

Choose a reason for hiding this comment

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

/lgtm

@ack-prow ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2023
@ack-prow
Copy link

ack-prow bot commented Apr 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List of structs containing references are patched without references
3 participants