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.

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

Driver Implementation for the cloudscale.ch API #951

Merged
merged 7 commits into from Dec 2, 2016

Conversation

@davidhalter
Copy link
Contributor

davidhalter commented Nov 22, 2016

A Driver Implementation for the cloudscale.ch API

Description

It's pretty simple, I have added a new driver for the cloudscale.ch API (https://www.cloudscale.ch/en/api/v1). The cloudscale.ch API is very simple and doesn't offer a lot more than what libcloud's compute module by default offers. I have also tried to communicate this in the documentation.

Status

The code feels mostly like it's done and ready for review. It passes 2.7, 3.3, 3.4, 3.5, lint and pylint. I have looked at other drivers in libcloud and tried to copy how they document/test. I haven't added tons of tests, but comparably to others the driver is also smaller and therefore the amount of tests seem to align.

Just let me know what you think. Happy to improve my code!

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)
---------------------

Simply visit `<https://control.cloudscale.ch/user/api-tokens>`_ and start
having fun!

This comment has been minimized.

@Feuermurmel

Feuermurmel Nov 23, 2016

Maybe you want to reword this. ;)

libcloud.DriverType.COMPUTE.CLOUDSCALE
)

driver = cls('3pjzjh3h3rfynqa4iemvtvc33pyfzss2')

This comment has been minimized.

@Feuermurmel

Feuermurmel Nov 23, 2016

I'd store the token string in a variable token to clarify what that string is. Or maybe add a comment.

'''
return self._list_resources('/v1/images', self._to_image)

def create_node(self, name, size, image, location=None, ex_create_attr={}):

This comment has been minimized.

@Feuermurmel

Feuermurmel Nov 23, 2016

What do you think about accepting the arguments in ex_create_attr directly separate parameters instead? Compare with CloudStackNodeDriver.create_node(): https://github.com/apache/libcloud/blob/trunk/libcloud/compute/drivers/cloudstack.py#L1528

This comment has been minimized.

@davidhalter

davidhalter Nov 23, 2016 Author Contributor

I would argue that some other drivers (e.g. digitalocean) are doing it the same way. Prefixing everything with ex_ feels clunky and having a dictionary makes it easy to be "future-compatible".

This comment has been minimized.

@tonybaloney

tonybaloney Nov 28, 2016 Contributor

I'm ok with this, as long as the attributes are documented somewhere. You could also name the main ones and just merge them into the dictionary.

This comment has been minimized.

@davidhalter

davidhalter Nov 28, 2016 Author Contributor

I have documented in docs/compute/drivers/cloudscale.rst. Should I add it to the docstring?

- ``use_private_network``: ``boolean`` Attaching/Detaching the private network interface.
- ``use_ipv6``: ``boolean`` Enabling/Disabling IPv6.
- ``anti_affinity_with``: ``string``; Pass the UUID of another server.
- ``user_data``: ``string``; Cloud-init configuration (cloud-config). Provide YAML.

This comment has been minimized.

@Feuermurmel

Feuermurmel Nov 23, 2016

I'd use Python's standard names for the types here (i.e. bool, str, …).

Also, is there a standard convention for how to format such lists with type information, e.g. here the format <id> (<type>) – <description> is used.

This comment has been minimized.

@davidhalter

davidhalter Nov 23, 2016 Author Contributor

I have adapted that. I think it's nicer.


class CloudscaleConnection(ConnectionKey):
"""
Connection class for the Vultr driver.

This comment has been minimized.

@Feuermurmel

Feuermurmel Nov 23, 2016

You're not covering your tracks very well. :P

@@ -0,0 +1 @@
[{"slug":"ubuntu-16.10","name":"Ubuntu 16.10 (2016-10-17)","operating_system": "Ubuntu"}]

This comment has been minimized.

@Feuermurmel

Feuermurmel Nov 23, 2016

Why no pretty printing here? :(

If you want to keep it on a single line, if suggest adding spaces after : and ,.

This comment has been minimized.

@davidhalter

davidhalter Nov 23, 2016 Author Contributor

I have actually removed the pretty printing because other drivers also don't use it (and this one is pretty short).

This comment has been minimized.

@Feuermurmel

Feuermurmel Nov 23, 2016

Then at least remove that space after the last :. :P

@davidhalter
Copy link
Contributor Author

davidhalter commented Nov 23, 2016

@Feuermurmel I have improved pretty much all the documentation that you complained about, except for the two places where I've commented.

Copy link
Contributor

tonybaloney left a comment

Overall this is a great contribution, I had a concern about the way errors are handled if there is a cleaner way of achieving this?



class CloudscaleResponse(JsonResponse):
valid_response_codes = [httplib.OK, httplib.ACCEPTED, httplib.CREATED,

This comment has been minimized.

@tonybaloney

tonybaloney Nov 28, 2016 Contributor

this is overridden just because of the addition of httplib.NO_CONTENT?

This comment has been minimized.

@davidhalter

davidhalter Nov 28, 2016 Author Contributor

Hmm, I don't see us overriding anything here, valid_response_codes does not exist in JsonResponse or Response.

# but that doesn't really matter. It's nicer if the error is just
# one error (because it's a Python API and there's only one
# exception.
return next(iter(body.values()))

This comment has been minimized.

@tonybaloney

tonybaloney Nov 28, 2016 Contributor

wah, ok what's this about? This is unusual!

This comment has been minimized.

@pquentin

pquentin Nov 28, 2016 Contributor

It means "take the first/any element of the collection". There's no nicer way to express it if your collection is not a list.

This comment has been minimized.

@tonybaloney

tonybaloney Nov 30, 2016 Contributor

If it means that we change the base method then I'd be happier with that, you should have a bit more control over the meta in the exception. Assuming that the other errors in the response could provide context to the user about what they did wrong they shouldn't have to use a packet sniffer to get that data https://github.com/apache/libcloud/blob/trunk/libcloud/common/base.py#L178-L180
Change that to an instance method (self.exception_from_message), which calls the top level function.
Override that method in your own class to raise a new exception class, inheriting from LibcloudException, adding a field for the list of errors. update __str__ with the first error, or a list joined with commas?

@davidhalter

This comment has been minimized.

@tonybaloney

tonybaloney Dec 2, 2016 Contributor

I'll do this another time. 👍 merging

'''
return self._list_resources('/v1/images', self._to_image)

def create_node(self, name, size, image, location=None, ex_create_attr={}):

This comment has been minimized.

@tonybaloney

tonybaloney Nov 28, 2016 Contributor

I'm ok with this, as long as the attributes are documented somewhere. You could also name the main ones and just merge them into the dictionary.

)
return response.status == httplib.OK

def _list_resources(self, url, tranform_func):

This comment has been minimized.

@tonybaloney

tonybaloney Nov 28, 2016 Contributor

Nice pattern 👍


- ``driver.list_sizes()``
- ``driver.list_images()``
- ``driver.list_sizes()``

This comment has been minimized.

@href

href Nov 28, 2016

It seems like this is a duplicate.

This comment has been minimized.

@davidhalter

davidhalter Nov 28, 2016 Author Contributor

Hmm, how is it a duplicate? Do you mean the autoclass listing?

This comment has been minimized.

@tonybaloney

tonybaloney Nov 28, 2016 Contributor

list_sizes is twice

This comment has been minimized.

@davidhalter

davidhalter Nov 29, 2016 Author Contributor

Oh, how did I not see that after you pointed it out. Thank you!

It should be list_nodes. I have changed it.

@davidhalter
Copy link
Contributor Author

davidhalter commented Nov 28, 2016

@tonybaloney How would you like me to modify error handling? Or what do you think is not clean enough?

@tonybaloney
Copy link
Contributor

tonybaloney commented Nov 28, 2016

@davidhalter if you haven't already, I need you to sign the ASF ICLA (an agreement for individual contributors) https://libcloud.readthedocs.io/en/latest/development.html#contributing-bigger-changes

@davidhalter
Copy link
Contributor Author

davidhalter commented Nov 29, 2016

@tonybaloney

I have signed it and I have also gotten confirmation:

This message acknowledges receipt of your ICLA, which has been filed in the Apache Software Foundation records.
With this message, the Libcloud PMC has been notified that your ICLA has been filed.

@asfgit asfgit merged commit 75300c5 into apache:trunk Dec 2, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
asfgit pushed a commit that referenced this pull request Dec 2, 2016
asfgit pushed a commit that referenced this pull request Dec 2, 2016
@tonybaloney
Copy link
Contributor

tonybaloney commented Dec 2, 2016

merged. thanks @davidhalter @pquentin @Feuermurmel and @href for getting this over the line and thanks for contributing to the project. New drivers can take a little bit of time but it's important to get some consistency.

@davidhalter
Copy link
Contributor Author

davidhalter commented Dec 2, 2016

@tonybaloney Thanks for reacting so quickly. Wish I could do that on my open source projects as well :) Keep up the good work!

@davidhalter
Copy link
Contributor Author

davidhalter commented Jan 3, 2017

@tonybaloney The documentation doesn't seem like it's up-to-date. Did we break the builds with this PR?

(AFAIK I was able to build the docs with this commit).

@Kami
Copy link
Member

Kami commented Jan 3, 2017

@davidhalter It looks like that for some reasons docs builds haven't been automatically triggered for a while now.

I manually triggered it for latest for now and I will look what is going on with automatically triggered builds.

@Kami
Copy link
Member

Kami commented Jan 3, 2017

The manually triggered build has finished and the provider is listed there now - https://libcloud.readthedocs.io/en/latest/compute/supported_providers.html

I believe there were two issues.

  1. Automatic build triggering has been broken for a while now (:sadface:) - c06497f
  2. Table with link to the provider documentation has been broken. Just a heads up - this table is automatically generated using ./contrib/generate_provider_feature_matrix_table.py script - e5fc049

I believe both issues should be fixed now, but need to wait for the Travis and RTD build to finish :)

@Kami
Copy link
Member

Kami commented Jan 3, 2017

It looks like automatic docs build triggering is working again - https://travis-ci.org/apache/libcloud/jobs/188560819#L855 and provider matrix is also back again https://libcloud.readthedocs.io/en/latest/supported_providers.html#provider-matrix :)

/cc @tonybaloney

@davidhalter
Copy link
Contributor Author

davidhalter commented Jan 4, 2017

@Kami Thanks! Is there any way to PR for the website as well? I wasn't able to find a repository of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.