Add support for references within lists of structs #332
Conversation
There was a problem hiding this comment.
Very nice work, @RedbackThomson. I double-checked all the possibilities for variable shadowing and I think you've covered the potential cases appropriately. Would be good to have the docstrings updated for the generating functions, but otherwise, this is good to go.
| for serviceName := range referencedServiceNamesMap { | ||
| referencedServiceNames = append(referencedServiceNames, serviceName) | ||
| } | ||
| sort.Strings(referencedServiceNames) |
There was a problem hiding this comment.
I presume this is to ensure determinism?
There was a problem hiding this comment.
Yeah, I was seeing the imports and schemas jumping around in my git diff
| for serviceName, _ := range serviceNamesMap { | ||
| serviceNames = append(serviceNames, serviceName) | ||
| } | ||
| sort.Strings(serviceNames) |
| crd := testutil.GetCRDByName(t, g, "Api") | ||
| require.NotNil(crd) | ||
| expected := "false" | ||
| expected := "return false" |
There was a problem hiding this comment.
Initially I left out the "return" part to make these conditionals reusable in any "if" block... However now i can see that it's not possible to support all the types with that limitation.
No objections to the change but i thought of commenting the initial intention of not adding the "return" word.
There was a problem hiding this comment.
Ah I was wondering why that was left out. Hopefully if anything else needs that logic, it can just use the method directly, itself. Nice forward thinking, though
| } | ||
|
|
||
| if parentField.ShapeRef.Shape.Type == "list" { | ||
| iteratorsOut += fmt.Sprintf("for _, iter%d := range %s.%s {\n", fieldIndex, fieldAccessPrefix, parentField.Path) |
There was a problem hiding this comment.
This should be preceded by a nil check or it will throw nil pointer dereference error inside range r.ko.Spec.FieldA.FieldB.ListFieldC
There is a generator method already written to generate nil check for nested fields.
There was a problem hiding this comment.
Yep. Not necessary for this particularly example I'm testing against, since range ko.Spec.Routes will just iterate 0 times in Routes is nil. But easy enough to add
| } | ||
| {{ else }} | ||
| {{ $parentField := index .CRD.Fields $fp.String }} | ||
| {{ if eq $parentField.ShapeRef.Shape.Type "list" -}} |
There was a problem hiding this comment.
nit: I do not think you need this if eq $parentField.ShapeRef.Shape.Type "list" check(L#143) and it's corresponding else block(L#164).
Because after first if block ( {{ if not $isList -}} L#117)only lists will be passed down to else if and else. else if(L#127) checks for nested lists , and only non-nested lists will reach else block(L#141).
I don't think you need another shape check(L#143) inside else block.
There was a problem hiding this comment.
L#143 is not the same is $isList though. It checks that the parent of the field is a list. The first conditional is checking for single primitives, the second condition (the else if) is checking for lists of primitives. Within the third condition (the else) we are checking for lists of structs (the list is the parent field, not the field itself) and falling back to lists of primitives.
These are (some of) the conditions for resolving references - and which ones we support:
- Single primitives (Conditional 1)
- List of primitives (Conditional 2)
- Singly-nested primitives (Conditional 1)
- Singly-nested struct (Conditional 1)
- Singly-nested list of primitives (Conditional 2)
- Singly-nested list of structs (Conditional 3)
- Recursively-nested primitives (Conditional 1)
- Recursively-nested list of primitives
- Recursively-nested list of structs
Don't quote me on that. Perhaps some of these solutions can be generalised to match those, but it's also possible there are other cases I am not considering. Either way, I think this code will eventually need to be moved out of references.go.tpl and made with a string builder in Go. It's a bit too complex (especially if we use layers of iterators) for Go templates (and a PITA to debug).
Edit: I just realised I forgot to push a commit that contains changes to condition 1's logic. I think you are right given the code in this PR, but the logic is actually not quite as simple
There was a problem hiding this comment.
BTW I'd love to draw a Karnaugh map of the conditions and figure out a minimised conditional for this
brycahta
changed the title
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jaypipes, RedbackThomson, vijtrip2 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:
Approvers can indicate their approval by writing |
4 participants
Issue #, if available: aws-controllers-k8s/community#1256
Description of changes:
This pull request adds support for references that occur within lists of structs. It updates the generated
references.gofiles in each controller to now iterate through the structs to check for the existence of references and again for resolving their individual references. Rather than creating a new slice of structs with resolved references, it instead resolves the references in place - updating the base field value and leaving the reference untouched.This pull request has been tested against the APIGatewayv2 and EC2 resource reference tests, as well as manually against the EC2 changes that inspired the original issue.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.