[do-not-merge-before-28] Enable updates for ScalableTarget and ScalingPolicy #27
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
mbaijal
force-pushed
the
enable-updates
branch
from
1a2f1c3 to
0d19511
Compare
| require ( | ||
| github.com/aws-controllers-k8s/runtime v0.2.1 | ||
| github.com/aws/aws-sdk-go v1.37.4 | ||
| github.com/aws-controllers-k8s/runtime v0.12.0 |
There was a problem hiding this comment.
@mbaijal I would recommend to do the runtime update in a separate PR, since will require regenerating all the apis and tuning/re-running all the unit tests
There was a problem hiding this comment.
Agree. It's quite difficult to see in this review what is simply updating to 0.12.0 and what is custom code. Especially considering we are coming from 0.2.1 - there are a lot of rebasing changes.
There was a problem hiding this comment.
I had to update the runtime here since I was using the latest code generator changes for this PR which includes late initializers and without this version of runtime things don't work.
I could however have a separate PR for runtime update which gets merged before this one.
There was a problem hiding this comment.
Yes please. A separate PR for runtime from v0.2.1 to v0.12.0 will greatly reduce subsequent PRs (like this one)
mbaijal
force-pushed
the
enable-updates
branch
from
0d19511 to
161afe0
Compare
|
@mbaijal: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
mbaijal
changed the title
|
Will create a new pull request since runtime update is in a separate PR now and there have been other changes since. |
… operations (#166) 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](aws-controllers-k8s/applicationautoscaling-controller#27) 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- 1. the cfg itself is nil 2. cfg is not nil but opId is not in the cfg 3. cfg is not nil, the opID is in the cfg but the opType is not specified ``` operations: DescribeScalableTargets: primary_identifier_field_name: ResourceID ``` 4. One API -> one operations ``` operations: PutScalingPolicy: operation_type: - Create ``` 5. 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.
4 participants
Issue #, if available:
aws-controllers-k8s/community#867
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.