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 473 Add EBS Volume Attributes #210

Closed
wants to merge 4 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
# Set our name if the Name key/value if available
# If we don't get anything back then use the volume id
name = tags.get('Name', volId)

This comment has been minimized.

@Kami

Kami Dec 26, 2013

Member

This now shadows name argument which is passed to this function.

Maybe a better approach would be to use name argument which is passed to the function (if available), otherwise fall back to retrieving a name from the tags (`name = name if name else tags.get('Name', volId)).

@Kami

Kami Dec 26, 2013

Member

This now shadows name argument which is passed to this function.

Maybe a better approach would be to use name argument which is passed to the function (if available), otherwise fall back to retrieving a name from the tags (`name = name if name else tags.get('Name', volId)).

This comment has been minimized.

@Kami

Kami Dec 26, 2013

Member

While you are at it, I would also do the following changes:

  1. Modify _to_volume method signature and default name to None
  2. Modify self._to_volume(el,``) call to omit name argument (empty string) all together
  3. Add a docstring which explains that if not provided, name is obtained from the tags (if available)
@Kami

Kami Dec 26, 2013

Member

While you are at it, I would also do the following changes:

  1. Modify _to_volume method signature and default name to None
  2. Modify self._to_volume(el,``) call to omit name argument (empty string) all together
  3. Add a docstring which explains that if not provided, name is obtained from the tags (if available)

This comment has been minimized.

@cderamus

cderamus Dec 26, 2013

Contributor

Not a problem, will do.

@cderamus

cderamus Dec 26, 2013

Contributor

Not a problem, will do.

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
namespace=NAMESPACE)
# Get our tag items
tag_items = findall(element=element,

This comment has been minimized.

@Kami

Kami Dec 26, 2013

Member

There seem to be multiple places in the file where the tags are read into the dict and exactly the same code is repeated (ex_describe_tags, _to_node, _to_network, etc.)

It would be great if this functionality could be refactored in a separate function and re-used.

@Kami

Kami Dec 26, 2013

Member

There seem to be multiple places in the file where the tags are read into the dict and exactly the same code is repeated (ex_describe_tags, _to_node, _to_network, etc.)

It would be great if this functionality could be refactored in a separate function and re-used.

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
namespace=NAMESPACE)
if value is not None:
# Convert our create/attach time to ISO 8601
if attribute == 'create_time' or attribute == 'attach_time':

This comment has been minimized.

@Kami

Kami Dec 26, 2013

Member

Maybe instead of type, you could call it a format_func / cast_func, specify parse_date as format_func for those two attributes and get rid of this if.

@Kami

Kami Dec 26, 2013

Member

Maybe instead of type, you could call it a format_func / cast_func, specify parse_date as format_func for those two attributes and get rid of this if.

This comment has been minimized.

@cderamus

cderamus Dec 26, 2013

Contributor

Sounds good.

@cderamus

cderamus Dec 26, 2013

Contributor

Sounds good.

# Define and build our extra dictionary
extra = {}
for attribute, values in extra_attributes_map.items():

This comment has been minimized.

@Kami

Kami Dec 26, 2013

Member

This functionality now also appears to be repeated multiple times in this file.

It would be great if you could refactor this into a separate function (e.g. _extract_extra_attributes or something like that) which takes extraction definition (basically, extra_attributes_map argument) and returns a dict with extracted attributes.

@Kami

Kami Dec 26, 2013

Member

This functionality now also appears to be repeated multiple times in this file.

It would be great if you could refactor this into a separate function (e.g. _extract_extra_attributes or something like that) which takes extraction definition (basically, extra_attributes_map argument) and returns a dict with extracted attributes.

This comment has been minimized.

@cderamus

cderamus Dec 26, 2013

Contributor

I was thinking about doing that actually, but wanted to submit it as a separate issue for tracking purposes. Once this PR is closed I'll go ahead and create a JIRA issue for that and go through all calls where the extra_attributes_map exists if you're okay with that approach.

@cderamus

cderamus Dec 26, 2013

Contributor

I was thinking about doing that actually, but wanted to submit it as a separate issue for tracking purposes. Once this PR is closed I'll go ahead and create a JIRA issue for that and go through all calls where the extra_attributes_map exists if you're okay with that approach.

This comment has been minimized.

@Kami

Kami Dec 26, 2013

Member

Yeah, I'm fine with that.

@Kami

Kami Dec 26, 2013

Member

Yeah, I'm fine with that.

Chris DeRamus
Added new function _get_resource_tags to build and return the tags di…
…ctionary

Changed _to_volume to default name to None
Updated the extra_attributes_map to cast_func and removed an unnecessary if condition
@cderamus

This comment has been minimized.

Show comment
Hide comment
@cderamus

cderamus Dec 27, 2013

Contributor

Assuming that you're okay with these changes I will create a new branch later today to convert all extra_attributes_map dictionaries from type -> cast_func to remain consistent and also will set up the new utility function that we previously discussed. Thanks!

Contributor

cderamus commented Dec 27, 2013

Assuming that you're okay with these changes I will create a new branch later today to convert all extra_attributes_map dictionaries from type -> cast_func to remain consistent and also will set up the new utility function that we previously discussed. Thanks!

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Dec 27, 2013

Member

@cderamus Yeah, sounds good.

I've squashed the commits, updated the docstring and merged patch into trunk. Thanks!

Member

Kami commented Dec 27, 2013

@cderamus Yeah, sounds good.

I've squashed the commits, updated the docstring and merged patch into trunk. Thanks!

@cderamus cderamus closed this Dec 27, 2013

@cderamus cderamus deleted the DivvyCloud:LIBCLOUD-473_Add_EBS_Volume_Attributes branch Dec 27, 2013

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