Merge lp:~frankban/lpsetup/bug-1034602-handle-target-dir into lp:lpsetup

[Francesco Banconi]

handle_target_dir validates the provided directory.

= Summary =

handle_target_dir (used by update subcommand) should raise an error if the target dir does not exist or is not writable.

== Implementation ==

I decided to also add another validation: handle_target_dir fails if the target dir does not look like a Launchpad branch. This way we can avoid `update` to start, create required directories (eggs, yui etc...), and then crash because `update-sourcecode` can not be found in the target dir.

== Other changes ==

Updated dry run smoke tests to pass a valid target dir to `lp-setup update`.

Added tests for handle_target_dir.

Defined `test_template_dir` at module level in lpsetup.tests.utils: that template directory is now used in 3 different places.

[Brad Crittenden (bac)]

* typo: does not exist

* It is minor but I'd change "the target dir {0} is not a valid Launchpad branch" to say it is not a valid Launchpad tree. It may be a valid branch but not have a tree associated with it, i.e. devel vs sandbox.

* I think it would be cleaner to move the addCleanup in HandleTargetDirTest.setUp to be done right after the mktemp().

* Your test rely on the supposition that mktemp() will put things in /tmp. It might be more self-documenting and future proof if you extracted the actual directory using os.path.dirname and use that instead.

* The test_exists test is a bit fragile in that other processes could create that same directory after you remove it. Highly unlikely. But, you could avoid the appearance of a problem by using a subdirectory inside temporary directory as something you know does not exist. Picky, for sure.

* In test_is_writable your cleanup is either unnecessary because rmtree doesn't care and removes it anyway or it depends on the chmod cleanup being called before the rmtree cleanup. I just verified it is the former, rmtree does not care about write permissions when removing an owned tree, so the chmod cleanup is unnecessary.

This is a great improvement. Thanks.

review: Approve

[Launchpad QA Bot (lpqabot)]

The attempt to merge lp:~frankban/lpsetup/bug-1034602-handle-target-dir into lp:lpsetup failed. Below is the output from the failed tests.

+ set -o errexit
++ grep -v distribute_setup.py
++ find . -name build -prune -o -name '*.py'
+ pyfiles='./setup.py
./lplxcip/tests/utils.py
./lplxcip/tests/__init__.py
./lplxcip/tests/test_lxcip.py
./lplxcip/tests/test_helpers.py
./lplxcip/tests/test_utils.py
./lplxcip/lxcip.py
./lpsetup/utils.py
./lpsetup/__init__.py
./lpsetup/handlers.py
./lpsetup/tests/utils.py
./lpsetup/tests/__init__.py
./lpsetup/tests/test_argparser.py
./lpsetup/tests/test_cli.py
./lpsetup/tests/test_handlers.py
./lpsetup/tests/subcommands/__init__.py
./lpsetup/tests/subcommands/test_version.py
./lpsetup/tests/subcommands/test_initrepo.py
./lpsetup/tests/subcommands/test_init_target.py
./lpsetup/tests/subcommands/test_smoke.py
./lpsetup/tests/integration/common.py
./lpsetup/tests/integration/test_install_lxc.py
./lpsetup/tests/integration/test_init_target.py
./lpsetup/tests/test_utils.py
./lpsetup/subcommands/__init__.py
./lpsetup/subcommands/install_lxc.py
./lpsetup/subcommands/initlxc.py
./lpsetup/subcommands/init_target.py
./lpsetup/subcommands/update.py
./lpsetup/subcommands/initrepo.py
./lpsetup/subcommands/version.py
./lpsetup/exceptions.py
./lpsetup/argparser.py
./lpsetup/settings.py
./lpsetup/cli.py'
+ pocketlint ./setup.py ./lplxcip/tests/utils.py ./lplxcip/tests/__init__.py ./lplxcip/tests/test_lxcip.py ./lplxcip/tests/test_helpers.py ./lplxcip/tests/test_utils.py ./lplxcip/lxcip.py ./lpsetup/utils.py ./lpsetup/__init__.py ./lpsetup/handlers.py ./lpsetup/tests/utils.py ./lpsetup/tests/__init__.py ./lpsetup/tests/test_argparser.py ./lpsetup/tests/test_cli.py ./lpsetup/tests/test_handlers.py ./lpsetup/tests/subcommands/__init__.py ./lpsetup/tests/subcommands/test_version.py ./lpsetup/tests/subcommands/test_initrepo.py ./lpsetup/tests/subcommands/test_init_target.py ./lpsetup/tests/subcommands/test_smoke.py ./lpsetup/tests/integration/common.py ./lpsetup/tests/integration/test_install_lxc.py ./lpsetup/tests/integration/test_init_target.py ./lpsetup/tests/test_utils.py ./lpsetup/subcommands/__init__.py ./lpsetup/subcommands/install_lxc.py ./lpsetup/subcommands/initlxc.py ./lpsetup/subcommands/init_target.py ./lpsetup/subcommands/update.py ./lpsetup/subcommands/initrepo.py ./lpsetup/subcommands/version.py ./lpsetup/exceptions.py ./lpsetup/argparser.py ./lpsetup/settings.py ./lpsetup/cli.py
+ pep8 --exclude=build ./setup.py ./lplxcip/tests/utils.py ./lplxcip/tests/__init__.py ./lplxcip/tests/test_lxcip.py ./lplxcip/tests/test_helpers.py ./lplxcip/tests/test_utils.py ./lplxcip/lxcip.py ./lpsetup/utils.py ./lpsetup/__init__.py ./lpsetup/handlers.py ./lpsetup/tests/utils.py ./lpsetup/tests/__init__.py ./lpsetup/tests/test_argparser.py ./lpsetup/tests/test_cli.py ./lpsetup/tests/test_handlers.py ./lpsetup/tests/subcommands/__init__.py ./lpsetup/tests/subcommands/test_version.py ./lpsetup/tests/subcommands/test_initrepo.py ./lpsetup/tests/subcommands/test_init_target.py ./lpsetup/tests/subcommands/test_smoke.py ./lpsetup/tests/integration/common.py ./lpsetup/tests/integration/test_install_lxc.py ./lpsetup/tests/integration/test_init...

[lp:~frankban/lpsetup/bug-1034602-handle-target-dir]

83. By Francesco Banconi on 2012-08-24

Changes per review.

[Francesco Banconi (frankban)]

Thanks for your review Brad. I fixed the branch following your suggestions with one exception:
I've seen that removing the chmod clean up generates this error:

======================================================================
ERROR: test_is_writable (lpsetup.tests.test_handlers.HandleTargetDirTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/shutil.py", line 245, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "/usr/lib/python2.7/shutil.py", line 254, in rmtree
    onerror(os.rmdir, path, sys.exc_info())
  File "/usr/lib/python2.7/shutil.py", line 252, in rmtree
    os.rmdir(path)
OSError: [Errno 13] Permission denied: '/tmp/tmp6TfXhT/utilities'

So, the write permission missing does not prevent you from deleting the directory iself, but its contents.
The chmod cleanup is called before the rmtree cleanup because the (documented) system used is LIFO.
Maybe a try/finally block could be more explicit?

review: Approve

=== modified file 'lpsetup/handlers.py'
--- lpsetup/handlers.py	2012-08-10 08:34:16 +0000
+++ lpsetup/handlers.py	2012-08-24 16:08:23 +0000
@@ -268,10 +268,35 @@
     """Handle path to the working directory.
 
     The namespace must contain *target_dir*.
+    Raise *ValidationError* if the resulting target dir:
+
+        - does not exist
+        - is not a directory
+        - is not writable by the current user
+        - does not seem a valid Launchpad branch
+
     It should be invoked when *target_dir* is explicitly passed to a command.
     """
-    path = namespace.target_dir
-    namespace.target_dir = os.path.abspath(os.path.expanduser(path))
+    target_dir = os.path.abspath(os.path.expanduser(namespace.target_dir))
+    # The target dir must be an existing directory.
+    if not os.path.isdir(target_dir):
+        raise ValidationError(
+            'the target dir {0} does not exist or is not a directory.'.format(
+                target_dir))
+    # The target dir must be writable.
+    if not os.access(target_dir, os.W_OK):
+        raise ValidationError(
+            'the target dir {0} is not writable by the current user.'.format(
+                target_dir))
+    # The target dir must look like a Launchpad tree.
+    # This check is done in a naive way: `utilities/update-sourcecode`
+    # must be included in the target dir.
+    path_to_check = os.path.join(target_dir, 'utilities', 'update-sourcecode')
+    if not os.path.exists(path_to_check):
+        raise ValidationError(
+            'the target dir {0} is not a valid Launchpad tree.'.format(
+                target_dir))
+    namespace.target_dir = target_dir
 
 
 def handle_branch_and_checkout(namespace):

=== modified file 'lpsetup/tests/subcommands/test_smoke.py'
--- lpsetup/tests/subcommands/test_smoke.py	2012-08-23 19:21:55 +0000
+++ lpsetup/tests/subcommands/test_smoke.py	2012-08-24 16:08:23 +0000
@@ -10,7 +10,10 @@
     SUBCOMMANDS,
     )
 
-from lpsetup.tests.utils import capture_output
+from lpsetup.tests.utils import (
+    capture_output,
+    test_template_dir,
+    )
 
 
 class SmokeTest(unittest.TestCase):
@@ -31,7 +34,7 @@
             'init-lxc': required_args,
             'init-repo': [],
             'install-lxc': required_args,
-            'update': [],
+            'update': [test_template_dir],
             }
         warning = 'This command will perform the following actions:'
         for name, args in name_args_map.items():

=== added directory 'lpsetup/tests/test-branch/utilities'
=== added file 'lpsetup/tests/test-branch/utilities/update-sourcecode'
=== modified file 'lpsetup/tests/test_handlers.py'
--- lpsetup/tests/test_handlers.py	2012-08-06 09:37:04 +0000
+++ lpsetup/tests/test_handlers.py	2012-08-24 16:08:23 +0000
@@ -25,6 +25,7 @@
     handle_lpuser_as_username,
     handle_lpuser_from_lplogin,
     handle_ssh_keys,
+    handle_target_dir,
     handle_target_from_repository,
     handle_testing,
     handle_user,
@@ -33,6 +34,7 @@
 from lpsetup.tests.utils import (
     lpuser,
     skip_if_no_lpuser,
+    test_template_dir,
     )
 
 
@@ -276,6 +278,61 @@
             handle_ssh_keys(self.namespace)
 
 
+class HandleTargetDirTest(HandlersTestMixin, unittest.TestCase):
+
+    def setUp(self):
+        # Create a base target dir structure.
+        self.target_dir = tempfile.mktemp()
+        self.addCleanup(shutil.rmtree, self.target_dir)
+        shutil.copytree(test_template_dir, self.target_dir)
+        self.dirname = os.path.split(self.target_dir)[1]
+
+    def test_absolute_path(self):
+        # Ensure the resulting target dir is an absolute path.
+        namespace = argparse.Namespace(target_dir=self.dirname)
+        with cd(tempfile.tempdir):
+            handle_target_dir(namespace)
+        self.assertEqual(self.target_dir, namespace.target_dir)
+
+    def test_expanduser(self):
+        # Ensure the user is correctly expanded in the resulting target dir.
+        namespace = argparse.Namespace(target_dir='~/' + self.dirname)
+        with environ(HOME=tempfile.tempdir):
+            handle_target_dir(namespace)
+        self.assertEqual(self.target_dir, namespace.target_dir)
+
+    def test_isdir(self):
+        # An error is raised if the provided target dir is not a directory.
+        with tempfile.NamedTemporaryFile() as f:
+            namespace = argparse.Namespace(target_dir=f.name)
+            with self.assertNotValid('is not a directory'):
+                handle_target_dir(namespace)
+
+    def test_exists(self):
+        # An error is raised if the provided target dir does not exist.
+        target_dir = os.path.join(self.target_dir, '__does_not_exist__')
+        namespace = argparse.Namespace(target_dir=target_dir)
+        with self.assertNotValid('does not exist'):
+            handle_target_dir(namespace)
+
+    def test_is_writable(self):
+        # An error is raised if the target dir can not be written by
+        # current user.
+        namespace = argparse.Namespace(target_dir=self.target_dir)
+        os.chmod(self.target_dir, 0500)
+        self.addCleanup(os.chmod, self.target_dir, 0700)
+        with self.assertNotValid('is not writable'):
+            handle_target_dir(namespace)
+
+    def test_is_launchpad_tree(self):
+        # An error is raised if the target dir is not a valid Launchpad tree.
+        os.remove(
+            os.path.join(self.target_dir, 'utilities', 'update-sourcecode'))
+        namespace = argparse.Namespace(target_dir=self.target_dir)
+        with self.assertNotValid('is not a valid Launchpad tree'):
+            handle_target_dir(namespace)
+
+
 class HandleTargetFromRepositoryTest(unittest.TestCase):
 
     branch_name = 'branch'

=== modified file 'lpsetup/tests/utils.py'
--- lpsetup/tests/utils.py	2012-08-13 14:27:59 +0000
+++ lpsetup/tests/utils.py	2012-08-24 16:08:23 +0000
@@ -97,6 +97,10 @@
         self.assertEqual(self.message + '\n', stream.getvalue())
 
 
+# Template directory used to create a Launchpad branch.
+test_template_dir = os.path.join(os.path.dirname(__file__), 'test-branch')
+
+
 def create_test_branch(template_dir=None):
     """Create a temporary test branch containing files in *template_dir*.
 
@@ -106,7 +110,7 @@
     Return the path of the newly created branch.
     """
     if template_dir is None:
-        template_dir = os.path.join(os.path.dirname(__file__), 'test-branch')
+        template_dir = test_template_dir
     branch_path = tempfile.mktemp()
     shutil.copytree(template_dir, branch_path)
     call('bzr', 'init', '--quiet', branch_path)