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.

Sign up for GitHub

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

Jump to bottom

ec2_elb_lb fix/add useful exception handling to _create_elb_listeners #35994

Closed
wants to merge 2 commits into from
Closed

ec2_elb_lb fix/add useful exception handling to _create_elb_listeners #35994

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d18f6d3
fix add useful exception handling to _create_elb_listeners
Feb 10, 2018
45531b5
fix build errors
Feb 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 18 additions & 4 deletions lib/ansible/modules/cloud/amazon/ec2_elb_lb.py
View file
Open in desktop
Expand Up @@ -738,10 +738,24 @@ def _create_elb(self):
self.status = 'created'

def _create_elb_listeners(self, listeners):
"""Takes a list of listener tuples and creates them"""
# True if succeeds, exception raised if not
self.changed = self.elb_conn.create_load_balancer_listeners(self.name,
complex_listeners=listeners)
"""Takes a list of listener tuples and creates them

Arguments:
listeners (list): list of tuples

Returns:
True if succeeds

Raises:
IndexError: If HTTPS is specified as a listener but SSL cert is not specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically these docs aren't quite right. This doesn't raise IndexError - it catches an IndexError and then calls fail_json itself, so callers don't need to also catch IndexError.

Copy link
Author

Choose a reason for hiding this comment

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

Ahhh ok righto then, yeah now that you explain it like that I feel kinda stupid haha. Sorry about that, trying to make it clean has probably resulted in me trying to hard on this PR. So because it doesn't Raise the error, I should just remove the Raises section and leave the docstring as is? Or should I replace it with a Catch section? Sorry for the 50 questions here.


"""
try:
self.changed = self.elb_conn.create_load_balancer_listeners(self.name,
complex_listeners=listeners)
except IndexError as e:
self.module.fail_json(msg='unable to create listener for port 443 HTTPS without specifying an SSL cert: %s'
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to hardcode the port.

Copy link
Author

Choose a reason for hiding this comment

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

msg='unable to create listener for HTTPS without specifying an SSL cert: %s'

Is that better?

% e.message, exception=traceback.format_exc())
Copy link
Contributor

Choose a reason for hiding this comment

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

e.message doesn't exist on Python 3 so this may raise an AttributeError

Copy link
Author

Choose a reason for hiding this comment

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

I'll work out what I need to do with this instead then, I kinda didn't even think about the different python versions when I wrote this, my mistake.


def _delete_elb_listeners(self, listeners):
"""Takes a list of listener tuples and deletes them from the elb"""
Expand Down