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-775 Update node, update VMware tools, add storage, change st… #644

Conversation

@tonybaloney
Copy link
Contributor

@tonybaloney tonybaloney commented Nov 30, 2015

…orage size or speed, remove storage. With tests

…orage size or speed, remove storage. With tests

def ex_change_storage_speed(self, node, disk_id, speed):
"""
Remove storage from a node

This comment has been minimized.

@Kami

Kami Dec 2, 2015
Member

I believe this comment needs to be updated :)


def ex_change_storage_size(self, node, disk_id, size):
"""
Remove storage from a node

This comment has been minimized.

@Kami

Kami Dec 2, 2015
Member

Same here.

'server/%s?clone=%s&desc=%s' %
(node.id, image_name, image_description)).object
response_code = findtext(result, 'result', GENERAL_NS)
return response_code in ['IN_PROGRESS', 'SUCCESS']

This comment has been minimized.

@Kami

Kami Dec 2, 2015
Member

Noticed all those API are async - eventually it might be handy to provide a "sync" interface in libcloud (e.g. this way you could potentially return a NodeImage object from this method).

if image_description is None:
image_description = ''
result = self.connection.request_with_orgId_api_1(
'server/%s?clone=%s&desc=%s' %

This comment has been minimized.

@Kami

Kami Dec 2, 2015
Member

Eventually it would also be good to refactor request_with_orgId_api_1 to take body params as a dict and urlencode it in the request_with_orgId_api_1 method.

This way it would be safer and more robust since right now special characters (e.g. & inside the description) could break the manual encoding...

@Kami
Copy link
Member

@Kami Kami commented Dec 2, 2015

Added some comments for future improvements (not a blocker for this PR, you can address them in a future PR).

Besides that, LGTM, +1.

Please also make sure you update changelog.

@tonybaloney
Copy link
Contributor Author

@tonybaloney tonybaloney commented Dec 2, 2015

Will update CHANGES.rst separately since I had a couple of other closed PRs anyway.

@asfgit asfgit closed this in 36ed630 Dec 2, 2015
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

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