Skip to content
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

Support 'list' type in output_wrapper_field_path #337

Merged
merged 7 commits into from
May 24, 2022
Merged

Support 'list' type in output_wrapper_field_path #337

merged 7 commits into from
May 24, 2022

Conversation

brycahta
Copy link
Contributor

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

Description of changes:

  • adds support for list types to output_wrapper_field_path
  • cleans up logic in setResourceReadMany
  • updates EC2 unit tests in model and code packages

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

@ack-bot ack-bot requested review from a-hilaly and jaypipes May 20, 2022 22:01
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.

@brycahta I think this code might be getting a little too icky. I'm thinking perhaps it would be better if we instead took an approach of generating a separate findSourceShape() function that would return a singular shape that we wish to set the CR's fields from and have SetResource() only contain code that loops over a singular Shape's member fields.

In other words, we'd want to generate Go code that looks something like this:

// findSourceShape{{ $OpType }} returns the Shape that contains the fields
// we want to set on our CR when given an Output shape for the {{ $OpType }}
// Operation
func (rm *resourceManager) findSourceShape{{ $OpType }}(
	resp *svcsdk.{{$crd.Ops.$OpType.OutputRef.Shape.ShapeName }},
) (*svcsdk.{{ $crd.GetSingularStructNameForOp $OpType }}, error) {
	{{ GoCodeFindSingularSource $crd $OpType }}
}

We would need a pkg/model.CRD.GetSingularStructNameForOp method that would return the name of the sdksvc struct that contains the fields that we want to set a CR's field values from.

It might look something like this:

// GetSingularStructNameForOp returns the name of the struct
// in the svcsdk package that contains fields that we want to set
// a CR's field values from.
//
// Any "wrapper" fields in an output shape will be popped as needed.
//
// For List/ReadMany operations, for example, this method will
// return the name of the struct containing a single resource
// representation within the list.
func (crd *CRD) GetSingularStructNameForOp(
	opType mode.OpType,
) string {
	// .. code here would pop any wrapper fields and
	// pop any list container fields, returning just the
	// name of the struct that represents a single resource
	// within the Operation's Output shape...
}

We would need to implement a new Go-code-outputting function in pkg/generate/code called FindSingularSource() that would produce the implementation of the findSourceShape resource manager method referenced in the template above.

Something like this...

// FindSingularSource produces the Go code that accepts an Output shape
// from the response of an operation and returns the struct containing the
// singular resource representation that we will set a CR's fields from.
//
// For List/ReadMany operations, the code produced from this function might
// look something like this (example from VPC resource in EC2 controller):
//
// if len(resp.Vpcs) == 1 {
//     return resp.Vpcs[0]
// }
// return nil
//
// For other operations such as EC2's RunInstances output response, the
// code might look like this instead:
//
// if len(resp.Reservations) == 1 {
//     if len(resp.Reservations[0].Instances) == 1 {
//         return resp.Reservations[0].Instances[0]
//     }
// }
// return nil
//
// For ReadOne operations, where the response Output shape contain a
// "wrapper" field that contains the singular resource fields,
// the code returned from this function might look like this,
// such as the RDS CreateGlobalCluster Output shape in this example (note
// that "GlobalCluster" is the "wrapper" field
//
// if resp.GlobalCluster != nil {
//     return resp.GlobalCluster
// }
// return nil
func FindSingularSource(
	r *model.CRD,
	opType model.OpType,
	respVarName string,
	indentLevel int,
) string {
	// Code returned here would contain all the logic to "find"
	// a struct within a response output struct that
	// represents the singular resource. This struct
	// might a wrapper field or an element field in a
	// list container field...
}

We could then update our sdkFind template in the code-generator from this:

	// Merge in the information we read from the API call above to the copy of
	// the original Kubernetes object we passed to the function
	ko := r.ko.DeepCopy()
{{- if $hookCode := Hook .CRD "sdk_read_many_pre_set_output" }}
{{ $hookCode }}
{{- end }}
{{ GoCodeSetReadManyOutput .CRD "resp" "ko" 1 }}

to something like this:

	// Pop the singular resource element from our response shape
	respSingle := rm.findSourceShapeReadMany(resp)
	// Merge in the information we read from the API call above to the copy of
	// the original Kubernetes object we passed to the function
	ko := r.ko.DeepCopy()
{{- if $hookCode := Hook .CRD "sdk_read_many_pre_set_output" }}
{{ $hookCode }}
{{- end }}
{{ GoCodeSetReadManyOutput .CRD "respSingle" "ko" 1 }}

pkg/generate/code/set_resource.go Show resolved Hide resolved
pkg/generate/code/set_resource.go Show resolved Hide resolved
@brycahta
Copy link
Contributor Author

I think this code might be getting a little too icky.

@jaypipes I'm with ya-- I have the refactor tasks for this package on my radar. I don't disagree with your solution, but to confirm this approach requires refactoring setResourceReadMany first, then resolving aws-controllers-k8s/community#1285? If so, would you mind commenting this approach on a refactor issue so we can discuss details?

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.

OK, from looking at the tests, it seems this does indeed solve the immediate problem at hand with RunInstances/DescribeInstances, so I'm going to merge this and work on a cleaner approach separately as I described in my earlier comment. Thanks @brycahta!

@jaypipes
Copy link
Collaborator

/lgtm

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

ack-bot commented May 24, 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

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