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

Libcloud 469 add reserved instance support #205

Closed
wants to merge 3 commits into from

Conversation

@cderamus
Copy link
Contributor

cderamus commented Dec 22, 2013

No description provided.

@@ -559,6 +559,22 @@ def __repr__(self):
% (self.name, self.zone_state, self.region_name))


class ExEC2ReservedNode(object):

This comment has been minimized.

Copy link
@Kami

Kami Dec 22, 2013

Member

You can drop the Ex prefix. I know one class in EC2 driver uses it, but that's mostly an artifact from the past.

Nowadays we only use ex prefix for methods and arguments.

This comment has been minimized.

Copy link
@Kami

Kami Dec 22, 2013

Member

This class could potentially inherit from Node and you can then just pass None for public_ips and private_ips argument, right?

Adding docstrings for attributes which are not already present on the Node class would also be good (just by looking at the constructor, I don't know what the type attribute represents).

state=findattr(element=element,
xpath="state",
namespace=NAMESPACE),
driver=self.connection.driver,

This comment has been minimized.

Copy link
@Kami

Kami Dec 22, 2013

Member

driver=self is shorter :)

xpath='instanceType',
namespace=NAMESPACE),
state=findattr(element=element,
xpath="state",

This comment has been minimized.

Copy link
@Kami

Kami Dec 22, 2013

Member

For consistency, please use single quotes around strings everywhere except for docstrings.

'duration': findattr(element=element,
xpath="duration",
namespace=NAMESPACE),
'usage_price': findattr(element=element,

This comment has been minimized.

Copy link
@Kami

Kami Dec 22, 2013

Member

This attribute should probably be float, right?

'fixed_price': findattr(element=element,
xpath="fixedPrice",
namespace=NAMESPACE),
'instance_count': findattr(element=element,

This comment has been minimized.

Copy link
@Kami

Kami Dec 22, 2013

Member

int? :)

xpath="state",
namespace=NAMESPACE),
driver=self.connection.driver,
extra={'availability': findattr(element=element,

This comment has been minimized.

Copy link
@Kami

Kami Dec 22, 2013

Member

To get rid of a bunch of lines and making it easier to add a new attributes, you could use a for loop.

So, something like:

extra_attributes_map = {
    'availability': {
        'xpath:' 'availabilityZone',
        'type': str
    }
}

for attribute, values in extra_attributes_map.items():
   type_func = values['type']
   name = values['name']
   value = findattr(element=element, xpath=values['xpath'],
                               namespace=NAMESPACE)
   extra[attribute] = cast_func(value)
   ...
Updated EC2ReservedNode to use the extra_attribute_map to allow for easier modifications to the extra dictionary
Updated tests to reflect the class changes
@cderamus
Copy link
Contributor Author

cderamus commented Dec 23, 2013

I went ahead and made the requested changes. To inherit from Node I also had to set the name argument to None as reserved instances do not have names or tags where we can pull the Name key/value pair.

@Kami
Copy link
Member

Kami commented Dec 23, 2013

Thanks!

The PR looks mostly good, but I've made some minor changes after merging the patch:

  1. I've modified EC2ReservedNode constructor to call parent constructor - 49e2f3b
  2. I've removed unused arguments from the EC2ReservedNode constructor method - 49e2f3b
  3. I've also passed size attribute to the EC2ReservedNode constructor. We can do this because list_sizes call is cheap (it doesn't result in HTTP request since sizes are stored locally in the module). - 11abec4
  4. I've moved test from NimbusTests to EC2Tests class since this functionality is EC2 specific. I imagine you just looked for list_nodes tests and you accidentally put list_reserved_nodes test above it. - 1655e85
  5. I've renamed list_reserved_nodes method to ex_list_reserved_nodes. Sorry if I've confused you with "you can drop the ex prefix from the class name" comment. We still use ex_ prefix, but only for method and argument names.
@cderamus
Copy link
Contributor Author

cderamus commented Dec 23, 2013

Ah, thanks for that and good call on the list_sizes.

@cderamus cderamus closed this Dec 23, 2013
@cderamus cderamus deleted the DivvyCloud:LIBCLOUD-469_Add_Reserved_Instance_Support branch Dec 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.