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

Add `ex_set_volume_auto_delete` to the GCE driver. #264

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
3 participants
@fcuny
Contributor

fcuny commented Mar 14, 2014

Sets the auto-delete flag for a volume attached to a node.

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/gce.py
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Mar 14, 2014

Member

Besides the naming thing, LGTM.

/cc @wrigri

Member

Kami commented Mar 14, 2014

Besides the naming thing, LGTM.

/cc @wrigri

@fcuny

This comment has been minimized.

Show comment
Hide comment
@fcuny

fcuny Mar 14, 2014

Contributor

Ha, interesting. For both suggestions, that's what I would normally have done. But the previous function declaration (def detach_volume(self, volume, ex_node=None):) was setting a ex_node to None by default (even if it's required). I was trying to copy the style :)

Would a second pull request to modify the detach_function signature be accepted ?

Thanks for the quick feedback, will update.

Contributor

fcuny commented Mar 14, 2014

Ha, interesting. For both suggestions, that's what I would normally have done. But the previous function declaration (def detach_volume(self, volume, ex_node=None):) was setting a ex_node to None by default (even if it's required). I was trying to copy the style :)

Would a second pull request to modify the detach_function signature be accepted ?

Thanks for the quick feedback, will update.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Mar 14, 2014

Member

@franckcuny Yeah, a second pull request would be much appreciated.

An sadly we currently only enforce things like this manually during the PR review so it's kinda easy to miss it. The good news is that we are working on an automatic system for checking that the driver and methods comply to the base API :)

Member

Kami commented Mar 14, 2014

@franckcuny Yeah, a second pull request would be much appreciated.

An sadly we currently only enforce things like this manually during the PR review so it's kinda easy to miss it. The good news is that we are working on an automatic system for checking that the driver and methods comply to the base API :)

Add `ex_set_volume_auto_delete` to the GCE driver.
Sets the auto-delete flag for a volume attached to a node.
@wrigri

This comment has been minimized.

Show comment
Hide comment
@wrigri

wrigri Mar 14, 2014

Contributor

Looks good to me

I'm a bit confused about the conversation regarding a second pull request for detach_volume. The signature for detach volume is like it is because the base API doesn't allow for the node to be set as a positional argument. So, even though volume.detach() won't actually work for GCE, adding a positional argument for node to detach_volume would cause volume.detach() to error out.

Contributor

wrigri commented Mar 14, 2014

Looks good to me

I'm a bit confused about the conversation regarding a second pull request for detach_volume. The signature for detach volume is like it is because the base API doesn't allow for the node to be set as a positional argument. So, even though volume.detach() won't actually work for GCE, adding a positional argument for node to detach_volume would cause volume.detach() to error out.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Mar 21, 2014

Member

Patch merged into trunk, the pull request can be closed now. Thanks.

And sorry for the delay. I thought I already merged this a while back.

Member

Kami commented Mar 21, 2014

Patch merged into trunk, the pull request can be closed now. Thanks.

And sorry for the delay. I thought I already merged this a while back.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Apr 7, 2014

Member

@franckcuny Can you please close this pull request (changes have already been merged a while back)?

Thanks

Member

Kami commented Apr 7, 2014

@franckcuny Can you please close this pull request (changes have already been merged a while back)?

Thanks

@fcuny fcuny closed this Apr 7, 2014

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