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-802] Add drivers for Aliyun cloud #712

Closed
wants to merge 3 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Feb 23, 2016

Member

First thanks for the contribution! We will look into it as soon as possible.

In the mean time, since this is a larger contribution, please make sure you file an ICLA with Apache - https://libcloud.readthedocs.org/en/latest/development.html#contributing-bigger-changes and please let us know when you have done so.

Member

Kami commented Feb 23, 2016

First thanks for the contribution! We will look into it as soon as possible.

In the mean time, since this is a larger contribution, please make sure you file an ICLA with Apache - https://libcloud.readthedocs.org/en/latest/development.html#contributing-bigger-changes and please let us know when you have done so.

@samsong8610

This comment has been minimized.

Show comment
Hide comment
@samsong8610

samsong8610 Feb 26, 2016

Contributor

@Kami I have signed icla and email the pdf to secretary@apache.org.
BTW, I fixed the failed test, but FORCE pushed it to my fork branch. How to trigger the ci to test it again?

Contributor

samsong8610 commented Feb 26, 2016

@Kami I have signed icla and email the pdf to secretary@apache.org.
BTW, I fixed the failed test, but FORCE pushed it to my fork branch. How to trigger the ci to test it again?

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Feb 26, 2016

Member

@samsong8610 Great, thanks!

It seems like that build was already triggered on force push (https://travis-ci.org/apache/libcloud - last build was started an hour ago).

It looks like there are still same unicode related issues in Python 3.2, I will look into it later.

Member

Kami commented Feb 26, 2016

@samsong8610 Great, thanks!

It seems like that build was already triggered on force push (https://travis-ci.org/apache/libcloud - last build was started an hour ago).

It looks like there are still same unicode related issues in Python 3.2, I will look into it later.

Show outdated Hide outdated libcloud/test/storage/test_oss.py
class ObjectTestCase(unittest.TestCase):
def test_object_with_chinese_name(self):
driver = OSSStorageDriver(*STORAGE_OSS_PARAMS)
obj = Object(name=u'中文', size=0, hash=None, extra=None,

This comment has been minimized.

@Kami

Kami Feb 26, 2016

Member

Changing this to use libcloud.utils.py3.u wrapper function should fix the test failure under Python 3.2.

So:

from libcloud.utils.py3 import u
...

obj = Object(name=u('中文'), size=0, hash=None, extra=None,
...
@Kami

Kami Feb 26, 2016

Member

Changing this to use libcloud.utils.py3.u wrapper function should fix the test failure under Python 3.2.

So:

from libcloud.utils.py3 import u
...

obj = Object(name=u('中文'), size=0, hash=None, extra=None,
...

This comment has been minimized.

@samsong8610

samsong8610 Feb 27, 2016

Contributor

Thanks for your help. But py3.u wrapper function doesn't work when I compare an unicode literal to a variable which stores an unicode string.
I fixed the failed test cases according to Stackoverflow. Is it acceptable in libcloud?

@samsong8610

samsong8610 Feb 27, 2016

Contributor

Thanks for your help. But py3.u wrapper function doesn't work when I compare an unicode literal to a variable which stores an unicode string.
I fixed the failed test cases according to Stackoverflow. Is it acceptable in libcloud?

This comment has been minimized.

@Kami

Kami Feb 27, 2016

Member

Ah, that's weird. I quickly tested it under Python 3.2 and it seemed to work OK (I didn't test other versions though).

Problem with unicode_literals is that we still support Python 2.5 where this functionality is not available (sadly we don't run 2.5 tests on Travis since Travis CI doesn't support 2.5 anymore).

In any case, consider this problem solved for now. I will try to figure out something when reviewing and merging this PR. In the worst case we can just use unicode_literals since we will probably drop support for Python 2.5 in Libcloud v1.0.0 anyway.

@Kami

Kami Feb 27, 2016

Member

Ah, that's weird. I quickly tested it under Python 3.2 and it seemed to work OK (I didn't test other versions though).

Problem with unicode_literals is that we still support Python 2.5 where this functionality is not available (sadly we don't run 2.5 tests on Travis since Travis CI doesn't support 2.5 anymore).

In any case, consider this problem solved for now. I will try to figure out something when reviewing and merging this PR. In the worst case we can just use unicode_literals since we will probably drop support for Python 2.5 in Libcloud v1.0.0 anyway.

Show outdated Hide outdated libcloud/compute/drivers/ecs.py
'accomplished': VolumeSnapshotState.AVAILABLE,
'failed': VolumeSnapshotState.ERROR}
def list_nodes(self, ex_node_ids=[], ex_filters=None):

This comment has been minimized.

@Kami

Kami Feb 27, 2016

Member

Minor thing - please don't default to a mutable type (list).

Rather to ex_node_ids=None and inside method body: ex_node_ids = ex_node_ids or []

@Kami

Kami Feb 27, 2016

Member

Minor thing - please don't default to a mutable type (list).

Rather to ex_node_ids=None and inside method body: ex_node_ids = ex_node_ids or []

This comment has been minimized.

@samsong8610

samsong8610 Mar 5, 2016

Contributor

Done

@samsong8610

samsong8610 Mar 5, 2016

Contributor

Done

Show outdated Hide outdated libcloud/compute/drivers/ecs.py
if 'ex_description' in kwargs:
params['Description'] = kwargs['ex_description']
if 'ex_internet_charge_type' in kwargs:

This comment has been minimized.

@Kami

Kami Feb 27, 2016

Member

Maybe this could be refactored in a utility method.

In any case, not a big deal.

@Kami

Kami Feb 27, 2016

Member

Maybe this could be refactored in a utility method.

In any case, not a big deal.

This comment has been minimized.

@samsong8610

samsong8610 Mar 5, 2016

Contributor

Done.

@samsong8610

samsong8610 Mar 5, 2016

Contributor

Done.

Show outdated Hide outdated libcloud/compute/drivers/ecs.py
locations = [self._to_location(each) for each in location_elements]
return locations
def create_node(self, **kwargs):

This comment has been minimized.

@Kami

Kami Feb 27, 2016

Member

If possible, please also explicitly declare all the arguments here instead of using kwargs (def create_node(self, name, size, image, ex_security_group_id, ...**kwargs)).

I know some drivers still use **kwargs, but we want to avoid that going forward.

@Kami

Kami Feb 27, 2016

Member

If possible, please also explicitly declare all the arguments here instead of using kwargs (def create_node(self, name, size, image, ex_security_group_id, ...**kwargs)).

I know some drivers still use **kwargs, but we want to avoid that going forward.

This comment has been minimized.

@samsong8610

samsong8610 Mar 5, 2016

Contributor

Done

@samsong8610

samsong8610 Mar 5, 2016

Contributor

Done

Show outdated Hide outdated libcloud/compute/drivers/ecs.py
extra['operation_locks'] = self._get_operation_locks(element)
return StorageVolume(_id, name, size, self, state=status, extra=extra)
def _list_to_json_array(self, value):

This comment has been minimized.

@Kami

Kami Feb 27, 2016

Member

I noticed you only use this method in one place, but if I'm not mistaken, it looks like you could also use it in some other places.

Or maybe not, since it looks like that in those other places you throw a more explicit and user-friendly exception if the argument is not of a type list.

@Kami

Kami Feb 27, 2016

Member

I noticed you only use this method in one place, but if I'm not mistaken, it looks like you could also use it in some other places.

Or maybe not, since it looks like that in those other places you throw a more explicit and user-friendly exception if the argument is not of a type list.

This comment has been minimized.

@samsong8610

samsong8610 Mar 5, 2016

Contributor

I should use it all the time except list_images method, in which the API need a comma separated string instead of a json array string.

@samsong8610

samsong8610 Mar 5, 2016

Contributor

I should use it all the time except list_images method, in which the API need a comma separated string instead of a json array string.

Show outdated Hide outdated libcloud/compute/drivers/ecs.py
"""
Define the extra dictionary for specific resources
"""
RESOURCE_EXTRA_ATTRIBUTES_MAP = {

This comment has been minimized.

@Kami

Kami Feb 27, 2016

Member

Can you please move this to the top after imports?

We tend to put global, module level constants at the top, immediately after imports.

@Kami

Kami Feb 27, 2016

Member

Can you please move this to the top after imports?

We tend to put global, module level constants at the top, immediately after imports.

This comment has been minimized.

@samsong8610

samsong8610 Mar 5, 2016

Contributor

But this dict is only used in this module, I mean that it is like a private utils function, so I think it should be put with other private functions.

Anyway, I will follow our conventions.😄

@samsong8610

samsong8610 Mar 5, 2016

Contributor

But this dict is only used in this module, I mean that it is like a private utils function, so I think it should be put with other private functions.

Anyway, I will follow our conventions.😄

Show outdated Hide outdated libcloud/compute/providers.py
@@ -173,6 +173,8 @@
('libcloud.compute.drivers.ciscoccs', 'CiscoCCSNodeDriver'),
Provider.NTTA:
('libcloud.compute.drivers.ntta', 'NTTAmericaNodeDriver'),
Provider.ECS:

This comment has been minimized.

@Kami

Kami Feb 27, 2016

Member

Is it possible to use some other name for the provider constant? Maybe ALIYUN_ECS?

If we call it just ECS it's slightly confusing and many users will probably mistaken it for Amazon Elastic Container Service (ECS).

@Kami

Kami Feb 27, 2016

Member

Is it possible to use some other name for the provider constant? Maybe ALIYUN_ECS?

If we call it just ECS it's slightly confusing and many users will probably mistaken it for Amazon Elastic Container Service (ECS).

This comment has been minimized.

@samsong8610

samsong8610 Mar 5, 2016

Contributor

Done

@samsong8610

samsong8610 Mar 5, 2016

Contributor

Done

Show outdated Hide outdated libcloud/loadbalancer/providers.py
@@ -42,6 +42,9 @@
('libcloud.loadbalancer.drivers.softlayer', 'SoftlayerLBDriver'),
Provider.DIMENSIONDATA:
('libcloud.loadbalancer.drivers.dimensiondata', 'DimensionDataLBDriver'),
Provider.SLB:

This comment has been minimized.

@Kami

Kami Feb 27, 2016

Member

Same here, probably a good idea to be explicit and add ALIYUN_ prefix to every provider constant.

@Kami

Kami Feb 27, 2016

Member

Same here, probably a good idea to be explicit and add ALIYUN_ prefix to every provider constant.

This comment has been minimized.

@samsong8610

samsong8610 Mar 5, 2016

Contributor

Done

@samsong8610

samsong8610 Mar 5, 2016

Contributor

Done

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Feb 27, 2016

Member

I've added some comments. Overall, very nice work 👍

After you look into / address those comments, could you please also add some documentation for those drivers?

Member

Kami commented Feb 27, 2016

I've added some comments. Overall, very nice work 👍

After you look into / address those comments, could you please also add some documentation for those drivers?

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Feb 29, 2016

Member

@samsong8610 Did you select to notify Libcloud project on the ICLA form?

I'm wondering, because I still didn't receive an email notification about the signed ICLA, maybe ASF secretary is just busy...

Edit: I do see it under "Unlisted CLAs" here - https://people.apache.org/committer-index.html (unless that's some other Sam Song :)).

Member

Kami commented Feb 29, 2016

@samsong8610 Did you select to notify Libcloud project on the ICLA form?

I'm wondering, because I still didn't receive an email notification about the signed ICLA, maybe ASF secretary is just busy...

Edit: I do see it under "Unlisted CLAs" here - https://people.apache.org/committer-index.html (unless that's some other Sam Song :)).

@samsong8610

This comment has been minimized.

Show comment
Hide comment
@samsong8610

samsong8610 Mar 2, 2016

Contributor

@Kami It's my fault. I didn't fill the 'notify project' field. I will re-sign the ICLA.

Contributor

samsong8610 commented Mar 2, 2016

@Kami It's my fault. I didn't fill the 'notify project' field. I will re-sign the ICLA.

Refactor codes according to the project convention
Use immutable value for argument default
Prefer explicit arguments to kwargs
Add ALIYUN_ prefix to providers constant
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Mar 11, 2016

Member

@samsong8610 Sorry for the delay, I was traveling.

I will look into reviewing it again and merging it ASAP (tomorrow).

Member

Kami commented Mar 11, 2016

@samsong8610 Sorry for the delay, I was traveling.

I will look into reviewing it again and merging it ASAP (tomorrow).

@@ -64,6 +65,7 @@ class Provider(object):
KTUCLOUD = 'ktucloud'
AURORAOBJECTS = 'auroraobjects'
BACKBLAZE_B2 = 'backblaze_b2'
ALIYUN_OSS = 'oss'

This comment has been minimized.

@Kami

Kami Mar 12, 2016

Member

To avoid potential conflict in the future, I will prefix type values for all the Aliyun providers with "aliyun_", so "aliyun_oss" in this case.

@Kami

Kami Mar 12, 2016

Member

To avoid potential conflict in the future, I will prefix type values for all the Aliyun providers with "aliyun_", so "aliyun_oss" in this case.

This comment has been minimized.

@Kami
@Kami

@asfgit asfgit closed this in e6002e0 Mar 12, 2016

asfgit pushed a commit that referenced this pull request Mar 12, 2016

[LIBCLOUD-802] Refactor codes according to the project convention
Use immutable value for argument default
Prefer explicit arguments to kwargs
Add ALIYUN_ prefix to providers constant

Closes #712

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>

asfgit pushed a commit that referenced this pull request Mar 12, 2016

[LIBCLOUD-802] Add doc page and examples for Aliyun ecs
Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>

Closes #712
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Mar 12, 2016

Member

I went ahead and merged changes into trunk.

Thanks again for this very complete and high-quality contribution!

Member

Kami commented Mar 12, 2016

I went ahead and merged changes into trunk.

Thanks again for this very complete and high-quality contribution!

@samsong8610

This comment has been minimized.

Show comment
Hide comment
@samsong8610

samsong8610 Mar 13, 2016

Contributor

Glad to help the libcloud. Thanks for your review.

Contributor

samsong8610 commented Mar 13, 2016

Glad to help the libcloud. Thanks for your review.

@samsong8610 samsong8610 deleted the aliyun-beta:add_drivers_for_aliyun_cloud branch Mar 13, 2016

dineshbhoopathy pushed a commit to dineshbhoopathy/libcloud that referenced this pull request Mar 24, 2016

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