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-758] Standardize volumesnapshot state #602

Merged
merged 1 commit into from Nov 3, 2015

Conversation

@allardhoeve
Copy link
Contributor

@allardhoeve allardhoeve commented Oct 15, 2015

No description provided.

@allardhoeve
Copy link
Contributor Author

@allardhoeve allardhoeve commented Oct 21, 2015

anyone? @Kami? otherwise I'll merge 😄

@vdloo
Copy link
Member

@vdloo vdloo commented Oct 21, 2015

would like this as well

@Kami
Copy link
Member

@Kami Kami commented Oct 21, 2015

Sorry, didn't have time to review it yet.
On Oct 21, 2015 11:47 AM, "Allard Hoeve" notifications@github.com wrote:

anyone? @Kami https://github.com/kami? otherwise I'll merge [image:
😄]


Reply to this email directly or view it on GitHub
#602 (comment).

@@ -247,6 +247,18 @@ class StorageVolumeState(object):
UNKNOWN = 8


class VolumeSnapshotState(object):

This comment has been minimized.

@Kami

Kami Nov 1, 2015
Member

In newer parts of the code we decided to replace numeric enum values with string values (e.g. RecordType enum).

Strings makes it easier to see what's going on since you don't need to check the enum type definition to see to which enum name a particular value maps to.

I would appreciate if you can update the code to use string values (e.g. AVAILABLE = 'available', ERROR = ' error', etc.).

In the future I also plan to clean and improve the legacy code and update NodeState to also use string values.

@Kami
Copy link
Member

@Kami Kami commented Nov 1, 2015

Sorry for the delay.

I've added one comment - besides that the code looks good to me. I would appreciate if you can address that and when you do, feel free to merge the PR.

Thanks!

@allardhoeve allardhoeve force-pushed the ByteInternet:standardize-volumesnapshot-state branch 2 times, most recently from 8416356 to 81ce457 Nov 3, 2015
@allardhoeve
Copy link
Contributor Author

@allardhoeve allardhoeve commented Nov 3, 2015

Changed to strings. Will merge.

@allardhoeve allardhoeve force-pushed the ByteInternet:standardize-volumesnapshot-state branch 2 times, most recently from 20550df to 533ded5 Nov 3, 2015
LIBCLOUD-759
closes: #602

Signed-off-by: Allard Hoeve <allard@byte.nl>
@allardhoeve allardhoeve force-pushed the ByteInternet:standardize-volumesnapshot-state branch from 533ded5 to 728bd88 Nov 3, 2015
@asfgit asfgit merged commit 728bd88 into apache:trunk Nov 3, 2015
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@allardhoeve allardhoeve deleted the ByteInternet:standardize-volumesnapshot-state branch Nov 3, 2015
@@ -236,15 +236,27 @@ class StorageVolumeState(object):
"""
Standard states of a StorageVolume
"""
AVAILABLE = "available"

This comment has been minimized.

@Kami

Kami Nov 4, 2015
Member

Sorry, for a late comment, I missed this in the previous pass.

For consistency, it would be great if you can change "INUSE" to "IN_USE" and "BACKUP" to "SNAPSHOTING" (this way it's a verb and it's consistent with other states).

Thanks!

This comment has been minimized.

@allardhoeve

allardhoeve via email Nov 4, 2015
Author Contributor

This comment has been minimized.

@Kami

Kami Nov 4, 2015
Member

True, but I personally find "backup" state a bit confusing (backuping is also better, imo, but snapshotting is more provider agnostic) :)

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

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