Support cross-resource references in nested map types#681
Support cross-resource references in nested map types#681gustavodiaz7722 wants to merge 2 commits intoaws-controllers-k8s:mainfrom
Conversation
|
/retest |
1 similar comment
|
/retest |
knottnt
left a comment
knottnt
left a comment
There was a problem hiding this comment.
Thanks @gustavodiaz7722. Left a couple comments. Also have you been able to test the generated code locally with the quicksight-controller?
| hasReferences = true | ||
| arr := f0iter.CustomSQL.DataSourceRef.From | ||
| if arr.Name == nil || *arr.Name == "" { | ||
| return hasReferences, fmt.Errorf("provided resource reference is nil or empty: PhysicalTableMap.CustomSQL.DataSourceRef") |
There was a problem hiding this comment.
Q: should we include the map key in this error message? If there are multiple entries of the same object type it would help users identify the problematic part of their spec.
There was a problem hiding this comment.
I think for now we can leave as is, since it follows similar behavior for list type references. I agree though it's poor UX if we don't include key/element identifier.
For now I think it's okay to leave as is. I can follow up on this in another PR if that's okay
| require.NotNil(crd) | ||
|
|
||
| expected := | ||
| ` for f0idx, f0iter := range ko.Spec.PhysicalTableMap { |
There was a problem hiding this comment.
nit: It looks like we're re-using the list variables names here. Changing the name to be map specific could make the generated code more readable. Example names would be f0key and f0value.
There was a problem hiding this comment.
Yup, agreed. Will change this in new revision.
|
@knottnt Yes, I have tested these changes locally with new e2e test for verifying cross referencing is working correctly. I haven't updated the existing open PR with the new test yet, have them locally staged. |
|
@gustavodiaz7722: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gustavodiaz7722, michaelhtm The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Support cross-resource references in nested map types
Problem
The ACK code generator did not support cross-resource references for fields nested inside map types. When a
referencesconfig pointed to a field inside a map (e.g.,PhysicalTableMap.S3Source.DataSourceARNin QuickSight DataSet), the generator would produce an error during code generation. This blocked controllers like QuickSight from using K8s-native resource references for fields within map-typed structures.Solution
Extended two functions in
pkg/generate/code/resource_reference.go:iterReferenceValues()— Added a"map"case to the shape type switch. The new case mirrors the existing"list"case: it generates afor key, val := rangeloop over map entries, tracks iteration depth, and advances the field access prefix through the map value. Previously this function only handled"structure"and"list"shapes, returning an error for maps.buildIndexBasedFieldAccessorWithOffset()— Extended the list-shape condition to also match"map"shapes (cur.ShapeRef.Shape.Type == "list" || cur.ShapeRef.Shape.Type == "map"). This ensures index-based field accessors are correctly built when traversing map entries during reference resolution and clearing.Testing
generator-with-nested-references.yaml) that configuresPhysicalTableMap.CustomSql.DataSourceArnas a cross-resource reference to DataSource.Test_ResolveReferencesForField_SingleReference_WithinMap— verifies reference resolution code generation for map-nested fieldsTest_ClearResolvedReferencesForField_SingleReference_WithinMap— verifies reference clearing code generationTest_ReferenceFieldsValidation_MapNestedReference— verifies field validation accepts map-nested referencesAffected resources
Any ACK controller with references inside map-typed fields. Immediate use case: QuickSight DataSet's
PhysicalTableMapentries referencing DataSource ARNs.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.