yellow team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

Francesco Banconi has proposed merging lp:~frankban/lpsetup/bug-1034602-handle-target-dir into lp:lpsetup.

Requested reviews:
  Yellow Squad (yellow)
Related bugs:
  Bug #1034602 in lpsetup: "handle_target_dir should validate directory"
  https://bugs.launchpad.net/lpsetup/+bug/1034602

For more details, see:
https://code.launchpad.net/~frankban/lpsetup/bug-1034602-handle-target-dir/+merge/121198

= 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.

-- 
https://code.launchpad.net/~frankban/lpsetup/bug-1034602-handle-target-dir/+merge/121198
Your team Yellow Squad is requested to review the proposed merge of lp:~frankban/lpsetup/bug-1034602-handle-target-dir into lp:lpsetup.

=== modified file 'lpsetup/handlers.py'
--- lpsetup/handlers.py	2012-08-10 08:34:16 +0000
+++ lpsetup/handlers.py	2012-08-24 14:26: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 exists
+        - 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 branch.
+    # 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 branch.'.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 14:26: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 14:26: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,62 @@
             handle_ssh_keys(self.namespace)
 
 
+class HandleTargetDirTest(HandlersTestMixin, unittest.TestCase):
+
+    def setUp(self):
+        # Create a base target dir structure.
+        self.target_dir = tempfile.mktemp()
+        shutil.copytree(test_template_dir, self.target_dir)
+        self.dirname = os.path.basename(self.target_dir)
+        self.addCleanup(shutil.rmtree, self.target_dir)
+
+    def test_absolute_path(self):
+        # Ensure the resulting target dir is an absolute path.
+        namespace = argparse.Namespace(target_dir=self.dirname)
+        with cd('/tmp/'):
+            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='/tmp/'):
+            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 = tempfile.mkdtemp()
+        os.rmdir(target_dir)
+        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_branch(self):
+        # An error is raised if the target dir is not a valid Launchpad branch.
+        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 branch'):
+            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 14:26: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)