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

Lazy entry #822

Merged
merged 8 commits into from Oct 9, 2016

Conversation

Projects
None yet
5 participants
@tonybaloney
Contributor

tonybaloney commented Jun 24, 2016

Introduce a convenience method to the libcloud module for getting drivers

Description

I find the current mechanism for fetching drivers a little cumbersome, 2 namespaces to import. After using the requests library I really like the module __init__ accessors for common attributes. The most common factory is get_driver and the most common enum is the Provider enum.

This code allows for this example:

import libcloud
cls = libcloud.get_driver(libcloud.DriverType.COMPUTE, libcloud.DriverType.COMPUTE.RACKSPACE)

Status

  • done, ready for review

Checklist (tick everything that applies)

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

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jun 25, 2016

Member

I'm not too opinionated about of and I'm fine with the overall change and "external API".

Member

Kami commented Jun 25, 2016

I'm not too opinionated about of and I'm fine with the overall change and "external API".

Show outdated Hide outdated libcloud/__init__.py
@@ -75,3 +94,40 @@ def _init_once():
paramiko.common.logging.basicConfig(level=paramiko.common.DEBUG)
_init_once()
class DriverType:

This comment has been minimized.

@Kami

Kami Jun 25, 2016

Member

Good idea to be consistent and inherit from object even if it's just used as an ENUM class.

@Kami

Kami Jun 25, 2016

Member

Good idea to be consistent and inherit from object even if it's just used as an ENUM class.

Show outdated Hide outdated libcloud/__init__.py
"""
Get a driver
"""
return DriverTypeFactoryMap[type](provider)

This comment has been minimized.

@Kami

Kami Jun 25, 2016

Member

We should probably re-throw a more user-friendly exception in case API / driver is not found, right?

Since I imagine KeyError or similar might be a bit confusing and not immediately obvious to the user what is going on.

@Kami

Kami Jun 25, 2016

Member

We should probably re-throw a more user-friendly exception in case API / driver is not found, right?

Since I imagine KeyError or similar might be a bit confusing and not immediately obvious to the user what is going on.

Show outdated Hide outdated libcloud/__init__.py
class DriverType:
""" Backup-as-a-service driver """

This comment has been minimized.

@Kami

Kami Jun 25, 2016

Member

I would also prefer to move this code to base.py or similar module since usually the convention is for init to either contain initialization code or be empty (and this is not really initialization code).

We can of course also still import DriverType and get_driver from other module here so user can still do "from libcloud import get_driver" for convenience.

@Kami

Kami Jun 25, 2016

Member

I would also prefer to move this code to base.py or similar module since usually the convention is for init to either contain initialization code or be empty (and this is not really initialization code).

We can of course also still import DriverType and get_driver from other module here so user can still do "from libcloud import get_driver" for convenience.

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Jun 25, 2016

Contributor

@Kami updated

Contributor

tonybaloney commented Jun 25, 2016

@Kami updated

Show outdated Hide outdated libcloud/base.py
try:
return DriverTypeFactoryMap[type](provider)
except KeyError:
raise DriverTypeNotFoundError()

This comment has been minimized.

@Kami

Kami Jun 25, 2016

Member

I think it would also be good to include type in the error message.

@Kami

Kami Jun 25, 2016

Member

I think it would also be good to include type in the error message.

This comment has been minimized.

@Kami

Kami Jun 25, 2016

Member

And test for it would be nice :)

Besides that, LGTM, 👍

@Kami

Kami Jun 25, 2016

Member

And test for it would be nice :)

Besides that, LGTM, 👍

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jun 25, 2016

Member

Would also be good to be update upgrade notes if want this to be a preferred approach for retrieving a driver down the road :)

For that though, it might be worth soliciting feedback and reaching consensus from more people.

Member

Kami commented Jun 25, 2016

Would also be good to be update upgrade notes if want this to be a preferred approach for retrieving a driver down the road :)

For that though, it might be worth soliciting feedback and reaching consensus from more people.

@erjohnso

This comment has been minimized.

Show comment
Hide comment
@erjohnso

erjohnso Jul 3, 2016

Member

Looks good to me. A good follow-on would be to update site docs / examples for this new approach.

Member

erjohnso commented Jul 3, 2016

Looks good to me. A good follow-on would be to update site docs / examples for this new approach.

@JamieCressey

This comment has been minimized.

Show comment
Hide comment
@JamieCressey

JamieCressey Jul 4, 2016

Contributor

Observation from an outsider...

If you're defining a provider class as libcloud.DriverType.SERVICE.PROVIDER. Can we not determine the service from the provider, negating the requirement to also provide a service object, therefore simplifying the instantiation from:

cls = libcloud.get_driver(libcloud.DriverType.COMPUTE, libcloud.DriverType.COMPUTE.RACKSPACE)

to:

cls = libcloud.get_driver(libcloud.DriverType.COMPUTE.RACKSPACE)

Just a thought, either way seems much cleaner than the current method.

Contributor

JamieCressey commented Jul 4, 2016

Observation from an outsider...

If you're defining a provider class as libcloud.DriverType.SERVICE.PROVIDER. Can we not determine the service from the provider, negating the requirement to also provide a service object, therefore simplifying the instantiation from:

cls = libcloud.get_driver(libcloud.DriverType.COMPUTE, libcloud.DriverType.COMPUTE.RACKSPACE)

to:

cls = libcloud.get_driver(libcloud.DriverType.COMPUTE.RACKSPACE)

Just a thought, either way seems much cleaner than the current method.

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Jul 16, 2016

Contributor

@JamieCressey that would be better using the Enum base type in Python 3.4+ but once it has been called, its already been converted into the value (string), and there's no difference in values between storage.Providers.RACKSPACE and loadbalancer.Providers.RACKSPACE, they're both 'rackspace'.
Instead I created seperate factory methods, get_compute_driver, get_storage_driver with the type inbuilt.
There are some cases where people might want to generate a driver dynamically without knowing the type, so get_driver is there as a catch-all.

Contributor

tonybaloney commented Jul 16, 2016

@JamieCressey that would be better using the Enum base type in Python 3.4+ but once it has been called, its already been converted into the value (string), and there's no difference in values between storage.Providers.RACKSPACE and loadbalancer.Providers.RACKSPACE, they're both 'rackspace'.
Instead I created seperate factory methods, get_compute_driver, get_storage_driver with the type inbuilt.
There are some cases where people might want to generate a driver dynamically without knowing the type, so get_driver is there as a catch-all.

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Aug 11, 2016

Contributor

I'm pushing this in unless any serious objections?

Contributor

tonybaloney commented Aug 11, 2016

I'm pushing this in unless any serious objections?

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Oct 9, 2016

Contributor

merging

Contributor

tonybaloney commented Oct 9, 2016

merging

@asfgit asfgit merged commit 7db284c into apache:trunk Oct 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

asfgit pushed a commit that referenced this pull request Oct 9, 2016

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