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

LIBCLOUD-753 MCP 2 coverage dimension data #593

Closed
wants to merge 15 commits into
base: trunk
from

Conversation

Projects
None yet
4 participants
@tonybaloney
Contributor

tonybaloney commented Oct 6, 2015

Extended methods for additional functions available in the API.

Show outdated Hide outdated libcloud/common/dimensiondata.py
DimensionData Public IP Block with location.
"""
def __init__(self, id, baseIp, size, location, network_domain,

This comment has been minimized.

@Kami

Kami Oct 7, 2015

Member

Minor thing - for consistency please use base_ip instead of baseIp.

@Kami

Kami Oct 7, 2015

Member

Minor thing - for consistency please use base_ip instead of baseIp.

Show outdated Hide outdated libcloud/common/dimensiondata.py
"""
def __init__(self, id, name, action, location, network_domain,
status, ipVersion, protocol, source, destination,

This comment has been minimized.

@Kami

Kami Oct 7, 2015

Member

Same here - ipVersion -> ip_version.

@Kami

Kami Oct 7, 2015

Member

Same here - ipVersion -> ip_version.

class DimensionDataVlan(object):
"""
DimensionData VLAN.
"""
def __init__(self, id, name, description, location, status):
def __init__(self, id, name, description, location, status,
private_ipv4_range_address, private_ipv4_range_size):

This comment has been minimized.

@Kami

Kami Oct 7, 2015

Member

Is private_ipv4_range_size CIDR suffix (e.g. "32" part of ip/32 notation) or something else?

In any case, it would be good to add a method docstring which at least documents and explain the arguments which are a bit less self-explanatory.

@Kami

Kami Oct 7, 2015

Member

Is private_ipv4_range_size CIDR suffix (e.g. "32" part of ip/32 notation) or something else?

In any case, it would be good to add a method docstring which at least documents and explain the arguments which are a bit less self-explanatory.

Show outdated Hide outdated libcloud/compute/drivers/dimensiondata.py
result = findtext(body, 'responseCode', TYPES_URN)
return result == 'IN_PROGRESS'
responseCode = findtext(body, 'responseCode', TYPES_URN)
return responseCode == 'IN_PROGRESS' or responseCode == 'OK'

This comment has been minimized.

@Kami

Kami Oct 7, 2015

Member

Minor style thing - you could also do return responseCode in ['IN_PROGRESS', 'OK']

Also, for consistency, please use _ notation for variable names (response_code) :)

@Kami

Kami Oct 7, 2015

Member

Minor style thing - you could also do return responseCode in ['IN_PROGRESS', 'OK']

Also, for consistency, please use _ notation for variable names (response_code) :)

Show outdated Hide outdated libcloud/compute/drivers/dimensiondata.py
result = findtext(body, 'responseCode', TYPES_URN)
return result == 'IN_PROGRESS'
responseCode = findtext(body, 'responseCode', TYPES_URN)
return responseCode == 'IN_PROGRESS' or responseCode == 'OK'

This comment has been minimized.

@Kami

Kami Oct 7, 2015

Member

It seems like the same check is repeated all over the driver so one thing you could do is define a module / class level constant for response codes which indicate success and a utility method which performs this check, e.g. return self._operation_successful(response_object).

@Kami

Kami Oct 7, 2015

Member

It seems like the same check is repeated all over the driver so one thing you could do is define a module / class level constant for response codes which indicate success and a utility method which performs this check, e.g. return self._operation_successful(response_object).

network_domain_id = None
for info in findall(response, 'info', TYPES_URN):

This comment has been minimized.

@Kami

Kami Oct 7, 2015

Member

I believe you could also use a more complex xpath expression to directly match the networkDomainID attribute to avoid for loop, but I'm also fine with leaving it as is.

@Kami

Kami Oct 7, 2015

Member

I believe you could also use a more complex xpath expression to directly match the networkDomainID attribute to avoid for loop, but I'm also fine with leaving it as is.

Show outdated Hide outdated libcloud/compute/drivers/dimensiondata.py
network_domain_id = None
for info in findall(response, 'info', TYPES_URN):
if info.get('name') == 'networkDomainId"':

This comment has been minimized.

@Kami

Kami Oct 7, 2015

Member

Is " at the end of the attribute name intentional?

@Kami

Kami Oct 7, 2015

Member

Is " at the end of the attribute name intentional?

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Oct 7, 2015

Member

Great, thanks.

I've added some comments.

Member

Kami commented Oct 7, 2015

Great, thanks.

I've added some comments.

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Oct 7, 2015

Contributor

All fixed. going to work through the doc strings next week hopefully to properly document how each method should be used

Contributor

tonybaloney commented Oct 7, 2015

All fixed. going to work through the doc strings next week hopefully to properly document how each method should be used

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Oct 7, 2015

Contributor

Oops. broken 22 unit tests. luckily it's at 92% coverage!

Contributor

tonybaloney commented Oct 7, 2015

Oops. broken 22 unit tests. luckily it's at 92% coverage!

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Oct 8, 2015

Contributor

done and fixed

Contributor

tonybaloney commented Oct 8, 2015

done and fixed

@@ -355,11 +472,14 @@ def __repr__(self):
class DimensionDataVIPNode(object):
def __init__(self, id, name, status, ip):
def __init__(self, id, name, status, ip, connection_limit='10000',

This comment has been minimized.

@Kami

Kami Oct 8, 2015

Member

It looks like the limit values are string and not integers? Or just the default values are strings.

In any case, would be better if they were integers.

@Kami

Kami Oct 8, 2015

Member

It looks like the limit values are string and not integers? Or just the default values are strings.

In any case, would be better if they were integers.

@asfgit asfgit closed this in 44c4cd6 Oct 8, 2015

asfgit pushed a commit that referenced this pull request Oct 8, 2015

asfgit pushed a commit that referenced this pull request Oct 8, 2015

Completed functions for firewall rules in network domains, get, list,…
… update (disable/enable), create and delete. Inc unit tests

Closes #593

asfgit pushed a commit that referenced this pull request Oct 8, 2015

asfgit pushed a commit that referenced this pull request Oct 8, 2015

asfgit pushed a commit that referenced this pull request Oct 8, 2015

asfgit pushed a commit that referenced this pull request Oct 8, 2015

asfgit pushed a commit that referenced this pull request Oct 8, 2015

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Oct 8, 2015

Member

Merged, thanks.

Member

Kami commented Oct 8, 2015

Merged, thanks.

return response_code in ['IN_PROGRESS', 'OK']
def ex_delete_firewall_rule(self, rule):
update_node = ET.Element('deleteFirewallrule', {'xmlns': TYPES_URN})

This comment has been minimized.

@EfrenRey

EfrenRey Oct 16, 2015

@tonybaloney I think there is a typo, the XML element name is deleteFirewallRule

@EfrenRey

EfrenRey Oct 16, 2015

@tonybaloney I think there is a typo, the XML element name is deleteFirewallRule

return self._to_nat_rule(rule, network_domain)
def ex_delete_nat_rule(self, rule):
update_node = ET.Element('deleteNatrule', {'xmlns': TYPES_URN})

This comment has been minimized.

@EfrenRey

EfrenRey Oct 16, 2015

@tonybaloney I think there is a typo, the XML element name is deleteNatRule

@EfrenRey

EfrenRey Oct 16, 2015

@tonybaloney I think there is a typo, the XML element name is deleteNatRule

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Oct 18, 2015

Contributor

thanks @EfrenRey the PR is closed but I will fix these issues separately. good catch. these methods are really repetitive!

Contributor

tonybaloney commented Oct 18, 2015

thanks @EfrenRey the PR is closed but I will fix these issues separately. good catch. these methods are really repetitive!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment