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

Implement create_ and destroy_volume_snapshot for OS #478

Merged
merged 1 commit into from Mar 27, 2015

Conversation

allardhoeve
Copy link
Contributor

@allardhoeve allardhoeve commented Mar 6, 2015

The OS driver currently implements ex_create_snapshot and ex_delete_snapshot instead of create_volume_snapshot and delete_volume_snapshot. This PR fixes that.

  • Add libcloud.compute.drivers.openstack.create_volume_snapshot.
  • Add libcloud.compute.drivers.openstack.destroy_volume_snapshot.
  • Clean up base signature to match current implementations: the name argument is considered optional by all drivers that use the signature.
  • The CloudStack signature was breaking contract by not accepting the name argument.
  • Documented that CloudStack disregards any name given to a new snapshot even though it accepts the argument.
  • Fixed a lot of docstrings.
  • Removes superfluous docstrings that matched the base docstring.

@@ -1611,6 +1611,41 @@ def list_volume_snapshots(self, volume):
return [snapshot for snapshot in self.ex_list_snapshots()
if snapshot.extra['volume_id'] == volume.id]

def create_volume_snapshot(self, volume, name, ex_description=None,
Copy link
Contributor

@mvschaik mvschaik Mar 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is name optional here? Or does OpenStack mandate a name?

Copy link
Contributor Author

@allardhoeve allardhoeve Mar 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, should be optional, like the base driver. Updating in 3, 2, 1.

@sebgoa
Copy link
Member

sebgoa commented Mar 10, 2015

@allardhoeve will you merge this ?

@allardhoeve
Copy link
Contributor Author

allardhoeve commented Mar 10, 2015

If you give the 👍, yes.

@sebgoa
Copy link
Member

sebgoa commented Mar 10, 2015

Seems you got conflicts. Usually I also try to squash commits. Since it touches base let's get @Kami to chime in

@allardhoeve
Copy link
Contributor Author

allardhoeve commented Mar 10, 2015

I prefer to merge trunk into my branch, because that way I only have to deal with recent changes. But I get the feeling you guys prefer linear history? That is: simple git lola trees? Will rebase when it's ready.

@allardhoeve allardhoeve force-pushed the openstack-create-volume-snapshot branch from cf78667 to 6839d64 Compare Mar 10, 2015
@allardhoeve
Copy link
Contributor Author

allardhoeve commented Mar 14, 2015

@Kami, what do you think of the API change?

@@ -15,6 +15,12 @@ General
Compute
~~~~~~~

- Deprecated ex_create_snapshot and ex_delete_snapshot in favor of
create_storage_volume and destroy_storage_volume. Updated base driver
Copy link
Member

@Kami Kami Mar 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should say create_volume_snapshot and destroy_volume_snapshot, right?

Copy link
Member

@Kami Kami Mar 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also say that you deprecated those methods in the OpenStack driver.

@Kami
Copy link
Member

Kami commented Mar 14, 2015

Thanks. Change looks good to me - +1

- Document create_volume_snapshot arguments properly
- Document destroy_volume_snapshot arguments properly
- Update signature to match all existing implementations
  - name should be optional

closes apache#478
@allardhoeve allardhoeve force-pushed the openstack-create-volume-snapshot branch from 9b39dd6 to b81ede3 Compare Mar 27, 2015
@asfgit asfgit merged commit b81ede3 into apache:trunk Mar 27, 2015
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants