Allow OperatorType to be a list such that one API can map to multiple operations #166

mbaijal · 2021-08-24T09:20:26Z

Allow OperatorType to be a list such that one API can map to multiple operations.
This is required in certain cases where the same API call maps to both Create and Update operations such as RegisterScalableTarget and PutScalingPolicy

Link to ApplicationAutoscaling PR with the related change
Note: This change will require all other services to update their generator configs as well.

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

Description of changes:

The getOpTypeAndResourceName method handles each of the following cases same as before in addition to the new case 5-

the cfg itself is nil
cfg is not nil but opId is not in the cfg
cfg is not nil, the opID is in the cfg but the opType is not specified

operations:
  DescribeScalableTargets:
  primary_identifier_field_name: ResourceID

One API -> one operations

operations:
  PutScalingPolicy:
    operation_type: 
    - Create

One API -> multiple operations

operations:
  PutScalingPolicy:
    operation_type: 
    - Create
    - Update

Testing

I tested by generating the controller code for both applicationAutoscaling and the SageMaker repos.
The unit tests pass as is.

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

ack-bot · 2021-08-24T09:20:34Z

Hi @mbaijal. 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.

mbaijal · 2021-08-24T09:26:26Z

pkg/model/sdk_helper.go

-	}

-	return opType, resName
+			if len(operationConfig.OperationType) > 0 {


pkg/model/sdk_helper.go

a-hilaly

👍

jaypipes

@mbaijal see inline for a helpful hint at solving the "need to update all the existing generator.yaml files" problem.

jaypipes · 2021-08-26T12:16:38Z

pkg/generate/config/operation.go

 	// Override for operation type in case of heuristic failure
 	// An example of this is `Put...` or `Register...` API operations not being correctly classified as `Create` op type
-	OperationType string `json:"operation_type"`
+	OperationType []string `json:"operation_type"`


I'll give you a little Go code I wrote for a different project that will allow you to change the type of this field to an array of strings without requiring all the existing generator.yaml files to be updated.

// NOTE(jaypipes): A type that can be represented in JSON as *either* a string // *or* an array of strings, which is what JSONSchema's type field needs. // see: https://github.com/go-yaml/yaml/issues/100 type StringArray []string func (a *StringArray) UnmarshalJSON(b []byte) error { var multi []string err := json.Unmarshal(b, &multi) if err != nil { var single string err := json.Unmarshal(b, &single) if err != nil { return err } *a = []string{single} } else { *a = multi } return nil }

You'd then change the above field definition to this instead:

OperationType StringArray `json:"operation_type"`

The existing generator.yaml files that specify operation_type as a string will work as-is without modification and the generator.yaml files that need to specify multiple operation types can specify a list of strings.

This looks good, I'll update the PR later today, thanks !

jaypipes

Couple tiny nits inline, @mbaijal, otherwise this looks great.

jaypipes · 2021-08-29T16:09:04Z

pkg/generate/config/operation.go

 import (
 	awssdkmodel "github.com/aws/aws-sdk-go/private/model/api"
-
+	"encoding/json"


This is a standard library import, so please place above the awssdkmodel import line and separate with an empty newline.

jaypipes · 2021-08-29T16:10:10Z

pkg/model/sdk_helper.go

 	"path/filepath"
 	"sort"
 	"strings"
+	"encoding/json"


Please run go fmt on your code or add an auto-formatter to your IDE. It will automatically correct the above into an alphabetical import order.

add an auto-formatter to your IDE

I should really, always forget to run the formatter. Ran it manually on the two files for now.

a-hilaly · 2021-08-30T12:34:22Z

pkg/generate/config/operation.go

-
+	"encoding/json"
 	"github.com/aws-controllers-k8s/code-generator/pkg/util"
+	awssdkmodel "github.com/aws/aws-sdk-go/private/model/api"


please change the import section to:

import ( "encoding/json" awssdkmodel "github.com/aws/aws-sdk-go/private/model/api" "github.com/aws-controllers-k8s/code-generator/pkg/util" )

Sure. gofmt does not fix this. Would you recommend using goimports or such to avoid such issues in the future ?

Yes exactly :). Sometimes goimports doesn't work as expected so you might need to fix the imports order manually. Here are the guidelines for imports https://github.com/golang/go/wiki/CodeReviewComments#imports

a-hilaly · 2021-08-30T12:36:01Z

pkg/model/sdk_helper.go

-			if operationConfig.ResourceName != "" {
-				resName = operationConfig.ResourceName
+		if b, err := json.Marshal(operationConfig.OperationType); err == nil {
+			unmarshaledOpTypes.UnmarshalJSON(b)


You don't have to call the method UnmarshalJSON here, bur rather call json.Unmarshal(b, &unmarshaledOpTypes)

… operations

a-hilaly · 2021-08-31T15:44:07Z

pkg/model/sdk_helper.go

-			if operationConfig.ResourceName != "" {
-				resName = operationConfig.ResourceName
+		if b, err := json.Marshal(operationConfig.OperationType); err == nil {
+			json.Unmarshal(b, &unmarshaledOpTypes)


the Unmarshal error should be handled here

Not applicable now.

a-hilaly · 2021-08-31T15:47:10Z

pkg/model/sdk_helper.go


-			if operationConfig.ResourceName != "" {
-				resName = operationConfig.ResourceName
+		if b, err := json.Marshal(operationConfig.OperationType); err == nil {


Thinking a second time about this, i think that you don't have to call json.Marshal/json.Unmarshall here - because StringArray.UnmarshalJSON will be called when model.Model is getting initialized https://github.com/aws-controllers-k8s/code-generator/blob/main/pkg/model/model.go#L700

Yep you are right :)

In this case, is operation.go the right place for the UnmarshalJSON method ?

Yes it makes sense to have the type and it methods on the same file so it's good!

mbaijal · 2021-08-31T17:48:09Z

/test unit-tests

ack-bot · 2021-08-31T17:48:24Z

@mbaijal: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test unit-tests

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.

a-hilaly · 2021-08-31T17:49:18Z

/ok-to-test

a-hilaly · 2021-08-31T17:52:37Z

pkg/model/sdk_helper.go

 	"github.com/aws-controllers-k8s/code-generator/pkg/names"
 	"github.com/aws-controllers-k8s/code-generator/pkg/util"
+
 	awssdkmodel "github.com/aws/aws-sdk-go/private/model/api"


Tiny nit: this import should be in the second block of imports

let me set up the goimports and create a tiny PR for this one.

a-hilaly

Looks good! thanks @mbaijal!

/lgtm

ack-bot · 2021-08-31T17:55:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, mbaijal

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]

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

) Issue #, if available: aws-controllers-k8s/community#867 Description of changes: 1. Enable updates based on changes to code generator made in this PR - aws-controllers-k8s/code-generator#166 2. Add an integration test to check for updates By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-bot requested review from RedbackThomson and a-hilaly August 24, 2021 09:20

ack-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 24, 2021

mbaijal changed the title ~~[Review] Allow OperatorType to be a list such that one API can map to multiple operations~~ Allow OperatorType to be a list such that one API can map to multiple operations Aug 24, 2021

mbaijal commented Aug 24, 2021

View reviewed changes

pkg/model/sdk_helper.go Outdated

}

return opType, resName

if len(operationConfig.OperationType) > 0 {

Copy link

Contributor Author

mbaijal Aug 24, 2021

Choose a reason for hiding this comment

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

clean up

mbaijal mentioned this pull request Aug 24, 2021

[do-not-merge-before-28] Enable updates for ScalableTarget and ScalingPolicy aws-controllers-k8s/applicationautoscaling-controller#27

Closed

a-hilaly reviewed Aug 24, 2021

View reviewed changes

pkg/model/sdk_helper.go Outdated Show resolved Hide resolved

pkg/model/sdk_helper.go Outdated Show resolved Hide resolved

mbaijal force-pushed the one-api-many-ops branch from c7357bd to 630b228 Compare August 25, 2021 00:15

mbaijal requested a review from a-hilaly August 26, 2021 06:05

a-hilaly approved these changes Aug 26, 2021

View reviewed changes

ack-bot added the approved label Aug 26, 2021

jaypipes reviewed Aug 26, 2021

View reviewed changes

mbaijal force-pushed the one-api-many-ops branch from 630b228 to 335a6d0 Compare August 27, 2021 20:32

mbaijal changed the title ~~Allow OperatorType to be a list such that one API can map to multiple operations~~ [wip] Allow OperatorType to be a list such that one API can map to multiple operations Aug 27, 2021

ack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 27, 2021

jaypipes requested changes Aug 29, 2021

View reviewed changes

mbaijal force-pushed the one-api-many-ops branch from 335a6d0 to 323a563 Compare August 29, 2021 18:25

mbaijal changed the title ~~[wip] Allow OperatorType to be a list such that one API can map to multiple operations~~ Allow OperatorType to be a list such that one API can map to multiple operations Aug 29, 2021

ack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 29, 2021

a-hilaly requested changes Aug 30, 2021

View reviewed changes

ack-bot removed the approved label Aug 30, 2021

Allow OperatorType to be a list such that one API can map to multiple…

16781ee

… operations

mbaijal force-pushed the one-api-many-ops branch from 323a563 to 16781ee Compare August 30, 2021 21:27

mbaijal mentioned this pull request Aug 30, 2021

Enable updates for ScalableTarget and ScalingPolicy [on branch update-runtime-0.12] aws-controllers-k8s/applicationautoscaling-controller#30

Closed

a-hilaly reviewed Aug 31, 2021

View reviewed changes

ack-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 31, 2021

Marshal, unmarshal needn't be called

d76c07f

mbaijal force-pushed the one-api-many-ops branch from 3e23b12 to d76c07f Compare August 31, 2021 17:49

a-hilaly reviewed Aug 31, 2021

View reviewed changes

a-hilaly approved these changes Aug 31, 2021

View reviewed changes

ack-bot assigned a-hilaly Aug 31, 2021

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

ack-bot added the approved label Aug 31, 2021

ack-bot merged commit 9f6ca92 into aws-controllers-k8s:main Aug 31, 2021

mbaijal mentioned this pull request Sep 1, 2021

Enable resource updates based on latest code-gen changes on v0.13.0 aws-controllers-k8s/applicationautoscaling-controller#32

Merged

Allow OperatorType to be a list such that one API can map to multiple operations #166

Allow OperatorType to be a list such that one API can map to multiple operations #166

Uh oh!

Conversation

mbaijal commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes:

Testing

Uh oh!

ack-bot commented Aug 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbaijal commented Aug 31, 2021

Uh oh!

ack-bot commented Aug 31, 2021

Uh oh!

a-hilaly commented Aug 31, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

ack-bot commented Aug 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mbaijal commented Aug 24, 2021 •

edited

Loading