Libcloud
  1. Libcloud
  2. LIBCLOUD-249

This patch adds support for HostVirtual API http://www.vr.org/ (compute and dns driver)

    Details

    1. apifix.patch
      6 kB
      Dinesh Bhoopathy
    2. withdocstrings.patch
      18 kB
      Dinesh Bhoopathy
    3. compute.patch
      7 kB
      Dinesh Bhoopathy

      Activity

      Hide
      Tomaz Muraus added a comment -

      Did you also intended to attach a patch to this issue?

      Show
      Tomaz Muraus added a comment - Did you also intended to attach a patch to this issue?
      Hide
      Dinesh Bhoopathy added a comment -

      Oh yeah, I'll update it. Just checking a few things with it.

      Show
      Dinesh Bhoopathy added a comment - Oh yeah, I'll update it. Just checking a few things with it.
      Hide
      Dinesh Bhoopathy added a comment -

      patch with pep8 clean up

      Show
      Dinesh Bhoopathy added a comment - patch with pep8 clean up
      Hide
      Tomaz Muraus added a comment -

      Thanks for the patch.

      It mostly looks good, besides the PEP8 issues (lines longer then 80 characters, etc.), but it's missing tests. One of the requirements for this patch being merged are tests covering the new functionality.

      If you need any help with writing the tests, let us know.

      Show
      Tomaz Muraus added a comment - Thanks for the patch. It mostly looks good, besides the PEP8 issues (lines longer then 80 characters, etc.), but it's missing tests. One of the requirements for this patch being merged are tests covering the new functionality. If you need any help with writing the tests, let us know.
      Hide
      Dinesh Bhoopathy added a comment -

      Ah yeah. My bad, sorry about the pep8 issues, I did start to clean it up but missed a thing or two before I patched it, weird my vim plugin didn't catch it.

      I started looking at the tests and I'm working on it right now. Just a quick question, I've setup tox and things, how do I get to run the test just for a single provider ?

      Show
      Dinesh Bhoopathy added a comment - Ah yeah. My bad, sorry about the pep8 issues, I did start to clean it up but missed a thing or two before I patched it, weird my vim plugin didn't catch it. I started looking at the tests and I'm working on it right now. Just a quick question, I've setup tox and things, how do I get to run the test just for a single provider ?
      Hide
      Tomaz Muraus added a comment -

      You can run a single test by running the following command:

      PYTHONPATH=. python libcloud/test/compute/your_file.py

      Show
      Tomaz Muraus added a comment - You can run a single test by running the following command: PYTHONPATH=. python libcloud/test/compute/your_file.py
      Hide
      Tomaz Muraus added a comment -

      I see you removed the patch files, was this intentional?

      Show
      Tomaz Muraus added a comment - I see you removed the patch files, was this intentional?
      Hide
      Dinesh Bhoopathy added a comment -

      test cases + pep8 cleanup

      Show
      Dinesh Bhoopathy added a comment - test cases + pep8 cleanup
      Hide
      Dinesh Bhoopathy added a comment -

      Yeah, I managed to fix my vim plugin for pep8 and the cleanup was smooth, just uploaded the compute driver with test cases. I'll have to start on the dns tests tomo.
      Will keep you posted!

      Show
      Dinesh Bhoopathy added a comment - Yeah, I managed to fix my vim plugin for pep8 and the cleanup was smooth, just uploaded the compute driver with test cases. I'll have to start on the dns tests tomo. Will keep you posted!
      Hide
      Tomaz Muraus added a comment -

      Please use git diff to generate patch, because I can't easily apply git patch on a svn tree.

      You can find instructions on how to do that on the main page - http://libcloud.apache.org/contributing.html.

      Show
      Tomaz Muraus added a comment - Please use git diff to generate patch, because I can't easily apply git patch on a svn tree. You can find instructions on how to do that on the main page - http://libcloud.apache.org/contributing.html .
      Hide
      Dinesh Bhoopathy added a comment -

      git diff patch!

      Show
      Dinesh Bhoopathy added a comment - git diff patch!
      Hide
      Tomaz Muraus added a comment -

      Merged into trunk, thanks!

      Show
      Tomaz Muraus added a comment - Merged into trunk, thanks!
      Hide
      Tomaz Muraus added a comment -

      Reopening, because ticket was updated with DNS patch.

      Show
      Tomaz Muraus added a comment - Reopening, because ticket was updated with DNS patch.
      Hide
      Tomaz Muraus added a comment - - edited

      Any special reason why you ignore ttl which is passed to "create_zone" and manually set it to 3600?

      If there is a reason please document it in the method docstring.

      Show
      Tomaz Muraus added a comment - - edited Any special reason why you ignore ttl which is passed to "create_zone" and manually set it to 3600? If there is a reason please document it in the method docstring.
      Hide
      Tomaz Muraus added a comment -

      Patch looks good in general. Here are a couple of more things I have noticed:

      1. I renamed 'ttl_sec' to 'ttl' because all of the other drivers use 'ttl'

      3. Why do you pre-populate 'extra' dictionary with 'soa' and 'ns' attributes when creating a zone?

      3. Tests look good, but there are no tests for all the different edge cases.

      Thanks

      Show
      Tomaz Muraus added a comment - Patch looks good in general. Here are a couple of more things I have noticed: 1. I renamed 'ttl_sec' to 'ttl' because all of the other drivers use 'ttl' 3. Why do you pre-populate 'extra' dictionary with 'soa' and 'ns' attributes when creating a zone? 3. Tests look good, but there are no tests for all the different edge cases. Thanks
      Hide
      Tomaz Muraus added a comment -

      Thanks for including the tests, it already looks way better

      I've noticed one thing - inside test_get_zone_does_not_exist and other tests you are catching HostVirtualException exception.

      In DNS API we have special (more specific) exception classes for those cases (ZoneDoesNotExistError, RecordDoesNotExistError) and your code should throw and catch those exceptions.

      Show
      Tomaz Muraus added a comment - Thanks for including the tests, it already looks way better I've noticed one thing - inside test_get_zone_does_not_exist and other tests you are catching HostVirtualException exception. In DNS API we have special (more specific) exception classes for those cases (ZoneDoesNotExistError, RecordDoesNotExistError) and your code should throw and catch those exceptions.
      Hide
      Tomaz Muraus added a comment -

      Thanks, I have reviewed the updated patch. It mostly looked good, but there were still some issues which I have fixed myself:

      1. responseCls attribute belongs to the Connection class and NOT driver class.

      2. DNS_PARAMS_HOSTVIRTUAL = ('key') - missing a comma, comma makes a tuple.

      3. You have used the same response class (HostVirtualResponse) in the compute and DNS driver and referenced DNS API specific exceptions inside the compute driver. This is a bad practice in many way (code reuse, APIs shouldn't be tighly coupled, etc.).

      I have moved common functionality into a separate module (libcloud.common.hostvirtual.

      {HostVirtualConnection,HostVirtualException,HostVirtualResponse}

      ) and created a HostVirtualDNSResponse sub-class inside libcloud.dns.drivers.hostvirtual which throws DNS related exceptions.

      I have also merged patch into trunk now. Please have a look at the diff and the changes I made - http://svn.apache.org/viewvc?view=revision&revision=r1421073

      Thanks!

      Show
      Tomaz Muraus added a comment - Thanks, I have reviewed the updated patch. It mostly looked good, but there were still some issues which I have fixed myself: 1. responseCls attribute belongs to the Connection class and NOT driver class. 2. DNS_PARAMS_HOSTVIRTUAL = ('key') - missing a comma, comma makes a tuple. 3. You have used the same response class (HostVirtualResponse) in the compute and DNS driver and referenced DNS API specific exceptions inside the compute driver. This is a bad practice in many way (code reuse, APIs shouldn't be tighly coupled, etc.). I have moved common functionality into a separate module (libcloud.common.hostvirtual. {HostVirtualConnection,HostVirtualException,HostVirtualResponse} ) and created a HostVirtualDNSResponse sub-class inside libcloud.dns.drivers.hostvirtual which throws DNS related exceptions. I have also merged patch into trunk now. Please have a look at the diff and the changes I made - http://svn.apache.org/viewvc?view=revision&revision=r1421073 Thanks!
      Hide
      Dinesh Bhoopathy added a comment -

      Hey Tomaz, uploaded a new patch for the 0.12.1 release, can you review it ?

      Show
      Dinesh Bhoopathy added a comment - Hey Tomaz, uploaded a new patch for the 0.12.1 release, can you review it ?
      Hide
      Tomaz Muraus added a comment -

      Patch looks good to me.

      I will work on merging it as soon as Apache SVN repository is working (currently there are some issues and I can't commit changes).

      On a related note - please leave old patches / files attached to the ticket and don't delete them in the future. This way they stay here for the future reference.

      Show
      Tomaz Muraus added a comment - Patch looks good to me. I will work on merging it as soon as Apache SVN repository is working (currently there are some issues and I can't commit changes). On a related note - please leave old patches / files attached to the ticket and don't delete them in the future. This way they stay here for the future reference.
      Hide
      Tomaz Muraus added a comment -

      Actually, I just tested and reviewed the patch and here are some issues I have noticed:

      • Tests fail with new changes
      • New extension methods are missing docstrings

      Happy to merge it as soon as those issues are addresses.

      Show
      Tomaz Muraus added a comment - Actually, I just tested and reviewed the patch and here are some issues I have noticed: Tests fail with new changes New extension methods are missing docstrings Happy to merge it as soon as those issues are addresses.
      Hide
      Dinesh Bhoopathy added a comment -

      okay, its with docstrings for the extension methods now and tests are passing

      ----------------------------------------------------------------------
      Ran 13 tests in 0.118s

      OK

      Show
      Dinesh Bhoopathy added a comment - okay, its with docstrings for the extension methods now and tests are passing ---------------------------------------------------------------------- Ran 13 tests in 0.118s OK
      Hide
      ASF subversion and git services added a comment -

      Commit 1442703 from Tomaz Muraus
      [ https://svn.apache.org/r1442703 ]

      Minor improvements in the HostVirtual driver and add new ex_get_node and
      ex_build_node extension method.

      Contributed by Dinesh Bhoopathy, part of LIBCLOUD-249.

      Show
      ASF subversion and git services added a comment - Commit 1442703 from Tomaz Muraus [ https://svn.apache.org/r1442703 ] Minor improvements in the HostVirtual driver and add new ex_get_node and ex_build_node extension method. Contributed by Dinesh Bhoopathy, part of LIBCLOUD-249 .
      Hide
      Tomaz Muraus added a comment - - edited

      Latest patch merged into trunk with some minor fixes. Thanks!

      Show
      Tomaz Muraus added a comment - - edited Latest patch merged into trunk with some minor fixes. Thanks!
      Hide
      Dinesh Bhoopathy added a comment -

      Reopening to patch bugs and api changes

      Show
      Dinesh Bhoopathy added a comment - Reopening to patch bugs and api changes
      Hide
      ASF subversion and git services added a comment -

      Commit 1480490 from Tomaz Muraus
      [ https://svn.apache.org/r1480490 ]

      Various bug fixes and improvements in the HostVirtual driver.

      Contributed by Dinesh Bhoopathy, part of LIBCLOUD-249.

      Show
      ASF subversion and git services added a comment - Commit 1480490 from Tomaz Muraus [ https://svn.apache.org/r1480490 ] Various bug fixes and improvements in the HostVirtual driver. Contributed by Dinesh Bhoopathy, part of LIBCLOUD-249 .
      Hide
      Tomaz Muraus added a comment -

      Thanks. There were some issues (style, missing doc string, failing test, etc.) which I've fixed and merged your patch into trunk and 0.12.x.

      kami /w/lc/t1 (svn)$ PYTHONPATH=. python libcloud/test/compute/test_hostvirtual.py
      ...E..........
      ======================================================================
      ERROR: test_ex_build_node (_main_.HostVirtualTest)
      ----------------------------------------------------------------------
      Traceback (most recent call last):
      File "libcloud/test/compute/test_hostvirtual.py", line 120, in test_ex_build_node
      self.assertTrue(self.driver.ex_build_node(
      AttributeError: 'HostVirtualNodeDriver' object has no attribute 'ex_build_node'

      ----------------------------------------------------------------------
      Ran 14 tests in 0.008s

      Next time please make sure you add the docstrings and make sure tests are passing before submitting a patch.

      Show
      Tomaz Muraus added a comment - Thanks. There were some issues (style, missing doc string, failing test, etc.) which I've fixed and merged your patch into trunk and 0.12.x. kami /w/lc/t1 (svn)$ PYTHONPATH=. python libcloud/test/compute/test_hostvirtual.py ...E.......... ====================================================================== ERROR: test_ex_build_node (_ main _.HostVirtualTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "libcloud/test/compute/test_hostvirtual.py", line 120, in test_ex_build_node self.assertTrue(self.driver.ex_build_node( AttributeError: 'HostVirtualNodeDriver' object has no attribute 'ex_build_node' ---------------------------------------------------------------------- Ran 14 tests in 0.008s Next time please make sure you add the docstrings and make sure tests are passing before submitting a patch.
      Hide
      Dinesh Bhoopathy added a comment -

      This is embarrassing. I thought I had the docstrings and no tests were failing. I even remember fixing that broken test case a while ago.

      Thanks for patching it. Looking at your changes against my branch and gotta find what went wrong.

      Show
      Dinesh Bhoopathy added a comment - This is embarrassing. I thought I had the docstrings and no tests were failing. I even remember fixing that broken test case a while ago. Thanks for patching it. Looking at your changes against my branch and gotta find what went wrong.
      Hide
      Tomaz Muraus added a comment -

      No problem, this can happy to anyone.

      Show
      Tomaz Muraus added a comment - No problem, this can happy to anyone.
      Hide
      ASF subversion and git services added a comment -

      Commit 3386cfdeb38f995f9355bbcc089d20077a92c11b in branch refs/heads/0.12.x from Tomaz Muraus
      [ https://git-wip-us.apache.org/repos/asf?p=libcloud.git;h=3386cfd ]

      Various bug fixes and improvements in the HostVirtual driver.

      Contributed by Dinesh Bhoopathy, part of LIBCLOUD-249.

      git-svn-id: https://svn.apache.org/repos/asf/libcloud/trunk@1480490 13f79535-47bb-0310-9956-ffa450edef68

      Conflicts:
      CHANGES

      Show
      ASF subversion and git services added a comment - Commit 3386cfdeb38f995f9355bbcc089d20077a92c11b in branch refs/heads/0.12.x from Tomaz Muraus [ https://git-wip-us.apache.org/repos/asf?p=libcloud.git;h=3386cfd ] Various bug fixes and improvements in the HostVirtual driver. Contributed by Dinesh Bhoopathy, part of LIBCLOUD-249 . git-svn-id: https://svn.apache.org/repos/asf/libcloud/trunk@1480490 13f79535-47bb-0310-9956-ffa450edef68 Conflicts: CHANGES

        People

        • Assignee:
          Tomaz Muraus
          Reporter:
          Dinesh Bhoopathy
        • Votes:
          0 Vote for this issue
          Watchers:
          4 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development