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

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

patch with pep8 clean up

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.

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 ?

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

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

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

test cases + pep8 cleanup

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!

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.

git diff patch!

Merged into trunk, thanks!

Reopening, because ticket was updated with DNS patch.

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.

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

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.

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.

) 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!

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

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.

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.

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

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

OK

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.

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

Reopening to patch bugs and api changes

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.

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.

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.

No problem, this can happy to anyone.

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