Skip to content

Add ex_set_volume_auto_delete to the GCE driver.#264

Closed
fcuny wants to merge 1 commit into
apache:trunkfrom
fcuny:gce-autodelete
Closed

Add ex_set_volume_auto_delete to the GCE driver.#264
fcuny wants to merge 1 commit into
apache:trunkfrom
fcuny:gce-autodelete

Conversation

@fcuny
Copy link
Copy Markdown

@fcuny fcuny commented Mar 14, 2014

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

Comment thread libcloud/compute/drivers/gce.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the ex_node argument is required, you should simply not set the default value in the method signature and Python will throw if you don't pass this argument in.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, extension methods already have ex_ prefix in the method name so you can remove it from the argument (ex_node -> node).

You only need to add ex_ prefix to the argument if you add it to the standard method (e.g. list_nodes).

@Kami
Copy link
Copy Markdown
Member

Kami commented Mar 14, 2014

Besides the naming thing, LGTM.

/cc @wrigri

@fcuny
Copy link
Copy Markdown
Author

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
Copy link
Copy Markdown
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 :)

Sets the auto-delete flag for a volume attached to a node.
@wrigri
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants