Implement ReadMany for CheckRequiredFieldsMissingFromShape

[brycahta]

Issue #, if available: #890

Description of changes:

• Adds checkRequiredFieldsMissingFromShapeReadMany to check.go to handle ReadMany operations without a corresponding ReadOne.
• The new method will require resource identifier field should there exist an identifier or identifierS field in the ReadMany operation.
  • Eventually newListRequestPayload will use this identifier field to populate its request in order to guarantee the ReadMany operation returns the desired resource.
  • ex: DescribeVpcs has a VpcIds field. Therefore, Vpc shape should require VpcId, which is part of its status field.
• Updated tests and templates

Testing:

• make test ✅
• make build-controller ✅

func (rm *resourceManager) sdkFind(
	ctx context.Context,
	r *resource,
) (latest *resource, err error) {
	rlog := ackrtlog.FromContext(ctx)
	exit := rlog.Trace("rm.sdkFind")
	defer exit(err)
	// If any required fields in the input shape are missing, AWS resource is
	// not created yet. Return NotFound here to indicate to callers that the
	// resource isn't yet created.
	if rm.requiredFieldsMissingFromReadManyInput(r) {
		return nil, ackerr.NotFound
	}

	input, err := rm.newListRequestPayload(r)

...


// requiredFieldsMissingFromReadManyInput returns true if there are any fields
// for the ReadMany Input shape that are required but not present in the
// resource's Spec or Status
func (rm *resourceManager) requiredFieldsMissingFromReadManyInput(
	r *resource,
) bool {
	return r.ko.Status.VPCID == nil

}

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

[ack-bot]

Hi @brycahta. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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.

[brycahta]

I wanted a test where the Shape/CR have multiple identifiers, but the Id gets prioritized. The only one I could think of was SecurityGroups. The DescribeInput can take either Name or Id, and the SecurityGroup CR has both Name in the Spec and Id in the Status.

In this scenario Id should be a required field. However, when trying to test this I uncovered an edge case in the FindIdentifiers helper funcs. No identifiers are returned for SecurityGroup CR or the DescribeInput because the fields are not securityGroupName/securityGroupId, but groupName/groupId. 😒

I can address this edge case in a different PR, but if anyone else has a good API to test the above case I'd love to add and test it now!

[jaypipes]

@brycahta this is almost there. I have a request for a couple clarifying code comments to help future readers of this code inline.

[jaypipes]

Great explanation. ++

[jaypipes]

Great work on this @brycahta, thank you!

[jaypipes]

👍

[jaypipes]

/lgtm

[ack-bot]

[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:

• OWNERS [jaypipes]

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