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

Turn all States into string values instead of ints #624

Merged
merged 1 commit into from Dec 10, 2015

Conversation

@allardhoeve
Copy link
Contributor

@allardhoeve allardhoeve commented Nov 3, 2015

@Kami

Luckily, most tests already used the constants. I found a couple sloppy drivers, especially the ProfitBricks driver, which uses NodeState for everything, including Volumes and Snapshots. Maybe we fix that, maybe we leave it. It doesn't really matter.

@Kami
Copy link
Member

@Kami Kami commented Nov 3, 2015

Thanks.

We should just wait with merging this until we decide when to include it in a release since it's a breaking change if people used integers directly.

I'm fine with including it in 0.20.0 (next "major" release), but we also need to update documentation (upgrade notes) to reflect this change.

ERROR = 7
PAUSED = 8
RUNNING = "running"
REBOOTING = "rebooting"

This comment has been minimized.

@Kami

Kami Nov 3, 2015
Member

Minor style thing - for consistency, please use single quotes around simple strings :)

for item_key, item_value in values.items():
if value == item_value:
return item_key
return value

@classmethod
def fromstring(cls, value):

This comment has been minimized.

@Kami

Kami Nov 3, 2015
Member

Is this still needed?

This comment has been minimized.

@Kami

Kami Nov 3, 2015
Member

Would also be good to add some small test case for tostring and fromstring.

This comment has been minimized.

@jimbobhickville

jimbobhickville Dec 2, 2015
Contributor

Yes, please leave this in. We have a lot of code that uses these method to convert to/from the libcloud expected values, and removing this will break us.

This comment has been minimized.

@allardhoeve

allardhoeve Dec 10, 2015
Author Contributor

I will create tests for this

This comment has been minimized.

@allardhoeve

allardhoeve Dec 10, 2015
Author Contributor

Mmmm, there are no current tests for this 😢

SUSPENDED = 6
ERROR = 7
PAUSED = 8
RUNNING = "running"

This comment has been minimized.

@jimbobhickville

jimbobhickville Dec 2, 2015
Contributor

These were ALL_CAPS before, I don't think we should change the case on them.

@allardhoeve allardhoeve force-pushed the ByteInternet:all-states-are-string branch from 6518c92 to 0428067 Dec 10, 2015
@allardhoeve
Copy link
Contributor Author

@allardhoeve allardhoeve commented Dec 10, 2015

So I added tests for all the objects that have strings as values now. All tests pass.

@Kami: I'd like for this to go into 0.20. Do you think that is still possible?

@allardhoeve
Copy link
Contributor Author

@allardhoeve allardhoeve commented Dec 10, 2015

Also, I think the changes involved are minimal. I cannot imagine a use case where you would actually use the integers in normal code. I think everyone involved either used fromstring for NodeState and Provider, or raw strings, because the other objects did not have consistent states anyway.



class StorageVolumeState(object):
RUNNING = 'running'

This comment has been minimized.

@jimbobhickville

jimbobhickville Dec 10, 2015
Contributor

Are we ok with this behavior change? The NodeState.tostring(node.state) values previously were all-caps, now they're all lower-case. I know this means my team's codebase will require changes to keep existing behavior. Could we make Type.tostring return value.upper()?

This comment has been minimized.

@allardhoeve

allardhoeve Dec 10, 2015
Author Contributor

Ah! That is true. Let me add that test. We should've had tests for this stuff all along 😢 That would've made this much easier. Ah well, moving forward 😄

This comment has been minimized.

@allardhoeve

allardhoeve Dec 10, 2015
Author Contributor

Done

@jimbobhickville
Copy link
Contributor

@jimbobhickville jimbobhickville commented Dec 10, 2015

Thanks @allardhoeve - lgtm

@allardhoeve allardhoeve force-pushed the ByteInternet:all-states-are-string branch from 734ae6d to b2f17f7 Dec 10, 2015
@allardhoeve
Copy link
Contributor Author

@allardhoeve allardhoeve commented Dec 10, 2015

I added docs for 0.20

Allard Hoeve
- Let all types derive from Type
- Turn all type attributes into strings
- Keep things backwards-compatible by having
  - tostring
  - fromstring
- Add tests for tostring, fromstring

closes #624

Signed-off-by: Allard Hoeve <allardhoeve@gmail.com>
@allardhoeve allardhoeve force-pushed the ByteInternet:all-states-are-string branch from 7c6fce3 to bef1f94 Dec 10, 2015
@asfgit asfgit merged commit bef1f94 into apache:trunk Dec 10, 2015
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@allardhoeve allardhoeve deleted the ByteInternet:all-states-are-string branch Dec 10, 2015
* All NodeState, StorageVolumeState, VolumeSnapshotState and Provider attributes
are now strings instead of integers.

If you are using the ``tostring`` and ``fromstring`` methods of NodeState,

This comment has been minimized.

@Kami

Kami Dec 10, 2015
Member

It would also be good to note that you are affected if you directly store the enum value in the database (integer) - I know some users who do that.

For that scenario this is a backward incompatible change. We don't have any automatic migration path for it - users need to handle that in their code - so for users who do that it's quite an invasive change.

:return: the uppercase string that represents the state object
:rtype: str
"""
return value.upper()

This comment has been minimized.

@Kami

Kami Dec 10, 2015
Member

I think I'm fine with this change, but since it's a class method it might also also be a good idea to throw if users passes in an invalid enum value.

In any case, current approach works for now.

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.