Merge lp:~stevenk/launchpad/poppy-sftp-updates into lp:launchpad

[Steve Kowalik]

Clean up the poppy-sftp service a little bit, exercise more code in the tests, and add a prefix which means it is far less likely to have a directory name clash with Poppy.

This branch just does a little clean-up of the poppy-sftp service that landed yesterday:

* Export FileIsADirectory from lp.codehosting.sftp so I can use it.
* Add a prefix argument to poppy's hooks, and use it for the SFTP server. This means it is far less likely that we will have a directory name clash with both the FTP and SFTP service running on one host.
* Note the bug number for the time.sleep XXX.
* Conform to PEP8 in the one place I missed.

[Abel Deuring (adeuring)]

Hi Steve,

nice work, and thanks again for the added tests!

review: Approve (code)

[Curtis Hovey (sinzui)]

Lp XXX style is
# XXX <engineer> <yyyy-mm-dd> [bug=<number>|spec=<number>]

[Curtis Hovey (sinzui)]

You may land this once you have fixed the XXX.

review: Approve (release-critical)

=== modified file 'lib/lp/codehosting/sftp.py'
--- lib/lp/codehosting/sftp.py	2010-04-19 10:56:23 +0000
+++ lib/lp/codehosting/sftp.py	2010-05-28 13:44:40 +0000
@@ -16,6 +16,7 @@
 __all__ = [
     'avatar_to_sftp_server',
     'TransportSFTPServer',
+    'FileIsADirectory',
     ]
 
 

=== modified file 'lib/lp/poppy/hooks.py'
--- lib/lp/poppy/hooks.py	2010-03-17 11:43:01 +0000
+++ lib/lp/poppy/hooks.py	2010-05-28 13:44:40 +0000
@@ -21,13 +21,14 @@
     clients = {}
 
     def __init__(self, targetpath, logger, allow_user, cmd=None,
-                 targetstart=0, perms=None):
+                 targetstart=0, perms=None, prefix=''):
         self.targetpath = targetpath
         self.logger = logging.getLogger("%s.Hooks" % logger.name)
         self.cmd = cmd
         self.allow_user = allow_user
         self.targetcount = targetstart
         self.perms = perms
+        self.prefix = prefix
 
     def new_client_hook(self, fsroot, host, port):
         """Prepare a new client record indexed by fsroot..."""
@@ -81,7 +82,8 @@
         try:
             self.targetcount += 1
             timestamp = time.strftime("%Y%m%d-%H%M%S")
-            path = "upload-%s-%06d" % (timestamp, self.targetcount)
+            path = "upload%s-%s-%06d" % (
+                self.prefix, timestamp, self.targetcount)
             target_fsroot = os.path.join(self.targetpath, path)
 
             # Create file to store the distro used.

=== modified file 'lib/lp/poppy/tests/test_poppy.py'
--- lib/lp/poppy/tests/test_poppy.py	2010-05-27 07:11:05 +0000
+++ lib/lp/poppy/tests/test_poppy.py	2010-05-28 13:44:40 +0000
@@ -133,7 +133,9 @@
         pass
 
     def waitForClose(self):
-        # XXX: Eww
+        # XXX: Steve Kowalik 2010-05-24 bug=586695 There has to be a
+        # better way to wait for the SFTP server to process our upload
+        # rather than sleeping for 10 seconds.
         time.sleep(10)
 
     def getTransport(self):
@@ -278,7 +280,13 @@
         self.server.disconnect(transport)
         self.server.waitForClose()
 
-        self.assertEqual(os.stat(self._uploadPath('')).st_mode, 042770)
+        upload_path = self._uploadPath('')
+        self.assertEqual(os.stat(upload_path).st_mode, 042770)
+        dir_name = upload_path.split('/')[-2]
+        if transport._user == 'joe':
+            self.assertEqual(dir_name.startswith('upload-sftp-2'), True)
+        elif transport._user == 'ubuntu':
+            self.assertEqual(dir_name.startswith('upload-2'), True)
         for upload in files:
             wanted_path = self._uploadPath(
                 "~ppa-user/ppa/ubuntu/%s" % upload)

=== modified file 'lib/lp/poppy/tests/test_twistedsftp.py'
--- lib/lp/poppy/tests/test_twistedsftp.py	2010-05-27 07:11:05 +0000
+++ lib/lp/poppy/tests/test_twistedsftp.py	2010-05-28 13:44:40 +0000
@@ -9,16 +9,17 @@
 import tempfile
 import unittest
 
-from twisted.trial.unittest import TestCase as TrialTestCase
-
+from lp.codehosting.sftp import FileIsADirectory
 from lp.poppy.twistedsftp import SFTPServer
-
-
-class TestSFTPServer(TrialTestCase):
+from lp.testing import TestCase
+
+
+class TestSFTPServer(TestCase):
 
     def setUp(self):
         self.fs_root = tempfile.mkdtemp()
         self.sftp_server = SFTPServer(None, self.fs_root)
+        super(TestSFTPServer, self).setUp()
 
     def test_gotVersion(self):
         # gotVersion always returns an empty dict, since the server does not
@@ -51,6 +52,13 @@
         self.assertEqual(test_file.read(), "This is a test")
         test_file.close()
         self.assertEqual(os.stat(file_name).st_mode, 0100654)
+        dir_name = os.path.join(self.sftp_server._current_upload, 'bar/foo')
+        os.makedirs(dir_name)
+        upload_file = self.sftp_server.openFile('bar/foo', None, None)
+        self.assertRaisesWithContent(
+            FileIsADirectory,
+            "File is a directory: '%s'" % dir_name,
+            upload_file.writeChunk, 0, "This is a test")
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/poppy/twistedsftp.py'
--- lib/lp/poppy/twistedsftp.py	2010-05-27 07:07:24 +0000
+++ lib/lp/poppy/twistedsftp.py	2010-05-28 13:44:40 +0000
@@ -9,9 +9,9 @@
     'SFTPServer',
     ]
 
+import errno
 import logging
 import os
-import stat
 import tempfile
 
 from twisted.conch.interfaces import ISFTPFile, ISFTPServer
@@ -19,6 +19,7 @@
 import zope.component.event
 from zope.interface import implements
 
+from lp.codehosting.sftp import FileIsADirectory
 from lp.poppy.filesystem import UploadFileSystem
 from lp.poppy.hooks import Hooks
 from lp.services.sshserver.events import SFTPClosed
@@ -36,9 +37,10 @@
         self._fs_root = fsroot
         self.uploadfilesystem = UploadFileSystem(tempfile.mkdtemp())
         self._current_upload = self.uploadfilesystem.rootpath
-        os.chmod(self._current_upload, stat.S_IRWXU + stat.S_IRWXG)
+        os.chmod(self._current_upload, 0770)
         self._log = logging.getLogger("poppy-sftp")
-        self.hook = Hooks(self._fs_root, self._log, "ubuntu", perms='g+rws')
+        self.hook = Hooks(
+            self._fs_root, self._log, "ubuntu", perms='g+rws', prefix='-sftp')
         self.hook.new_client_hook(self._current_upload, 0, 0)
         self.hook.auth_verify_hook(self._current_upload, None, None)
 
@@ -104,6 +106,7 @@
             return
         self.hook.client_done_hook(self._current_upload, 0, 0)
 
+
 class SFTPFile:
 
     implements(ISFTPFile)
@@ -124,7 +127,7 @@
         except OSError, e:
             if e.errno != errno.EISDIR:
                 raise
-            raise FileIsADirectory(name)
+            raise FileIsADirectory(self.filename)
         os.lseek(chunk_file, offset, 0)
         os.write(chunk_file, data)
         os.close(chunk_file)