module_utils/dimensiondata #17604

relaxdiego · 2016-09-16T00:08:16Z

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

dimensiondata

ANSIBLE VERSION

ansible 2.2.0 (for-upstream-pr bef03042b8) last updated 2016/09/15 12:53:04 (GMT -700)
  lib/ansible/modules/core: (detached HEAD db5cb54b23) last updated 2016/09/08 14:25:22 (GMT -700)
  lib/ansible/modules/extras: (for_devel 0b5e7136aa) last updated 2016/09/15 16:39:58 (GMT -700)
  config file = 
  configured module search path = Default w/o overrides

SUMMARY

This change contains common code required by the dimensiondata modules.

SPECIAL NOTE

Requires apache-libcloud

REQUIRED BY

TBD

bcoca

Just from cursory review, a couple of issues, I only made one comment of each but both are repeated in several spots.

bcoca · 2016-09-20T19:12:31Z

lib/ansible/module_utils/dimensiondata.py

+import sys
+import ConfigParser
+from os.path import expanduser
+from libcloud.common.dimensiondata import API_ENDPOINTS


cannot assume non 'core' modules are in the system, this should attempt to import and then allow for the AnsibleModule class to report about the missing module in a 'friendly' way, see ec2.py for examples.

bcoca · 2016-09-20T19:16:46Z

lib/ansible/module_utils/dimensiondata.py

+    if is_uuid(locator):
+        net_id = locator
+    else:
+        networks = driver.ex_list_network_domains(location=location)


don't all API calls need to capture exceptions?

Exception handling is done in module code. This way we can keep the handling of the error specific to the module using the shared code.

it seems inconsistent as you do capture it in some cases

Yes in some cases the error is always the same so we return False to ease module development. In other cases there are multiple error possibilities from API, some of which we ignore in one case and not another.

If the above is an issue we can make all return the exception and consistently handle them in the module.

+1 on raising a custom exception instead of failing silently.

bcoca · 2016-09-20T19:22:09Z

lib/ansible/module_utils/dimensiondata.py

+    if block_id is not 'False':
+        try:
+            block = driver.ex_get_public_ip_block(block_id)
+        except DimensionDataAPIException:


Note: I'm assuming libcloud requires python >2.6, as such you can use Exception as e, the exc_info is only required for modules that are 2.4 compatible, which most cloud modules cannot be due to library dependencies.

Typically we always use "except DimensionDataAPIException as e". This will be updated to reflect out standard process.

@bcoca @aimonb using Exception as e causes some builds in shippit to fail though.

a) was just note, not required to pass review
b) if libcloud already requries 2.6 we can make an exception in the check as we do for other cloud modules.

I vote for consistency of "except DimensionDataAPIException as e"

Requests are listed in: ansible#17604 (review)

relaxdiego · 2016-09-21T19:48:07Z

Rebased to ansible/devel

Requests are listed in: ansible#17604 (review)

gundalow · 2016-09-27T19:21:25Z

@relaxdiego @mikeoutland This PR was added to ansible/community#124 we have discussed this and as we are now in feature freeze for 2.2 this (and related PRs) will not be reviewed/merged till after Ansible 2.2 has reached rc1 (release candidate 1). At that point we will have branched and allow new PRs to be merged.

abadger · 2016-10-19T18:44:49Z

lib/ansible/module_utils/dimensiondata.py

+#
+# Common methods to be used by versious module components
+import os
+import ConfigParser


This formulation of importing configparser is not compatible with python3. You can use

from ansible.module_utis.six.moves import configparser

Sorry, just checking - do you mean:

from ansible.module_utis.six.moves import configparser

or

from ansible.module_utis.six.moves import ConfigParser

(I've never seen this code before so it's a lot to take in, but it looks like the latter rather than the former?)

BTW, apologies for asking potentially stupid questions, but I'm jumping into this blind - it's been a while since I've written anything fancier than a simple script in Python and I'm a little rusty:

I can find ansible/module_utils/six.py but I'm not sure where moves comes from?

BTW, if I'm importing it as configparser I assume references to the ConfigParser class are still valid (again, sorry if this is a stupid question)?

abadger · 2016-10-19T18:52:45Z

lib/ansible/module_utils/dimensiondata.py

+    region_list = filter(lambda i: i.startswith('dd-'), all_regions)
+
+    # Strip prefix
+    regions = [region[3:] for region in region_list]


List comprehensions and generator expressions are more pythonic than using filter. This one can be rewritten as a single listcomprehension or a combination of as two steps:

# One combined list comprehension regions = [r[3:] for r in all_regions if r.startswith('dd-')] # two steps, using a generator expression region_list = (r for r in all_regions if r.startswith('dd-')) regions = [r[3:] for r in region_list]

abadger · 2016-10-19T18:54:21Z

lib/ansible/module_utils/dimensiondata.py

+    Get a network domain object by its name
+    """
+    networks = driver.ex_list_network_domains(location=location)
+    found_networks = filter(lambda x: x.name == name, networks)


This usage of filter won't work on python3. filter on python3 will return a generator which will evaluate to true in the conditional below. Use a list comprehension so that found_networks ends up with an empty list on python3 which will evaluate to false.

Same change needs to be made to many other uses of filter.

abadger · 2016-10-19T18:56:35Z

lib/ansible/module_utils/dimensiondata.py

+    Get public IP block details
+    """
+    # Block ID given, try to use it.
+    if block_id is not 'False':


I don't think this works? block_id defaults to the boolean False in the parameters to this function. So you probably need to check:

if block_id is not False:

rather than checking against a string.

abadger · 2016-10-19T18:58:32Z

lib/ansible/module_utils/dimensiondata.py

+    else:
+        blocks = list_public_ip_blocks(module, driver, network_domain)
+        if blocks is not False:
+            block = filter(lambda x: x.base_ip == base_ip, blocks)[0]


This will fail on python3 for a similar reason as the conditional check noted above. filter returns a generator. The generator can't be indexed.

Use a list comprehension instead:

block = [b for b in blocks if b.base_ip == base_ip]

Thanks - I've replaced all uses of filter with comprehensions.

abadger · 2016-10-19T19:00:18Z

lib/ansible/module_utils/dimensiondata.py

+            block = filter(lambda x: x.base_ip == base_ip, blocks)[0]
+        else:
+            module.exit_json(changed=False, msg="IP block starting with "
+                             "'%s' does not exist." % base_ip)


the message here looks like a failure case rather than an exit case... Perhaps it should use module.fail_json or the message should be changed?

Stylistically, exit_json() cases should retun back to the main function in the module and let the exit_json() occur there. It's more flexible to changing needs inside of the module than calling exit_json() here.

get_credentials will now look in environment, dotfile, and module configuration for credentials (in that order).

abadger · 2017-01-13T03:43:25Z

lib/ansible/module_utils/dimensiondata.py

-    This method will get user_id and key from module configuration, environment, or dotfile.
-    Environment takes priority.
+    Get user_id and key from module configuration, environment, or dotfile.
+    Order of priority is environment, dotfile, module.


Near as I can tell the order of priority here is module, environment, dotfile. Module should be first, so the coded order is fine but the documentation here should agree with the code.

Sorry, I think the current logic will only fall back to module if environment variable has not been specified: https://github.com/ansible/ansible/pull/17604/files/6c412fbeefa07e4296b33ec7544c5b1a69a6dcd8..85164a272274514877a6369877e851461a5acf0b#diff-0b1c579a3ccbeed5e582e7693e046cb6R112

I'll change the order, though, to put module first. Give me 5 mins.

abadger · 2017-01-13T03:44:17Z

@tintoy. Looks good. Just a disagreement between the documentation and the code to straighten out. Once that's fixed, this is ready to merge.

…environment or dotfile (ansible#17604).

tintoy · 2017-01-13T04:10:25Z

Thanks again for your patience, BTW.

abadger · 2017-01-13T04:51:02Z

Merged to devel! thanks for your patience as well :-)

tintoy · 2017-01-13T05:55:53Z

Great! I'll start looking at the first PR that depends on this :)

relaxdiego mentioned this pull request Sep 16, 2016

New Module: Dimonsion Data cloud modules ansible/ansible-modules-extras#2934

Closed

relaxdiego force-pushed the devel branch from bef0304 to de82717 Compare September 16, 2016 00:55

alikins added feature_pull_request new_plugin This PR includes a new plugin. labels Sep 19, 2016

relaxdiego force-pushed the devel branch from de82717 to db382c4 Compare September 19, 2016 22:07

mikeoutland mentioned this pull request Sep 20, 2016

Agenda for Public Core Meeting - 2016-09-20 19:00 UTC ansible/community#124

Closed

bcoca requested changes Sep 20, 2016

View reviewed changes

relaxdiego force-pushed the devel branch from db382c4 to 156a5cf Compare September 20, 2016 21:18

relaxdiego force-pushed the devel branch from 156a5cf to ece2098 Compare September 21, 2016 19:47

relaxdiego added a commit to DimensionDataResearch/ansible that referenced this pull request Sep 21, 2016

Implement change requests from PR ansible#17604

ece2098

Requests are listed in: ansible#17604 (review)

relaxdiego added a commit to relaxdiego/ansible that referenced this pull request Sep 21, 2016

Implement change requests from PR ansible#17604

0bec939

Requests are listed in: ansible#17604 (review)

jr2730 mentioned this pull request Sep 22, 2016

New dimensiondata backup ansible/ansible-modules-extras#3012

Closed

relaxdiego mentioned this pull request Sep 29, 2016

Tests for dimensiondata_load_balancer module #17802

Closed

abadger reviewed Oct 19, 2016

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 2, 2017

jimi-c removed the plugin label Jan 4, 2017

ansibot added module_util needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 5, 2017

jimi-c added c:module_utils/ and removed c:module_utils/ module_util labels Jan 5, 2017

ansibot added module_util new_module This PR includes a new module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 5, 2017

Unify get_credentials implementation (ansible#17604).

85164a2

get_credentials will now look in environment, dotfile, and module configuration for credentials (in that order).

abadger reviewed Jan 13, 2017

View reviewed changes

Resolve user Id and password from module configuration before trying …

b00b6df

…environment or dotfile (ansible#17604).

abadger merged commit b598575 into ansible:devel Jan 13, 2017

tintoy mentioned this pull request Jan 16, 2017

New dimensiondata vlan #20321

Closed

tintoy mentioned this pull request Jan 23, 2017

New dimensiondata firewall #20583

Closed

ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 4, 2018

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module_utils/dimensiondata #17604

module_utils/dimensiondata #17604

relaxdiego commented Sep 16, 2016 •

edited

bcoca left a comment

bcoca Sep 20, 2016

bcoca Sep 20, 2016 •

edited

aimonb Sep 20, 2016

bcoca Sep 20, 2016

aimonb Sep 20, 2016

aimonb Sep 20, 2016

relaxdiego Sep 20, 2016

bcoca Sep 20, 2016 •

edited

aimonb Sep 20, 2016

relaxdiego Sep 20, 2016 •

edited

bcoca Sep 20, 2016

aimonb Sep 20, 2016

relaxdiego commented Sep 21, 2016

gundalow commented Sep 27, 2016

abadger Oct 19, 2016

tintoy Dec 13, 2016

tintoy Dec 13, 2016

tintoy Dec 13, 2016

abadger Oct 19, 2016

abadger Oct 19, 2016

abadger Oct 19, 2016

abadger Oct 19, 2016

abadger Oct 19, 2016

tintoy Dec 13, 2016

abadger Oct 19, 2016

abadger Jan 13, 2017

tintoy Jan 13, 2017

tintoy Jan 13, 2017

abadger commented Jan 13, 2017

tintoy commented Jan 13, 2017

abadger commented Jan 13, 2017

tintoy commented Jan 13, 2017

module_utils/dimensiondata #17604

module_utils/dimensiondata #17604

Conversation

relaxdiego commented Sep 16, 2016 • edited

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

SUMMARY

SPECIAL NOTE

REQUIRED BY

bcoca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoca Sep 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoca Sep 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relaxdiego Sep 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relaxdiego commented Sep 21, 2016

gundalow commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abadger commented Jan 13, 2017

tintoy commented Jan 13, 2017

abadger commented Jan 13, 2017

tintoy commented Jan 13, 2017

relaxdiego commented Sep 16, 2016 •

edited

bcoca Sep 20, 2016 •

edited

bcoca Sep 20, 2016 •

edited

relaxdiego Sep 20, 2016 •

edited