Replace primary_identifier_field_name with is_primary_key

[RedbackThomson]

Description of changes:
We have had quite a few issues with the logic of SetResourceIdentifiers struggling to find the correct identifier field name or matching it to the corresponding spec or status field. The current implementation solves the first of these issues by pointing directly to the identifier member in the input shape, but the logic to link this back to a spec or status field is still flawed.
Rather than pointing to a member in the input shape and trying to infer the spec or status field, this new implementation directly denotes a field on a resource as the "primary key". The SetResourceIdentifiers logic can now directly find the target field and create the corresponding setter. The override logic for ARNs has now been moved to a is_arn_primary_key on the ResourceConfig object, since we cannot access the ARN as a FieldConfig.

This is a backwards compatibility breaking change as all existing primary_identifier_field_name configurations need to be refactored into is_primary_key fields on the respective resource's field. For example:

operations:
  DescribeDBInstances:
    primary_identifier_field_name: DBInstanceIdentifier
  DescribeDBSubnetGroups:
    primary_identifier_field_name: DBSubnetGroupName

becomes:

resources:
  DBInstance:
    fields:
      DBInstanceIdentifier:
        is_primary_key: true
  DBSubnetGroup:
    fields:
      Name:
        is_primary_key: true

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

[RedbackThomson]

Seeing an issue with additional keys duplicating the primary key:

func (r *resource) SetIdentifiers(identifier *ackv1alpha1.AWSIdentifiers) error {
	if identifier.NameOrID == "" {
		return ackerrors.MissingNameIdentifier
	}
	r.ko.Spec.ResourceID = &identifier.NameOrID

	f3, f3ok := identifier.AdditionalKeys["resourceID"]
	if f3ok {
		r.ko.Spec.ResourceID = &f3
	}
	f4, f4ok := identifier.AdditionalKeys["scalableDimension"]
	if f4ok {
		r.ko.Spec.ScalableDimension = &f4
	}
	f5, f5ok := identifier.AdditionalKeys["serviceNamespace"]
	if f5ok {
		r.ko.Spec.ServiceNamespace = &f5
	}

	return nil
}

Edit: Resolved by comparing additional key field to the primary key field

[brycahta]

Thanks a lot for this @RedbackThomson -- should unblock some of my work as well!

I'm downloading this PR locally to test, but will post questions here prior to SIG for discussion:

1. Interface seems a little clunky. I agree with setting at the resource level, but I don't think is_primary_key is clearer or better than is_identifier (or similar).

• Defining nested paths won't be clean
• Should setting arn as primary key exist on same level (in yaml)?

2. Implementation doesn't seem to handle nested paths, can you confirm? Ex: I need VpcEndpoint.vpcEndpointId from DescribeVpcEndpoints response

3. Would it be safe to remove some of that custom identifier logic I introduced a little while back? If not, maybe it could be simplified in future PR

[brycahta]

LGTM

[mbaijal]

Haven't reviewed thoroughly but the code works for both ApplicationAutoscaling resources exactly as expected. Approving based on functionality.

[mbaijal]

what's up with this commented out code ?

[RedbackThomson]

It's a code sample for the generated code for the following block :)

[mbaijal]

ApplicationAutoscaling does not utilize this, so I have not verified this snippet, looks ok though.

[RedbackThomson]

The SageMaker controller uses it (for ModelPackage). We have unit tests that cover it.

[mbaijal]

My local config has both is_name and is_primary_key since I had not looked at this code yet. But generator does not throw an error. We should have some validation in the future for extra fields too.

[RedbackThomson]

Yes, I did a global search for all service controllers using is_name and I believe SageMaker is the only example of them. I also agree that the generator should at least warn teams if they have unspecified fields.

[ack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

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