Skip to content

Use SetConfig in SetResourceForStruct when target shape has no members #292

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

Merged
merged 3 commits into from
Feb 17, 2022
Merged

Use SetConfig in SetResourceForStruct when target shape has no members #292

merged 3 commits into from
Feb 17, 2022

Conversation

brycahta
Copy link
Contributor

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.

Copy link
Collaborator

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Correct that one hard-coded OpTypeList thing and I'm good with this, thank you @brycahta!

Comment on lines +1345 to +1347
// Use setResourceForScalar and dereference sourceAdaptedVarName
// because primitives are being set.
sourceAdaptedVarName = "*" + sourceAdaptedVarName
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fine for now, but I recommend we clean up the setResourceForScalar() method implementation to be able to properly handle this only using the sourceShapeRef information.

@brycahta brycahta requested a review from jaypipes February 16, 2022 17:25
@brycahta
Copy link
Contributor Author

logs from failing s3-olm-test

==== building s3-controller release artifacts ====
Building release artifacts for s3-v0.0.13
Generating common custom resource definitions
Generating custom resource definitions for s3
Generating RBAC manifests for s3
Generating operator lifecycle manager bundle assets for s3
Error: canot clone repository: context deadline exceeded
make: *** [Makefile:36: build-controller] Error 1

@brycahta
Copy link
Contributor Author

/test s3-olm-test

Copy link
Collaborator

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

👍

fields:
DHCPConfigurations.Values:
set:
- from: AttributeValue.Value
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might consider adding:

      method: CREATE

here in order to exercise the OpType passing down into the recursive SetResourceForXXX functions.

@jaypipes
Copy link
Collaborator

/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2022
@ack-bot
Copy link
Collaborator

ack-bot commented Feb 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brycahta, jaypipes

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-bot ack-bot merged commit f8c0f47 into aws-controllers-k8s:main Feb 17, 2022
@brycahta brycahta deleted the setterConfig-fix branch February 17, 2022 15:56
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.

3 participants