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

Add VolumeSnapshot.created attribute #473

Conversation

allardhoeve
Copy link
Contributor

@allardhoeve allardhoeve commented Mar 3, 2015

One very important aspect of snapshots is when they are made, especially when used as historical backups. The field in VolumeSnapshot.extra that is used to represent this data varies between OS, EC2, GCE and Rackspace. Also, EC2 boasts a datetime value in extra, while OpenStack and derivatives use a plain string.

This PR adds a created attribute to VolumeSnapshot, that is a parsed date. This unified all implementations.

driver = None
size = None
extra = None
created = None
Copy link
Contributor Author

@allardhoeve allardhoeve Mar 3, 2015

Choose a reason for hiding this comment

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

Added to aid introspection in all sorts of IDEs and ipython.

Copy link
Member

@Kami Kami Mar 4, 2015

Choose a reason for hiding this comment

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

The behavior is not exactly the same. Previously those variables were only instance variables, now they are also class variables which could have undesired consequences in some cases (https://stackoverflow.com/questions/207000/python-difference-between-class-and-instance-attributes).

I would prefer to keep it as it was. As far as introspection goes -I need to dig into it, but at least as far as helping with types go - you can help IDE's by adding type asserts and docstring annotation (we already do the latter in most places).

Copy link
Contributor Author

@allardhoeve allardhoeve Mar 5, 2015

Choose a reason for hiding this comment

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

@allardhoeve
Copy link
Contributor Author

allardhoeve commented Mar 3, 2015

@Kami

@allardhoeve
Copy link
Contributor Author

allardhoeve commented Mar 3, 2015

  • CloudFrames implements a CloudFramesSnapshot, which has nothing to do with a VolumeSnapshot 😢

@@ -39,7 +40,7 @@
from libcloud.test.file_fixtures import ComputeFileFixtures


class CloudStackCommonTestCase(TestCaseMixin):
class CloudStackCommonTestCase(unittest.TestCase, TestCaseMixin):
Copy link
Contributor Author

@allardhoeve allardhoeve Mar 3, 2015

Choose a reason for hiding this comment

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

To aid introspection in PyCharm.

@allardhoeve allardhoeve force-pushed the uniform-snapshot-creation-attribute branch 2 times, most recently from 23bace9 to 1ddf58f Compare Mar 4, 2015
@allardhoeve allardhoeve force-pushed the uniform-snapshot-creation-attribute branch from 1ddf58f to ad31155 Compare Mar 4, 2015
@Kami
Copy link
Member

Kami commented Mar 4, 2015

Besides the class variables change, this PR looks good to me.

@allardhoeve
Copy link
Contributor Author

allardhoeve commented Mar 5, 2015

Removed instance variables and merged trunk into the branch.

@asfgit asfgit closed this in 373059d Mar 7, 2015
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

2 participants