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.

Sign up for GitHub

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

Jump to bottom

[LIBCLOUD-802] Add drivers for Aliyun cloud #712

Closed
wants to merge 3 commits into from
Closed

[LIBCLOUD-802] Add drivers for Aliyun cloud #712

wants to merge 3 commits into from

Conversation

samsong8610
Copy link
Contributor

@Kami
Copy link
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
Copy link
Contributor Author

@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
Copy link
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.

Kami
Kami reviewed Feb 26, 2016
View reviewed changes
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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@samsong8610 samsong8610 force-pushed the add_drivers_for_aliyun_cloud branch 2 times, most recently from e50e4cd to 608ff12 Compare February 27, 2016 00:53
Kami
Kami reviewed Feb 27, 2016
View reviewed changes
libcloud/compute/drivers/ecs.py
'accomplished': VolumeSnapshotState.AVAILABLE,
'failed': VolumeSnapshotState.ERROR}

def list_nodes(self, ex_node_ids=[], ex_filters=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 []

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Kami
Copy link
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
Copy link
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
Copy link
Contributor Author

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

@samsong8610
Refactor codes according to the project convention
9d834da 
Use immutable value for argument default
Prefer explicit arguments to kwargs
Add ALIYUN_ prefix to providers constant
@Kami
Copy link
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).

Kami
Kami reviewed Mar 12, 2016
View reviewed changes
libcloud/storage/types.py
@@ -64,6 +65,7 @@ class Provider(object):
KTUCLOUD = 'ktucloud'
AURORAOBJECTS = 'auroraobjects'
BACKBLAZE_B2 = 'backblaze_b2'
ALIYUN_OSS = 'oss'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3da1574

@asfgit asfgit closed this in e6002e0 Mar 12, 2016
asfgit pushed a commit that referenced this pull request Mar 12, 2016
@samsong8610 @Kami
[LIBCLOUD-802] Refactor codes according to the project convention
ae4c482 
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
@samsong8610 @Kami
[LIBCLOUD-802] Add doc page and examples for Aliyun ecs
a8479e1 
Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>

Closes #712
@Kami
Copy link
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
Copy link
Contributor Author

Glad to help the libcloud. Thanks for your review.

@samsong8610 samsong8610 deleted the add_drivers_for_aliyun_cloud branch March 13, 2016 14:24
dineshbhoopathy pushed a commit to dineshbhoopathy/libcloud that referenced this pull request Mar 24, 2016
Merge LIBCLOUD-712_fix-ec2-sigver into trunk
59cfd4f 
closes apache#712
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@samsong8610 @Kami