-
Notifications
You must be signed in to change notification settings - Fork 398
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
ec2_asg: Add functionality to detach specified instances from ASG #933
ec2_asg: Add functionality to detach specified instances from ASG #933
Conversation
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mandar242 Thank you for working on this. You should add some integration tests for these features and a changelog fragment.
18ceaaf
to
96b9bbc
Compare
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mandar242 You also would need a changelog fragment and integration tests for these options.
96b9bbc
to
459af5e
Compare
Build failed.
|
Build failed.
|
Build failed.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor nits on some error messages, otherwise the code looks good - thanks @mandar242
recheck |
afaict the timeout is just because we have to wait for instances on several tasks and the API was being slow when the tests ran. |
recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor grammar fixes - LGTM!
def detach(connection): | ||
group_name = module.params.get('name') | ||
detach_instances = module.params.get('detach_instances') | ||
as_group = describe_autoscaling_groups(connection, group_name)[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a case where multiple autoscaling groups are returned here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think as we are passing only single group name, we will only single auto scaling group's info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just checking! Wasnt sure if there were cases where multiple groups had the same name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ASG name is unique within a region and account. Since we can only act on one region and account at a time, this works well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code lgtm, thanks very much @mandar242! I'll hold off on tagging this to merge over the weekend in case @alinabuzachis would like to update her review but with CI passing I think this is ready.
…sible-collections#933) ec2_asg: Add functionality to detach specified instances from ASG SUMMARY Adds feature to detach specified instances from a AutoScalingGroup rather than terminating them directly. Detached instances are not terminated and can be managed independently. Implements ansible-collections#649 ISSUE TYPE Feature Pull Request COMPONENT NAME ec2_asg ADDITIONAL INFORMATION Makes use of https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/autoscaling.html#AutoScaling.Client.detach_instances Reviewed-by: Alina Buzachis <None> Reviewed-by: Mandar Kulkarni <[email protected]> Reviewed-by: Jill R <None> Reviewed-by: Joseph Torcasso <None>
ec2_asg: backport PRs 960 and 933 SUMMARY ec2_asg: backports PRs #960 and #933 ISSUE TYPE Bugfix Pull Request Docs Pull Request Feature Pull Request New Module Pull Request COMPONENT NAME ADDITIONAL INFORMATION Reviewed-by: Markus Bergholz <[email protected]>
SUMMARY
Adds feature to detach specified instances from a AutoScalingGroup rather than terminating them directly.
Detached instances are not terminated and can be managed independently.
Implements #649
ISSUE TYPE
COMPONENT NAME
ec2_asg
ADDITIONAL INFORMATION
Makes use of
https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/autoscaling.html#AutoScaling.Client.detach_instances