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

DNS driver for digitalocean and other minor Node driver updates #505

Closed
wants to merge 19 commits into from

Conversation

@jcastillo2nd
Copy link
Contributor

@jcastillo2nd jcastillo2nd commented Apr 14, 2015

  • This includes common.digitalocean with the Node driver using the DigitalOceanBaseDriver
  • DigitalOceanBaseDriver ( and NodeDriver ) will handle v1 and v2 credentials properly
  • DigitalOceanNodeDriver ( for v1 ) implements ( 4 of the ) KeyPair Management instead of using ex_ssh_*
  • DigitalOceanDNSDriver implemented, only with v2 credentials, as v1 will at some point be deprecated so it was not implemented
  • DigitalOceanDNSDriver tests have been created
  • Modified FixtureRoots to include common so that tests for common could use it

I'v run through the DigitalOcean files ( for common, node and dns ) with flake8, and with the exception of 2 tests files, ( with long strings that I don't know how to break up to pass the test AND land within 79 chars ) everything is clean.

I would like to note that the documentation on the KeyPair management may need to be updated to include return types that are consistent with the other components of libcloud. This could be setup in an issue, but at the same time, this could be arbitrarily added without breaking anything before accepting this merge.

  • KeyPair include a delete(self) function that calls driver.delete_key_pair(self)
  • create_key_pair() comment setup a rtype of KeyPair to make explicit that create_key_pair() returns a KeyPair instance ( like create_node returns a Node )
Javier Castillo II and others added 6 commits Apr 11, 2015
Pulled connection/response from libcloud.compute.driver.digitalocean into libcloud.common.digitalocean
Implemented majority functions in libcloud.dns.driver.digitalocean
- Added 'common' to FIXTURES_ROOT for FileFixtures in test/common
- Modified DigitalOceanBaseDriver from compute/drivers/digitalocean into common/digitalocean to support v1 and v2 by initialization
- Added dns/driver/digitalocean tests
- Added common/digitalocean tests
… tests from dns/test_digitalocean performed in DigitalOceanBaseDriver
- Support for v1 and v2 initialization with authentication secret
- Updated v1 DigitalOceanNodeDriver to use updated KeyPair Management
- Updated tests to reflect KeyPair Management changes
@jcastillo2nd
Copy link
Contributor Author

@jcastillo2nd jcastillo2nd commented Apr 14, 2015

I'm wondering if the hack is what is causing the provider feature matrix build to fail.

https://github.com/jcastillo2nd/libcloud/blob/trunk/contrib/generate_provider_feature_matrix_table.py#L224

Should this also include a check if this is node driver vs dns driver to let the normal driver selector hit without overriding?

        elif name.lower() == 'digital_ocean' and api != 'dns':
@Kami
Copy link
Member

@Kami Kami commented Apr 15, 2015

Great, thanks!

I will have a look asap.

@jcastillo2nd
Copy link
Contributor Author

@jcastillo2nd jcastillo2nd commented Apr 15, 2015

I'm thinking the hack can be removed entirely since the DigitalOceanNodeDriver can handle both cases with just passing of the secret or key only:

>>> from libcloud.compute.drivers.digitalocean import DigitalOceanNodeDriver
>>> n1 = DigitalOceanNodeDriver('v1_key', 'v1_secret')
>>> n1.__class__
<class 'libcloud.compute.drivers.digitalocean.DigitalOcean_v1_NodeDriver'>
>>> n2 = DigitalOceanNodeDriver('v2_key')
>>> n2.__class__
<class 'libcloud.compute.drivers.digitalocean.DigitalOcean_v2_NodeDriver'>

Also, I'm pulling the ex_ssh functions back into the v1 driver to live alongside the key pair functions so it doesn't break existing functionality. Poor foresight on my part.

- Removed hack to load DigitalOcean_v2_NodeDriver as DigitalOceanNodeDriver will handle v1/v2 properly
- Restored DigitalOceanNodeDriver (v1) ex_ssh* methods with Warning of deprecation
@jcastillo2nd
Copy link
Contributor Author

@jcastillo2nd jcastillo2nd commented Apr 15, 2015

Looks like removing the hack worked as it handles the tests properly for both v1 and v2 cases in DigitalOceanNodeDriver, also the old ex_ssh_* functions have been restored to v1, but with warnings.warn regarding deprecation.

This looks good from a working build perspective, let me know if there is anything else needed on this.

- Includes driver instantiation example
- Updated Node driver documentation to reflect duality of v1/v2 api in DigitalOceanNodeDriver
- Updated documentation for DigitalOcean regarding locations
- Fixed type for DigitalOcean DNS documentation
@@ -2,4 +2,4 @@
from libcloud.compute.providers import get_driver

cls = get_driver(Provider.DIGITAL_OCEAN)
driver = cls('client id', 'api key', api_version='v1')

This comment has been minimized.

@Kami

Kami May 3, 2015
Member

This is a minor thing - I prefer to be explicit and explicitly specify an api version. This way it's immediately obvious which API version is used.

This comment has been minimized.

@jcastillo2nd

jcastillo2nd May 28, 2015
Author Contributor

Hey, sorry for the delay. Personal life had some crazy changes cutting my time for projects to almost nothing. Going over these, and they should be easy to get done over the next couple days.

As for this entry in particular, I agree it should be explicit here as well. I'm also going to update the v2 API compute example to include the api_version='v2' as well.


def __new__(cls, key, secret=None, api_version='v2', **kwargs):
if cls is DigitalOceanBaseDriver:
if api_version == 'v1' or secret is not None:

This comment has been minimized.

@Kami

Kami May 3, 2015
Member

To avoid surprises we should throw if user specifies api_version == 'v2' and secret or api_version == 'v1' and missing secret).

This comment has been minimized.

@jcastillo2nd

jcastillo2nd May 28, 2015
Author Contributor

That is brilliant. I've used the common types InvalidCredsError here for both raise cases, as I think that is the best fit as far as I am familiar with the library. Let me know if there is another that would be better.

'domain_records')
# TODO: Not use list comprehension to add zone to record for proper data map
# functionality?
return list(map(self._to_record, data, [zone for z in data]))

This comment has been minimized.

@Kami

Kami May 3, 2015
Member

Hm, it looks like _to_record takes zone as a second argument, but you are passing a list of records as a zone argument.

Or am I missing something?

Also, please use keyword instead of position based arguments where possible to make this more readable and immediately obvious (self._to_record, data=data, zone=...).

This comment has been minimized.

@jcastillo2nd

jcastillo2nd May 28, 2015
Author Contributor

This is a tricky issue. It's reflected in the comment above it. So, the main action here is mapping the records ( list in data ) to the zone. In this particular case, I am building a list of the same zone to match the size of the list in data so that map can do it's thing.

So in this case, _to_record is actually getting an item from the data list, and an item from the zone list ( which is the same zone for all items in data ) through the map function so that it spits out a proper list of Record objects.

The reason I don't use the named arguments, is because this is in map which will break since it doesn't use named arguments.

Traceback (most recent call last): File "libcloud/test/dns/test_digitalocean.py", line 48, in test_list_records records = self.driver.list_records(zone) File "/home/jcastillo2nd/Development/libcloud/libcloud/dns/drivers/digitalocean.py", line 69, in list_records return list(map(self._to_record, data=data, zone=[zone for z in data])) TypeError: map() takes no keyword arguments

I would be glad to use the keyword here, but to have it work, and until a proper implementation can be generated for mapping this without making the ephemeral list of zone to properly get mapped to the data, I am at a loss. Any ideas on this would be appreciated on this, as this has bugged me from the beginning and though my memory is fuzzy, I think it was a compromise I made to generate proper record objects with real references to the Zone.

In the interim, I've gone ahead and added to the comment to clarify the lack of keyword usage here for map, and more clarification on why that list generator is used.

}
if data is None:
params['data'] = record.data
if extra:

This comment has been minimized.

@Kami

Kami May 3, 2015
Member

Since the same couple of lines of code are also used above this could perhaps be refactored into def _get_params_from_extra_args or a similar utility method.

This comment has been minimized.

@jcastillo2nd

jcastillo2nd May 28, 2015
Author Contributor

Regarding this case, create_record takes a zone as the input from which the values are derived, and in the update_record case, this is pulled directly from a record object. While they look similar, they are getting data from different objects which would make it tricky to derive the details in a single function. I'm not sure if this is worth pursuing, but if you think it is, any hints on how I would go about this?

This comment has been minimized.

@Kami

Kami Jun 14, 2015
Member

In this case, I think it's fine skipping it for now.

@Kami
Copy link
Member

@Kami Kami commented May 3, 2015

I've added some comments.

Overall the PR looks good, but there are some small things which need to be addressed so we can get it merged.

@Kami
Copy link
Member

@Kami Kami commented May 16, 2015

@jcastillo2nd Sorry for the delay and please ping me when those comments are addressed - I would love to get this change included in the next release.

- Raise on api_version / key or key-secret mismatch
- Added tests for v1 and v2 DigitalOceanNodeDriver using wrong keys
- Updated examples to include explicit api_version values
- Raise on api_version / key or key-secret mismatch
- Added tests for v1 and v2 DigitalOceanNodeDriver using wrong keys
- Updated examples to include explicit api_version values
- Cleaned up some whitespace lint
- Raise on api_version / key or key-secret mismatch
- Added tests for v1 and v2 DigitalOceanNodeDriver using wrong keys
- Updated examples to include explicit api_version values
- Cleaned up some whitespace lint
- Cleaned up tests
@jcastillo2nd
Copy link
Contributor Author

@jcastillo2nd jcastillo2nd commented May 28, 2015

@Kami Hopefully I've clarified the issues with the current code and moved things in the right direction. Let me know what your thoughts are on the pending issues.

- v2 driver referenced object instead of response
- v2 driver referenced object instead of response
- Update v2 Node driver to properly set data instead of parameters for API requests
- Added ex_ssh_key_ids to create_node
- Implemented list_key_pairs with public key retreival
- Consistency in v2 for attributes -> json request data
@jcastillo2nd jcastillo2nd mentioned this pull request May 28, 2015
@jcastillo2nd
Copy link
Contributor Author

@jcastillo2nd jcastillo2nd commented Jun 12, 2015

@Kami I've got some time over the next couple of weeks to work on anything else that may come up on this. If there are still any issues keeping this from being merged, I will hopefully be able to resolve them before the next release is established.

@asfgit asfgit closed this in fa6c99d Jun 14, 2015
@Kami
Copy link
Member

@Kami Kami commented Jun 14, 2015

I restored a slightly modified hack to make the table generation script work again (8952234) and merged changes into trunk.

Thanks.

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

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