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-649] Add new methods for LinodeNodeDriver #430

Closed
wants to merge 7 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@wirkijowski
Contributor

wirkijowski commented Jan 9, 2015

Unit tests included
Jira ticket: https://issues.apache.org/jira/browse/LIBCLOUD-649

wirkijowski added some commits Nov 9, 2014

Experimental create and list volumes for linode
It's to be rewritten to comply with libcloud design
Add new methods for the Linode driver
* ex_create_volume() for adding volumes for the Linode
* ex_list_volumes() for listing volumes for the Linode
* LINODE_DISK_FILESYSTEMS
* updated _linode_disk_list.json

Includes tests for new methods
Add destroy_volume() method for Linode driver
* destroy_volume() method for Linode
* basic test for destroy_volume()
Minor refactoring of destroy_volume() method for Linode
* refactoring of destroy_volume()
* minor fixes for coding style

@wirkijowski wirkijowski changed the title from Libcloud 649 basic volume management linode to [LIBCLOUD-649] Add create, list, destroy methods for LinodeNodeDriver Jan 9, 2015

@wirkijowski wirkijowski changed the title from [LIBCLOUD-649] Add create, list, destroy methods for LinodeNodeDriver to [LIBCLOUD-649] Add new methods for LinodeNodeDriver Jan 16, 2015

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jan 17, 2015

Member

Thanks.

I quickly glanced over the changes and they look good to me.

I'm just wondering, if we can make "ex_create_volume" and "ex_list_volume" methods some how comply with the standard API. Looking at the Linode API docs, LinodeId argument is required so sadly this might not be possible.

Member

Kami commented Jan 17, 2015

Thanks.

I quickly glanced over the changes and they look good to me.

I'm just wondering, if we can make "ex_create_volume" and "ex_list_volume" methods some how comply with the standard API. Looking at the Linode API docs, LinodeId argument is required so sadly this might not be possible.

Show outdated Hide outdated libcloud/compute/drivers/linode.py Outdated
@wirkijowski

This comment has been minimized.

Show comment
Hide comment
@wirkijowski

wirkijowski Jan 17, 2015

Contributor

In order to comply with standart libcloud API, there could be create_volume() method. There are two ways it could be done.
The first one is create_volume() could return StorageVolume instance which would be created for real after attach_volume(). But this would need that programmer should remeber that they operate on nonexistent volume until it's attached.
The second way is to use create_volume() with additional ex_ params. But this params need to be required.

ex_list_volumes() could be rewritten to list_volumes() with optional ex_ params, definig node or disk_id.

Contributor

wirkijowski commented Jan 17, 2015

In order to comply with standart libcloud API, there could be create_volume() method. There are two ways it could be done.
The first one is create_volume() could return StorageVolume instance which would be created for real after attach_volume(). But this would need that programmer should remeber that they operate on nonexistent volume until it's attached.
The second way is to use create_volume() with additional ex_ params. But this params need to be required.

ex_list_volumes() could be rewritten to list_volumes() with optional ex_ params, definig node or disk_id.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Feb 21, 2015

Member

Sorry for the delay.

It sounds like we should just stay with extension methods for now since none of the approaches makes it fully compliant with the base API, in fact, it might just make it more confusing to the end user (it's named as the base API, but it doesn't behave as such).

Member

Kami commented Feb 21, 2015

Sorry for the delay.

It sounds like we should just stay with extension methods for now since none of the approaches makes it fully compliant with the base API, in fact, it might just make it more confusing to the end user (it's named as the base API, but it doesn't behave as such).

@asfgit asfgit closed this in d9efecd Feb 21, 2015

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Feb 21, 2015

Member

Squashed the commits and merged changes into trunk. Thanks.

Member

Kami commented Feb 21, 2015

Squashed the commits and merged changes into trunk. Thanks.

@wirkijowski

This comment has been minimized.

Show comment
Hide comment
@wirkijowski

wirkijowski Feb 22, 2015

Contributor

Thanks:)

Contributor

wirkijowski commented Feb 22, 2015

Thanks:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment