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

fix issues with Get/SetAttributes Go code generation #312

Merged
merged 2 commits into from Sep 17, 2020

Conversation

jaypipes
Copy link
Collaborator

This patch adds to the code generator for the SDK linkage for APIs that
use the SetAttributes-flavor of updating a resource. Previously, the
sdk.go files for these APIs had empty sdkUpdate operations with a
big TODO that ended up breaking the service controller (quite
predictably).

While implementing this patch, I stumbled upon yet another unexplainable
inconsistency between two SetAttributes operations within the SNS
service API. The SNS SetPlatformApplicationAttributes API call accepts a
field in its Input shape called Attributes that is a map of key/value
pairs for attributes to set on the platform application. Unfortunately,
the similarly-named SNS SetTopicAttributes API call apparently does
NOT work the same way. Instead, there is a single AttributeName and
AttributeValue field in the Input shape and you need to call the
SetTopicAttributes API call once for each modified attribute. :(

In fact, the [official documentation][0] for the SNS SetTopicAttributes
API call says that the AttributeName field in the Input shape is
actually a "map of attributes with their corresponding values". But
that isn't actually the case...

So, the code in this patch for now just contains a TODO in the
CRD.GoCodeSetAttributesSetInput() method for SetAttributes APIs that
can only operate on a single attribute at a time. We will use the
CustomOperation functionality as a temporary workaround for these APIs
while we come up with a more permanent code-generated solution.

There were a series of problems that I uncovered when investigating the
cause of Issue #296:

  • The code returned from GoCodeGetAttributesForOutput was failing to set
    Status.ACKResourceMetadata.ARN and
    Status.ACKResourceMetadata.OwnerAccountID when the attribute fields
    corresponded to the primary resource ARN or the owner ID. This was the
    direct cause of Issue sns: GENERATION FAILURE! there's a required field PlatformApplicationArn in Shape GetPlatformApplicationAttributesInput that isn't in either the CR's Spec or Status structs #296
  • The code in templates/pkg/crd_sdk.go.tpl that ran at the end of the
    sdkCreate() call was inadvertently overwriting any setters of
    Status.ACKResourceMetadata that had previously executed
  • The code returned from APIs with GetAttributes operations was always
    returning a nil-guard and constructor for the ACKResourceMetadata
    struct, even when no attribute fields actually set either ARN or
    OwnerAccountID. When I removed the check in the template for Status
    fields being required for the resp variable to be defined, this was
    causing "resp" variable unused compilation failures

Issue #296

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

There were a series of problems that I uncovered when investigating the
cause of Issue aws-controllers-k8s#296:

* The code returned from GoCodeGetAttributesForOutput was failing to set
  `Status.ACKResourceMetadata.ARN` and
  `Status.ACKResourceMetadata.OwnerAccountID` when the attribute fields
  corresponded to the primary resource ARN or the owner ID. This was the
  direct cause of Issue aws-controllers-k8s#296
* The code in `templates/pkg/crd_sdk.go.tpl` that ran at the end of the
  `sdkCreate()` call was inadvertently overwriting any setters of
  `Status.ACKResourceMetadata` that had previously executed
* The code returned from APIs with GetAttributes operations was *always*
  returning a nil-guard and constructor for the ACKResourceMetadata
  struct, even when no attribute fields actually set either ARN or
  OwnerAccountID. When I removed the check in the template for Status
  fields being required for the `resp` variable to be defined, this was
  causing `"resp" variable unused` compilation failures

Issue aws-controllers-k8s#296
This patch adds to the code generator for the SDK linkage for APIs that
use the SetAttributes-flavor of updating a resource. Previously, the
`sdk.go` files for these APIs had empty `sdkUpdate` operations with a
big TODO that ended up breaking the service controller (quite
predictably).

While implementing this patch, I stumbled upon yet another unexplainable
inconsistency between two SetAttributes operations within the SNS
service API. The SNS `SetPlatformApplicationAttributes` API call accepts a
field in its Input shape called `Attributes` that is a map of key/value
pairs for attributes to set on the platform application. Unfortunately,
the similarly-named SNS `SetTopicAttributes` API call apparently does
*NOT* work the same way. Instead, there is a single `AttributeName` and
`AttributeValue` field in the Input shape and you need to call the
`SetTopicAttributes` API call once for each modified attribute. :(

In fact, the [official documentation][0] for the SNS SetTopicAttributes
API call says that the `AttributeName` field in the Input shape is
actually a "*map* of attributes with their corresponding values". But
that isn't actually the case...

So, the code in this patch for now just contains a TODO in the
`CRD.GoCodeSetAttributesSetInput()` method for SetAttributes APIs that
can only operate on a single attribute at a time. We will use the
CustomOperation functionality as a temporary workaround for these APIs
while we come up with a more permanent code-generated solution.

Issue aws-controllers-k8s#296

[0]: https://docs.aws.amazon.com/sns/latest/api/API_SetTopicAttributes.html
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm 👍
Was fun reading the comments :)

@a-hilaly a-hilaly merged commit 85f992e into aws-controllers-k8s:main Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants