Skip to content

SetFieldConfig is not being consumed for nested Resource fields #1146

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

Closed
brycahta opened this issue Jan 26, 2022 · 1 comment
Closed

SetFieldConfig is not being consumed for nested Resource fields #1146

brycahta opened this issue Jan 26, 2022 · 1 comment
Assignees
Labels
area/code-generation Issues or PRs as related to controllers or docs code generation kind/bug Categorizes issue or PR as related to a bug.

Comments

@brycahta
Copy link
Contributor

Describe the bug

  • code-generator does not consume SetFieldConfig for nested Resource fields

Steps to reproduce

  • Add SetFieldConfig to ec2-controller/generator.yaml to properly generate DhcpOptions:
resources:
  DhcpOptions:
    fields:
      DHCPConfigurations.Values:
        set:
          - from: Values.Value
  • export SERVICE=ec2 && make build-controller

Actual outcome

  • excerpt from sdkFind/sdkCreate:
if f0iter.Values != nil {
				f0elemf1 := []*string{}
				for _, f0elemf1iter := range f0iter.Values {
					var f0elemf1elem string
					f0elemf1 = append(f0elemf1, &f0elemf1elem)
				}
				f0elem.Values = f0elemf1
			}
  • issues: f0elemf1iter is unused and f0elemf1elem is never assigned.

Expected outcome

  • code-generator should consume SetFieldConfig to generate DhcpOptions Controller code correctly:
if f0iter.Values != nil {
				f0elemf1 := []*string{}
				for _, f0elemf1iter := range f0iter.Values {
					var f0elemf1elem string
                                          if f0elemf1iter.Value != nil {
							f0elemf1elem = *f0elemf1iter.Value
					}
                                          f0elemf1 = append(f0elemf1, &f0elemf1elem)
				}
				f0elem.Values = f0elemf1
			}

Environment

  • Kubernetes version: N/A
  • Using EKS (yes/no), if so version? N/A
  • AWS service targeted (S3, RDS, etc.) EC2
@brycahta brycahta added the kind/bug Categorizes issue or PR as related to a bug. label Jan 26, 2022
@jaypipes jaypipes added the area/code-generation Issues or PRs as related to controllers or docs code generation label Jan 26, 2022
@brycahta
Copy link
Contributor Author

Some Root Causes

  1. f.SetterConfig is never called for nested fields; only fields that are members of outputShape
  2. In SetResourceForStruct, we iterate over sourceShape fields. So if we are trying to set a targetShape that's a primitive (i.e. String) from a sourceShape that's a struct (hence the call to SetResourceForStruct), the loop will continue and skip generating code for the primitive field
  3. We are not passing or using SetterConfig in SetResourceForStruct

ack-bot pushed a commit to aws-controllers-k8s/code-generator that referenced this issue Feb 11, 2022
…#289)

Issue #, if available: aws-controllers-k8s/community#1146

After a recent update to `/pkg/model/field.go`, shapes _may_ contain **additional** fields/members with a "nested" format such as **DHCPOptions.Fields**: `[DHCPConfigurations TagSpecifications.Tags.Value Tags.Value TagSpecifications DHCPConfigurations.Values TagSpecifications.ResourceType TagSpecifications.Tags Tags.Key DHCPOptionsID OwnerID DHCPConfigurations.Key TagSpecifications.Tags.Key DryRun Tags]` The purpose of that update is to allow users to easily fetch nested shapes and their corresponding configurations using the "nested" field name as the key.

However, `SetResourceForStruct` will never have the opportunity to process and/or fetch shapes and configs for these "nested" fields because the algorithm processes fields **iff they exist in sourceShape**. *Note, sourceShape comes directly from `aws-sdk` in this context, so will NOT have these additional, nested fields.*

This patch iterates over `targetShape.Members` instead of `sourceShape` so these "nested" fields can be included in the lookup logic, but continues to use `sourceShape` index for consistency throughout codebase. There's no change in functionality, but this paves the way for a follow-up PR to check a targetShape's nested Field `SetConfig` if the Field cannot be found in the sourceShape: 

```
sourceMemberShapeRef := sourceShape.MemberRefs[targetMemberName]
		if sourceMemberShapeRef == nil {
			continue
		}

                // Future patch: check if targetMemberName has SetConfig logic that needs to be processed
```

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
ack-bot pushed a commit to aws-controllers-k8s/code-generator that referenced this issue Feb 17, 2022
…mbers (#292)

Issue #, if available: aws-controllers-k8s/community#1146

This is a follow-up to #289. A `targetShape` without members in `SetResourceForStruct` will not be processed today; however, this is **not** desired behavior for all APIs such as EC2's DHCPOptions resource. 

This patch will look up and consume `SetConfig` in `SetResourceForStruct` when `targetShape` has no members like when DHCPOptions' field, `DHCPConfigurations.Values []*string`, aligns (by name) with aws-sdk's output shape, `DhcpConfigurations.Values []*AttributeValue`. Instead of skipping the processing of `Values []*string` field, it will check if a `SetConfig` exists and if it's valid (referencing a *real* shape in sourceShape), then use that updated field to source the target field's value resulting in using `DhcpConfigurations.Values.Value *string`  to populate the resource's `DHCPConfigurations.Values []*string`.






By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/code-generation Issues or PRs as related to controllers or docs code generation kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

3 participants