Generate resource identifiers from `ReadOne` or `ReadMany` #105

RedbackThomson · 2021-06-23T19:18:38Z

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

Description of changes:
This pull request introduces a smarter system for detecting resource primary identifiers by using the fields from the ReadOne (or optionally ReadMany) operations.
It detects if a field is attempting to use the resource ARN and will set the status metadata to the corresponding identifier element. Otherwise, it will search through each of the required fields in the operation to attempt to identify the "primary" identifier field (such as *Name, Name, *ID). All other fields in the operation can be configured through the use of the new AdditionalKeys map (https://github.com/aws-controllers-k8s/runtime/pull/13/files). Optionally, the primary identifier field can be configured in the generator under operations.<operation>.primary_identifier_field_name if one cannot be (or is incorrectly) identified.

Example output from applicationautoscaling ScalingPolicy:

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

	f0, f0ok := identifier.AdditionalKeys["scalableDimension"]
	if f0ok {
		r.ko.Spec.ScalableDimension = f0
	}
	f1, f1ok := identifier.AdditionalKeys["serviceNamespace"]
	if f1ok {
		r.ko.Spec.ServiceNamespace = f1
	}

	return nil
}

Example output from mq Broker:

func (r *resource) SetIdentifiers(identifier *ackv1alpha1.AWSIdentifiers) error {
	if identifier.NameOrID == nil {
		return ackerrors.MissingNameIdentifier
	}
	r.ko.Status.BrokerID = identifier.NameOrID

	return nil
}

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

RedbackThomson · 2021-06-23T19:18:54Z

/assign @jaypipes @a-hilaly

RedbackThomson · 2021-06-23T20:20:44Z

/retest

vijtrip2

Looks good to me. Just one comment above about secret.

pkg/generate/code/set_resource.go

a-hilaly

LGTM Over all!

Left few comments bellow

pkg/generate/code/set_resource.go

a-hilaly · 2021-06-29T08:22:08Z

Nice! 👍
/approve

pkg/generate/code/set_resource.go

jaypipes · 2021-06-29T19:26:16Z

pkg/generate/code/set_resource.go

+			// r.ko.Status.BrokerID = identifier.NameOrID
+			primaryKeyOut += fmt.Sprintf("%s%s.%s.%s = %s.NameOrID\n", indent, targetVarName, memberPath, cleanMemberName, sourceVarName)
+		} else {
+			// f0, f0ok := identifier.AdditionalKeys["scalableDimension"]


Did we plan to support case-insensitive lookups? Because if the Spec or Status field is PascalCased, per Go export rules, and we're asking users to use lowerCamelCased, things might get confusing here.

I think we should probably support case-insensitive key lookups here.

Thoughts?

All other fields in the k8s spec are lowerCamelCased, I am trying to make the additionalKeys map feel more "k8s native".

@RedbackThomson yeah, I guess that's a fair point. I just worry that without the TODO below on line 953 being implemented, that simple typos here will be quite difficult to detect.

Yeah totally understandable. A typo in an additionalKeys key would not be detectable and would not produce any error statements at the moment. It would be interesting to pop values out of the additionalKeys map as they are consumed and return error conditions for any remaining values.

jaypipes

OK, I'm good with this. @vijtrip2 @a-hilaly can you do one more once-over and lgtm if good with you?

jaypipes · 2021-06-30T19:15:28Z

pkg/generate/code/set_resource.go

+			// r.ko.Status.BrokerID = identifier.NameOrID
+			primaryKeyOut += fmt.Sprintf("%s%s.%s.%s = %s.NameOrID\n", indent, targetVarName, memberPath, cleanMemberName, sourceVarName)
+		} else {
+			// f0, f0ok := identifier.AdditionalKeys["scalableDimension"]


@RedbackThomson yeah, I guess that's a fair point. I just worry that without the TODO below on line 953 being implemented, that simple typos here will be quite difficult to detect.

a-hilaly · 2021-06-30T19:43:54Z

LGTM
/approve

a-hilaly · 2021-07-01T11:39:21Z

/lgtm

ack-bot · 2021-07-01T11:39:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes, RedbackThomson, vijtrip2

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~~ [A-Hilaly,RedbackThomson,jaypipes,vijtrip2]

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

…de (#115) Related to #105 and aws-controllers-k8s/runtime#13 Description of changes: - replace nil pointer checks with string zero value checks - replace `string` value assignment with `string` ptr assignments By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…de (aws-controllers-k8s#115) Related to aws-controllers-k8s#105 and aws-controllers-k8s/runtime#13 Description of changes: - replace nil pointer checks with string zero value checks - replace `string` value assignment with `string` ptr assignments By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…ollers-k8s#105) Issue #, if available: aws-controllers-k8s/community#842 Description of changes: This pull request introduces a smarter system for detecting resource primary identifiers by using the fields from the `ReadOne` (or optionally `ReadMany`) operations. It detects if a field is attempting to use the resource ARN and will set the status metadata to the corresponding identifier element. Otherwise, it will search through each of the required fields in the operation to attempt to identify the "primary" identifier field (such as `*Name`, `Name`, `*ID`). All other fields in the operation can be configured through the use of the new `AdditionalKeys` map (https://github.com/aws-controllers-k8s/runtime/pull/13/files). Optionally, the primary identifier field can be configured in the generator under `operations.<operation>.primary_identifier_field_name` if one cannot be (or is incorrectly) identified. Example output from `applicationautoscaling` `ScalingPolicy`: ```golang func (r *resource) SetIdentifiers(identifier *ackv1alpha1.AWSIdentifiers) error { if identifier.NameOrID == nil { return ackerrors.MissingNameIdentifier } r.ko.Spec.ResourceID = identifier.NameOrID f0, f0ok := identifier.AdditionalKeys["scalableDimension"] if f0ok { r.ko.Spec.ScalableDimension = f0 } f1, f1ok := identifier.AdditionalKeys["serviceNamespace"] if f1ok { r.ko.Spec.ServiceNamespace = f1 } return nil } ``` Example output from `mq` `Broker`: ```golang func (r *resource) SetIdentifiers(identifier *ackv1alpha1.AWSIdentifiers) error { if identifier.NameOrID == nil { return ackerrors.MissingNameIdentifier } r.ko.Status.BrokerID = identifier.NameOrID return nil } ``` By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…de (aws-controllers-k8s#115) Related to aws-controllers-k8s#105 and aws-controllers-k8s/runtime#13 Description of changes: - replace nil pointer checks with string zero value checks - replace `string` value assignment with `string` ptr assignments By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Nicholas Thomson added 3 commits June 23, 2021 12:09

Switch SetIdentifiers to template code

ae4682e

Add primary identifier field config

8413c3a

Refactor code flow

8255972

ack-bot requested review from a-hilaly and vijtrip2 June 23, 2021 19:18

ack-bot added the approved label Jun 23, 2021

RedbackThomson requested review from jaypipes and removed request for a-hilaly and vijtrip2 June 23, 2021 19:18

ack-bot assigned a-hilaly and jaypipes Jun 23, 2021

RedbackThomson requested a review from a-hilaly June 23, 2021 19:19

Nicholas Thomson added 2 commits June 23, 2021 12:33

Update additional key syntax

32ef02e

Add unit tests

cd53dbc

vijtrip2 reviewed Jun 24, 2021

View reviewed changes

pkg/generate/code/set_resource.go Show resolved Hide resolved

a-hilaly reviewed Jun 24, 2021

View reviewed changes

pkg/generate/code/set_resource.go Show resolved Hide resolved

pkg/generate/code/set_resource.go Show resolved Hide resolved

pkg/generate/code/set_resource.go Outdated Show resolved Hide resolved

pkg/generate/code/set_resource.go Outdated Show resolved Hide resolved

Add GoDoc and clean up code flow

4f3a2a7

vijtrip2 approved these changes Jun 29, 2021

View reviewed changes

jaypipes reviewed Jun 29, 2021

View reviewed changes

Nicholas Thomson added 2 commits June 29, 2021 12:58

Error out on GetAttributes

3b95cb3

Fix member path value

f287df2

jaypipes approved these changes Jun 30, 2021

View reviewed changes

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 1, 2021

ack-bot merged commit 316cbcb into aws-controllers-k8s:main Jul 1, 2021

a-hilaly mentioned this pull request Jul 2, 2021

Correct type mismatch errors in SetResourceIdentifiers generated code #115

Merged

Generate resource identifiers from ReadOne or ReadMany #105

Generate resource identifiers from ReadOne or ReadMany #105

Uh oh!

Conversation

RedbackThomson commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RedbackThomson commented Jun 23, 2021

Uh oh!

RedbackThomson commented Jun 23, 2021

Uh oh!

vijtrip2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

a-hilaly commented Jun 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaypipes Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

RedbackThomson Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

jaypipes Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

RedbackThomson Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

jaypipes Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Jun 30, 2021

Uh oh!

a-hilaly commented Jul 1, 2021

Uh oh!

ack-bot commented Jul 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Generate resource identifiers from `ReadOne` or `ReadMany` #105

Generate resource identifiers from `ReadOne` or `ReadMany` #105

RedbackThomson commented Jun 23, 2021 •

edited

Loading