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

[AWS] placement group #418

Closed
wants to merge 2 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@ardling

ardling commented Dec 18, 2014

creating/deleteing/listing placement groups with tests.
Plese give feedback if something wrong.

Show outdated Hide outdated libcloud/compute/drivers/ec2.py Outdated
Show outdated Hide outdated libcloud/compute/drivers/ec2.py Outdated
Show outdated Hide outdated libcloud/compute/drivers/ec2.py Outdated
response = self.connection.request(self.path, params=params).object
return self._get_boolean(response)
def ex_list_placement_groups(self, names=()):

This comment has been minimized.

@Kami

Kami Dec 20, 2014

Member

You should never use a mutable default argument - http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments

You can do names=None and inside the function do names = names or []

@Kami

Kami Dec 20, 2014

Member

You should never use a mutable default argument - http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments

You can do names=None and inside the function do names = names or []

This comment has been minimized.

@ardling

ardling Dec 22, 2014

I know about mutable default arguments, but () is tuple and tuples are immutable. Do I realy must fix it?

@ardling

ardling Dec 22, 2014

I know about mutable default arguments, but () is tuple and tuples are immutable. Do I realy must fix it?

Show outdated Hide outdated libcloud/compute/drivers/ec2.py Outdated
Show outdated Hide outdated libcloud/compute/drivers/ec2.py Outdated
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Dec 20, 2014

Member

Thanks!

I've added some comments in-line. Overall it looks good, there are just some minor styling and compatibility issues which need to be addressed.

Member

Kami commented Dec 20, 2014

Thanks!

I've added some comments in-line. Overall it looks good, there are just some minor styling and compatibility issues which need to be addressed.

@asfgit asfgit closed this in f7025a9 Jan 4, 2015

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jan 4, 2015

Member

Sorry for the delay - I didn't see you pushed a new commit (we don't get notifications for that).

Next time, feel free to ping us when you address the comments.

Anyway, I've merged patch into trunk. Thanks!

Member

Kami commented Jan 4, 2015

Sorry for the delay - I didn't see you pushed a new commit (we don't get notifications for that).

Next time, feel free to ping us when you address the comments.

Anyway, I've merged patch into trunk. Thanks!

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