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

wants to merge 2 commits into from

Conversation

JohnVonNeumann
Copy link

SUMMARY

Ran into an issue the other day whilst creating some ELBs and the exception messages seemed barely related to the actual problem so I've add some hopefully more useful exception handling. I've named it a fix but in reality I'm not sure what you'd call this, first PR so sorry if I've gotten anything wrong here.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ec2_elb_lb

ANSIBLE VERSION
ansible 2.6.0 (fix/ljaw/add-exception-handling-to-elb-listeners d18f6d3db3) last updated 2018/02/10 17:03:16 (GMT +1100)
  config file = /home/lw/code/work/infrastructure/ansible/ansible.cfg
  configured module search path = [u'/home/lw/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/lw/code/open_source/ansible/lib/ansible
  executable location = /home/lw/code/open_source/ansible/bin/ansible
  python version = 2.7.12 (default, Dec  4 2017, 14:50:18) [GCC 5.4.0 20160609]
ADDITIONAL INFORMATION

@ansibot
Copy link
Contributor

ansibot commented Feb 10, 2018

cc @jsdalton @willthames
click here for bot help

@ansibot ansibot added aws bugfix_pull_request cloud committer_review In order to be merged, this PR must follow the certified review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. support:certified This issue/PR relates to certified code. labels Feb 10, 2018
@ansibot
Copy link
Contributor

ansibot commented Feb 10, 2018

The test ansible-test sanity --test pylint [explain] failed with 3 errors:

lib/ansible/modules/cloud/amazon/ec2_elb_lb.py:742:0: trailing-whitespace Trailing whitespace
lib/ansible/modules/cloud/amazon/ec2_elb_lb.py:751:0: trailing-whitespace Trailing whitespace
lib/ansible/modules/cloud/amazon/ec2_elb_lb.py:757:0: trailing-whitespace Trailing whitespace

The test ansible-test sanity --test pep8 [explain] failed with 5 errors:

lib/ansible/modules/cloud/amazon/ec2_elb_lb.py:742:1: W293 blank line contains whitespace
lib/ansible/modules/cloud/amazon/ec2_elb_lb.py:751:1: W293 blank line contains whitespace
lib/ansible/modules/cloud/amazon/ec2_elb_lb.py:755:69: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/ec2_elb_lb.py:757:120: W291 trailing whitespace
lib/ansible/modules/cloud/amazon/ec2_elb_lb.py:758:40: E127 continuation line over-indented for visual indent

click here for bot help

@ansibot ansibot added 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. and removed committer_review In order to be merged, this PR must follow the certified review workflow. labels Feb 10, 2018
@ansibot ansibot added committer_review In order to be merged, this PR must follow the certified 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 Feb 10, 2018
@sivel sivel removed the needs_triage Needs a first human triage before being processed. label Feb 12, 2018
@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 Feb 20, 2018
s-hertel
s-hertel requested changes Feb 21, 2018
View reviewed changes
lib/ansible/modules/cloud/amazon/ec2_elb_lb.py
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?

lib/ansible/modules/cloud/amazon/ec2_elb_lb.py
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'
% 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.

ryansb
ryansb reviewed Feb 21, 2018
View reviewed changes
lib/ansible/modules/cloud/amazon/ec2_elb_lb.py
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.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed committer_review In order to be merged, this PR must follow the certified review workflow. labels Feb 21, 2018
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 2, 2018
@ansibot ansibot added the affects_2.6 This issue/PR affects Ansible v2.6 label May 21, 2018
@ansibot ansibot added support:core This issue/PR relates to code supported by the Ansible Engineering Team. and removed support:certified This issue/PR relates to certified code. labels Sep 18, 2018
@ansibot ansibot removed the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Oct 4, 2018
@ansibot ansibot added needs_maintainer Ansibot is unable to identify maintainers for this PR. (Check `author` in docs or BOTMETA.yml) support:certified This issue/PR relates to certified code. labels Oct 4, 2018
@ansibot ansibot added support:community This issue/PR relates to code supported by the Ansible community. and removed support:certified This issue/PR relates to certified code. labels Oct 12, 2018
@ansibot ansibot removed the needs_maintainer Ansibot is unable to identify maintainers for this PR. (Check `author` in docs or BOTMETA.yml) label Nov 10, 2018
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryansb ryansb ryansb left review comments

@s-hertel s-hertel s-hertel requested changes

Assignees
No one assigned
Labels
affects_2.6 This issue/PR affects Ansible v2.6 aws bug This issue/PR relates to a bug. 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_contributor This PR is the first contribution by a new community member. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@JohnVonNeumann @ansibot @ryansb @s-hertel @sivel