[to-be-closed] Enable AdoptedResource and add required custom logic for ScalableTarget #42
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
fix-adopted-resource-ids-post-release
branch
from
0848c57 to
58f6b96
Compare
| target_reference = k8s.create_reference( | ||
| CRD_GROUP, CRD_VERSION, TARGET_RESOURCE_PLURAL, target_name, "default" | ||
| ) | ||
| target_resource = k8s.wait_resource_consumed_by_controller(target_reference) |
There was a problem hiding this comment.
adopted resource reconciler now patches spec and status so if the status has ACK.Adopted true, we can just do k8s.get_resource
| @@ -0,0 +1,71 @@ | |||
| # Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
I think this name is more appropriate since this bootstraps AA resources using boto3 for the tests ?
| def application_autoscaling_client(): | ||
| return boto3.client("application-autoscaling") | ||
|
|
||
| def sagemaker_endpoint_register_scalable_target(resource_id): |
There was a problem hiding this comment.
arent these methods specific to the test? why in utills
There was a problem hiding this comment.
We can call it something other than utils but this file does for applicationAutoscaling what sagemaker_utils does for sagemaker. These are pre-setup steps for the test and not really related to what we want to test and thus IMO cleaner in a separate module.
| ) | ||
|
|
||
| yield resource_id | ||
| sagemaker_endpoint_deregister_scalable_target(resource_id) |
There was a problem hiding this comment.
delete only if exists? because its already being deleted at end of test
There was a problem hiding this comment.
I will have to run a botot3 describe call to check, seems unnecessary ?
There was a problem hiding this comment.
won't it throw an error if the resource doesn't exist?
There was a problem hiding this comment.
It hasn't so far and the resource should never exist at this point - let me check
mbaijal
force-pushed
the
fix-adopted-resource-ids-post-release
branch
from
58f6b96 to
c8a129f
Compare
mbaijal
changed the title
…#47) First two commits are the same as #42 but created a separate PR because I wanted preserve the changes of that PR. TODO: - I still need to address [one review comment from #42 ](#42 (comment)) - Runtime update to 0.14.0 Description of changes: - Enable AdoptedResource for both resources - Add required custom logic for ScalableTarget - Add integration test for the same Testing - Manually verified that scalableTarget can be adopted - Local Kind tests pass By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
3 participants
Description of changes:
Testing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.