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-356] Add config_drive to support user_data #330

Closed
wants to merge 4 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@stickystyle
Contributor

stickystyle commented Jun 28, 2014

No description provided.

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/openstack.py
@stickystyle

This comment has been minimized.

Show comment
Hide comment
@stickystyle

stickystyle Jun 30, 2014

Contributor

Going on your suggestion and looking through the convention in other drivers @Kami, would it be the correct style to raise a ValueError if the user set ex_config_drive to True and failed to pass in ex_userdata ?

Contributor

stickystyle commented Jun 30, 2014

Going on your suggestion and looking through the convention in other drivers @Kami, would it be the correct style to raise a ValueError if the user set ex_config_drive to True and failed to pass in ex_userdata ?

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jun 30, 2014

Member

@stickystyle I think that's reasonable, although in cases like this, we usually leave validation like this to be performed by the server / API.

This way, we don't need to update the code if the business logic changes and passing one value without another becomes OK (which is unlikely in this case, but still).

Member

Kami commented Jun 30, 2014

@stickystyle I think that's reasonable, although in cases like this, we usually leave validation like this to be performed by the server / API.

This way, we don't need to update the code if the business logic changes and passing one value without another becomes OK (which is unlikely in this case, but still).

@stickystyle

This comment has been minimized.

Show comment
Hide comment
@stickystyle

stickystyle Jun 30, 2014

Contributor

@Kami I've updated the pull request to have config_drive as a kwarg, and didn't include the ValueError exception as I agree with you that leaving it up to the API is probably a more maintainable solution from the client side.

Let me know if there are any other changes that need to be made, thanks!

Contributor

stickystyle commented Jun 30, 2014

@Kami I've updated the pull request to have config_drive as a kwarg, and didn't include the ValueError exception as I agree with you that leaving it up to the API is probably a more maintainable solution from the client side.

Let me know if there are any other changes that need to be made, thanks!

@@ -1342,6 +1347,9 @@ def _create_args_to_params(self, node, **kwargs):
server_params['user_data'] = base64.b64encode(
b(kwargs['ex_userdata'])).decode('ascii')
if 'ex_config_drive' in kwargs:

This comment has been minimized.

@Kami

Kami Jun 30, 2014

Member

Probably be better to do something like the code bellow because user could potentially pass in ex_config_drive=False to the method and we would still send True.

if 'ex_config_drive' in kwargs:
    server_params['config_drive'] = kwargs['ex_config_drive']
@Kami

Kami Jun 30, 2014

Member

Probably be better to do something like the code bellow because user could potentially pass in ex_config_drive=False to the method and we would still send True.

if 'ex_config_drive' in kwargs:
    server_params['config_drive'] = kwargs['ex_config_drive']

@asfgit asfgit closed this in 5e60407 May 17, 2015

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami May 17, 2015

Member

Really sorry for the long delay (we don't get notifications when you push commits so I didn't notice that you updated the PR).

In any case, this change has finally been merged now. Thanks again.

Member

Kami commented May 17, 2015

Really sorry for the long delay (we don't get notifications when you push commits so I didn't notice that you updated the PR).

In any case, this change has finally been merged now. Thanks again.

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