Reference output_wrapper_field_path when generating CRD fields as part of the Create* request in model.go

[brycahta]

Issue #, if available:

• model.go does not take into account any output_wrapper_field_path overrides when processing Status fields for a CRD

Description of changes:

• Refactored GetOutputShape api:
  • always return an output shape
  • make getWrapperOutputShape private
  • updated tests
• Added GetOutputShape invocation to model.go so that overrides are checked before creating the CRD

Local Testing:

• generator.yaml

operations:
  CreateVpcEndpoint:
    output_wrapper_field_path: VpcEndpoint

• make build-controller SERVICE=ec2
• Result: VPCEndpointStatus contains all unpacked fields. Assigning from aws sdk response (vpc_endpoint/sdk.go) includes resp.VpcEndpoint as prefix

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

[RedbackThomson]

model.go does not take into account any output_wrapper_field_path overrides when processing Status fields for a CRD

I think what you're really referring to here is that model.go doesn't reference output_wrapper_field_path when generating CRD fields as part of the Create* request. I believe the code does use it in set_resource.sdk for Describe*.

Were you able to find a way for the adaptedSourceVarName to use the correct path?

Also it would be great to have a unit test for using GetOutputShape with Create*. That is, generating a list of Spec and Status fields and making sure it unwraps correctly (such as in your case with VpcEndpoint).

[RedbackThomson]

You'll probably want to handle this error, as it's critical. A panic might be a fair response if this returns an error - you cannot find the shape to use.

[brycahta]

GetOutputShape has a panic for that scenario. I kept logic consistent with previous behavior when refactoring

[vijtrip2]

But the method does throw other errors apart from that panic.
I think it makes sense to catch error and panic
(OR ugly version would be to panic for all errors inside GetOutputShape and remove error from return type)

[brycahta]

I disagree with catching error and panic for this. Throughout the code having a nil shape is not considered an error (ex: line 114 here), and I don't think it'd be correct to start doing so now.

Your suggestion with removing error return type sounds like the right way to go. However, I wouldn't panic for all errors just the scenario where there's a wrapper override that is expected to work. What do you guys think?

[vijtrip2]

Fair enough...
Since in the existing code outputshape is being returned as nil and then it is being handled in line 114, we do not need the error in method signature.

I would be onboard with removing error from method signature and keep the panic statement in method body

[brycahta]

model.go does not take into account any output_wrapper_field_path overrides when processing Status fields for a CRD

I think what you're really referring to here is that model.go doesn't reference output_wrapper_field_path when generating CRD fields as part of the Create* request. I believe the code does use it in set_resource.sdk for Describe*.

You're right, that's clearer. I'll update PR title

Were you able to find a way for the adaptedSourceVarName to use the correct path?

Yessir, prior to these changes GetOutputShape would still try to unwrap a struct if it was the only member ref; however, I removed this because then the function would no longer be returning an OutputShape, but the nested field instead. Also, if no overrides are specified, GetOutputShape will return the original output shape,createOp.OutputRef.Shape.

Also it would be great to have a unit test for using GetOutputShape with Create*. That is, generating a list of Spec and Status fields and making sure it unwraps correctly (such as in your case with VpcEndpoint).

Good call, will add in next revision

[vijtrip2]

Looks good.

[RedbackThomson]

Thanks!

/lgtm

[ack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brycahta, 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 [RedbackThomson]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jaypipes]

@brycahta in future, please do create a GH issue for tracking these kinds of issues and include in the GH issue a simple test that demonstrates the buggy behaviour.

[jaypipes]

Sorry, but why didn't this block of code pick up the VpcEndpoint shape as being the wrapper struct?

[brycahta]

This code works as intended. The issue was model.go:164 executed first and was not checking for overrides initially, and then code-generator would crash because it could not locate VpcEndpoint identifier.

I added GetOutputShape call here after refactoring to clean up the code a bit-- the logic remains the same.

[RedbackThomson]

I would like to clarify that the original use for output_wrapper_field_path was working: You could unwrap a describe output structure into status and spec fields.

What we are adding in this is support for output_wrapper_field_path on a create output. That is, taking a wrapper structure from a create and creating new status and spec fields from its members. CreateVpcEndpointOutput has a wrapper of VpcEndpoint - its members are what we want in the spec and status for the CRD.

Some of the code changes @brycahta has made are re-using existing parts of the code that worked with the describe output to also work with create. I believe this is one of those elements.