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

Autoscaling Groups Lifecycle Hooks module #22412

Merged
merged 14 commits into from
Jan 3, 2018

Conversation

tsiganenok
Copy link
Contributor

SUMMARY

New module which allows managing ASG Lifecycle hooks.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

ec2_asg_lifecycle_hook

ANSIBLE VERSION

[root@localhost playbooks]# ansible --version
ansible 2.2.1.0
config file = /etc/ansible/ansible.cfg
configured module search path = Default w/o overrides

ADDITIONAL INFORMATION

There is no module allowing Lifecycle Hooks management for AWS Autoscaling Groups for now.
More about Lifecycle Hooks: http://docs.aws.amazon.com/autoscaling/latest/userguide/lifecycle-hooks.html
It uses boto3 and there is no separate documentation for hooks, but only in autoscaling section:
http://boto3.readthedocs.io/en/latest/reference/services/autoscaling.html

@ansibot
Copy link
Contributor

ansibot commented Mar 8, 2017

@ansibot ansibot added affects_2.3 This issue/PR affects Ansible v2.3 aws cloud module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. labels Mar 8, 2017
@ansibot
Copy link
Contributor

ansibot commented Mar 8, 2017

The test ansible-test compile --python 2.6 failed with the following error:

lib/ansible/modules/cloud/amazon/ec2_asg_lifecycle_hook.py:176:38: SyntaxError: modified = {o : (d1[o], d2[o]) for o in intersect_keys if d1[o] != d2[o]}

click here for bot help

@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Mar 10, 2017
@ansibot
Copy link
Contributor

ansibot commented Mar 10, 2017

The test ansible-test compile --python 2.6 failed with the following error:

lib/ansible/modules/cloud/amazon/ec2_asg_lifecycle_hook.py:176:38: SyntaxError: modified = {o : (d1[o], d2[o]) for o in intersect_keys if d1[o] != d2[o]}

The test ansible-test sanity --test ansible-doc --python 2.6 failed with the following error:

Command "ansible-doc ec2_asg_lifecycle_hook" returned exit status 1.
>>> Standard Error
[ERROR]: unable to parse /root/src/github.com/ansible/ansible/lib/ansible/modu
les/cloud/amazon/ec2_asg_lifecycle_hook.py
ERROR! module ec2_asg_lifecycle_hook missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test validate-modules failed with the following errors:

lib/ansible/modules/cloud/amazon/ec2_asg_lifecycle_hook.py:0:0: E303 DOCUMENTATION fragment missing: ['aws', 'ec2', 'asg']
lib/ansible/modules/cloud/amazon/ec2_asg_lifecycle_hook.py:0:0: E307 version_added should be 2.3. Currently 1.6
lib/ansible/modules/cloud/amazon/ec2_asg_lifecycle_hook.py:0:0: E312 No RETURN documentation provided
lib/ansible/modules/cloud/amazon/ec2_asg_lifecycle_hook.py:0:0: E208 module_utils imports should import specific components, not "*". line 101
lib/ansible/modules/cloud/amazon/ec2_asg_lifecycle_hook.py:0:0: E208 module_utils imports should import specific components, not "*". line 102

click here for bot help

@mattclay
Copy link
Member

@tsiganenok I restarted CI for this PR since the bot didn't pick up all the errors the first time.

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Mar 10, 2017
role_arn=dict(type='str'),
notification_target_arn=dict(type='str'),
notification_meta_data=dict(type='str'),
heartbeat_timeout=dict(type='str'),
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is set to type='int' this should avoid the later type conversion

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Mar 14, 2017
@ansibot
Copy link
Contributor

ansibot commented Mar 14, 2017

The test ansible-test compile --python 2.6 failed with the following error:

lib/ansible/modules/cloud/amazon/ec2_asg_lifecycle_hook.py:183:38: SyntaxError: modified = {o : (d1[o], d2[o]) for o in intersect_keys if d1[o] != d2[o]}

The test ansible-test sanity --test ansible-doc --python 2.6 failed with the following error:

Command "ansible-doc ec2_asg_lifecycle_hook" returned exit status 1.
>>> Standard Error
[ERROR]: unable to parse /root/src/github.com/ansible/ansible/lib/ansible/modu
les/cloud/amazon/ec2_asg_lifecycle_hook.py
ERROR! module ec2_asg_lifecycle_hook missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test validate-modules failed with the following errors:

lib/ansible/modules/cloud/amazon/ec2_asg_lifecycle_hook.py:0:0: E208 module_utils imports should import specific components, not "*". line 108
lib/ansible/modules/cloud/amazon/ec2_asg_lifecycle_hook.py:0:0: E208 module_utils imports should import specific components, not "*". line 109
lib/ansible/modules/cloud/amazon/ec2_asg_lifecycle_hook.py:0:0: E316 ANSIBLE_METADATA.metadata_version: required key not provided @ data['metadata_version']. Got None
lib/ansible/modules/cloud/amazon/ec2_asg_lifecycle_hook.py:0:0: E316 ANSIBLE_METADATA.version: extra keys not allowed @ data['version']. Got '1.0'

click here for bot help

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Mar 14, 2017
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Mar 15, 2017
@ansibot
Copy link
Contributor

ansibot commented Mar 15, 2017

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 15, 2017
@yuval-almog
Copy link

+1

@shayisrael
Copy link

+1
Will be very useful for me

@classx
Copy link

classx commented Mar 20, 2017

+1 very useful

@ansibot ansibot added the affects_2.4 This issue/PR affects Ansible v2.4 label Mar 20, 2017
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 11, 2017
@ansibot
Copy link
Contributor

ansibot commented Apr 20, 2017

@tsiganenok this PR contains the following merge comits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot ansibot removed the community_review In order to be merged, this PR must follow the community review workflow. label Apr 20, 2017
@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Apr 20, 2017
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 22, 2017
@stevenbaker
Copy link

Hope this makes it into 2.4.0. Looks super useful.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 10, 2017
Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

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

This looks like a helpful module to have. Thanks for the time and effort you've spent on it!

module: ec2_asg_lifecycle_hook
short_description: Create, delete or update AWS ASG Lifecycle Hooks.
description:
- Can create, delete or update Lifecycle Hooks for AWS Autoscaling Groups.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a more thorough description?

try:
import boto3
import botocore
HAS_BOTO3 = True
Copy link
Contributor

Choose a reason for hiding this comment

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

You could import this from ansible.module_utils.ec2 instead of manually setting it here and in the except ImportError.

'''

RETURN = '''

Copy link
Contributor

@s-hertel s-hertel Jun 15, 2017

Choose a reason for hiding this comment

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

Can you document what the returns look like?
I think this is fine since there isn't any helpful data returned by the boto3 calls. Maybe a facts module would be nice later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I'm leaving it as is?

if changed:
try:
connection.put_lifecycle_hook(**lch_params)
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

It would better to catch botocore.exceptions.ClientError instead of Exception.

if default_result:
lch_params['DefaultResult'] = default_result

existing_hook = connection.describe_lifecycle_hooks(
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to have a try/except around this call.

lch_name = module.params.get('lifecycle_hook_name')
asg_name = module.params.get('autoscaling_group_name')

all_hooks = connection.describe_lifecycle_hooks(
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to have a try/except around this call.

try:
connection.delete_lifecycle_hook(**lch_params)
changed = True
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

Catch botocore.exceptions.ClientError instead of Exception here too.

connection = boto3_conn(module, conn_type='client', resource='autoscaling', region=region, endpoint=ec2_url, **aws_connect_params)
if not connection:
module.fail_json(msg="failed to connect to AWS for the given region: %s" % str(region))
except boto.exception.NoAuthHandlerFound as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be botocore.exceptions.NoCredentialsError.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jun 15, 2017
@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Jun 29, 2017
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 27, 2017
@tsiganenok
Copy link
Contributor Author

@s-hertel Sorry for the time it took, but finally I fixed everything you asked.
What should I do about the RETURN?

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Aug 4, 2017
@ryansb ryansb self-assigned this Aug 31, 2017
@ansibot ansibot added the new_contributor This PR is the first contribution by a new community member. label Oct 18, 2017
@ansibot ansibot removed the new_contributor This PR is the first contribution by a new community member. label Nov 3, 2017
@tsiganenok
Copy link
Contributor Author

@ryansb Hi, is this going to be merged or do I need to fix / change anything?

@tsiganenok
Copy link
Contributor Author

@s-hertel @ryansb Hi, do I need to fix / edit anything else in order to get it merged?

@willthames willthames merged commit b14e5c3 into ansible:devel Jan 3, 2018
@willthames
Copy link
Contributor

Merged, thanks for this @tsiganenok. This will indeed be very useful.

@willthames
Copy link
Contributor

Didn't spot the stale_ci, oops. Should have reran the tests. I'll push a new PR shortly for the fixes.

@willthames
Copy link
Contributor

#34379 added to fix some of the problems that would have occurred had tests been rerun.

@tsiganenok tsiganenok deleted the ec2_asg_lifecycle_hook branch January 11, 2018 23:32
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.3 This issue/PR affects Ansible v2.3 affects_2.4 This issue/PR affects Ansible v2.4 aws cloud module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_module This PR includes a new module. new_plugin This PR includes a new plugin. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants