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 created datetime to node #698

Merged
merged 1 commit into from Feb 12, 2016

Conversation

@allardhoeve
Copy link
Contributor

allardhoeve commented Feb 10, 2016

Instead of the implementation in #697, I'd like to propose this instead.

The problem with #697 is that when you want to find a list of old nodes when you have a list of Node objects, you'd need to write:

old_nodes = [n for n in nodes if n.driver.ex_creation_time(n) > too_old]

Yuk, why is that driver in there? Better would be to write:

old_nodes = [n for n in nodes if n.created > too_old]

I have implemented this code for OpenStack, Digital Ocean and EC2. Other drivers, I have no access to. In those cases, the behaviour is to have created be NoneType.

@allardhoeve
Copy link
Contributor Author

allardhoeve commented Feb 10, 2016

@tonybaloney
Copy link
Contributor

tonybaloney commented Feb 11, 2016

this is a good addition to the base class. I agree with you @allardhoeve it is a good design. 👍

@@ -167,7 +167,7 @@ class Node(UuidMixin):
"""

def __init__(self, id, name, state, public_ips, private_ips,
driver, size=None, image=None, extra=None):
driver, size=None, image=None, created=None, extra=None):

This comment has been minimized.

@Kami

Kami Feb 11, 2016 Member

I would prefer the attribute to be called created_at.

@Kami
Copy link
Member

Kami commented Feb 11, 2016

It's also worth noting that this is a backward incompatible change for any place which doesn't use keyword arguments so we need to make sure it doesn't break any of the existing drivers and document it in the upgrade notes.

@allardhoeve
Copy link
Contributor Author

allardhoeve commented Feb 11, 2016

I could put the new keyword after the extra=None, that way it breaks nothing. But yes, it should be in the Changelog

@vdloo
Copy link
Member

vdloo commented Feb 11, 2016

I agree, this is better than using the driver and passing the node object in 👍

@allardhoeve
Copy link
Contributor Author

allardhoeve commented Feb 12, 2016

Okey, I changed the field name to created_at and the order of the named arguments. If everyone is on board, I'd like to commit this and revert #697.

@tonybaloney
Copy link
Contributor

tonybaloney commented Feb 12, 2016

👍 from me

* Base Node object has `created_at` which indicates the `datetime` the
  node was launched/started/created.
* EC2, Digital Ocean, OpenStack fill this attribute.
* Nodes at drivers that do not (yet) support it have `NoneType` as date.
* Document changes.

closes #698

[GITHUB-698]

Signed-off-by: Allard Hoeve <allardhoeve@gmail.com>
@allardhoeve allardhoeve force-pushed the ByteInternet:add-created-datetime-to-node branch from cb7e67f to 72399b4 Feb 12, 2016
@asfgit asfgit merged commit 72399b4 into apache:trunk Feb 12, 2016
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:add-created-datetime-to-node branch Feb 12, 2016
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

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