Launchpad itself

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

  1. poppy-sftp-updates
  2. Merge into devel
Proposed by Steve Kowalik
Status: Merged
Approved by: Steve Kowalik
Approved revision: no longer in the source branch.
Merged at revision: 10937
Proposed branch: lp:~stevenk/launchpad/poppy-sftp-updates
Merge into: lp:launchpad
Diff against target: 165 lines (+35/-13)
5 files modified
lib/lp/codehosting/sftp.py (+1/-0)
lib/lp/poppy/hooks.py (+4/-2)
lib/lp/poppy/tests/test_poppy.py (+10/-2)
lib/lp/poppy/tests/test_twistedsftp.py (+13/-5)
lib/lp/poppy/twistedsftp.py (+7/-4)
To merge this branch: bzr merge lp:~stevenk/launchpad/poppy-sftp-updates
Reviewer Review Type Date Requested Status
Curtis Hovey (community) release-critical Approve
Abel Deuring (community) code Approve
Review via email: mp+26279@code.launchpad.net

Commit message

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.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :

Hi Steve,

nice work, and thanks again for the added tests!

review: Approve (code)
Revision history for this message
Curtis Hovey (sinzui) wrote :

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

Revision history for this message
Curtis Hovey (sinzui) wrote :

You may land this once you have fixed the XXX.

review: Approve (release-critical)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
16__all__ = [16__all__ = [
17 'avatar_to_sftp_server',17 'avatar_to_sftp_server',
18 'TransportSFTPServer',18 'TransportSFTPServer',
19 'FileIsADirectory',
19 ]20 ]
2021
2122
2223
=== 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 @@
21 clients = {}21 clients = {}
2222
23 def __init__(self, targetpath, logger, allow_user, cmd=None,23 def __init__(self, targetpath, logger, allow_user, cmd=None,
24 targetstart=0, perms=None):24 targetstart=0, perms=None, prefix=''):
25 self.targetpath = targetpath25 self.targetpath = targetpath
26 self.logger = logging.getLogger("%s.Hooks" % logger.name)26 self.logger = logging.getLogger("%s.Hooks" % logger.name)
27 self.cmd = cmd27 self.cmd = cmd
28 self.allow_user = allow_user28 self.allow_user = allow_user
29 self.targetcount = targetstart29 self.targetcount = targetstart
30 self.perms = perms30 self.perms = perms
31 self.prefix = prefix
3132
32 def new_client_hook(self, fsroot, host, port):33 def new_client_hook(self, fsroot, host, port):
33 """Prepare a new client record indexed by fsroot..."""34 """Prepare a new client record indexed by fsroot..."""
@@ -81,7 +82,8 @@
81 try:82 try:
82 self.targetcount += 183 self.targetcount += 1
83 timestamp = time.strftime("%Y%m%d-%H%M%S")84 timestamp = time.strftime("%Y%m%d-%H%M%S")
84 path = "upload-%s-%06d" % (timestamp, self.targetcount)85 path = "upload%s-%s-%06d" % (
86 self.prefix, timestamp, self.targetcount)
85 target_fsroot = os.path.join(self.targetpath, path)87 target_fsroot = os.path.join(self.targetpath, path)
8688
87 # Create file to store the distro used.89 # Create file to store the distro used.
8890
=== 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 @@
133 pass133 pass
134134
135 def waitForClose(self):135 def waitForClose(self):
136 # XXX: Eww136 # XXX: Steve Kowalik 2010-05-24 bug=586695 There has to be a
137 # better way to wait for the SFTP server to process our upload
138 # rather than sleeping for 10 seconds.
137 time.sleep(10)139 time.sleep(10)
138140
139 def getTransport(self):141 def getTransport(self):
@@ -278,7 +280,13 @@
278 self.server.disconnect(transport)280 self.server.disconnect(transport)
279 self.server.waitForClose()281 self.server.waitForClose()
280282
281 self.assertEqual(os.stat(self._uploadPath('')).st_mode, 042770)283 upload_path = self._uploadPath('')
284 self.assertEqual(os.stat(upload_path).st_mode, 042770)
285 dir_name = upload_path.split('/')[-2]
286 if transport._user == 'joe':
287 self.assertEqual(dir_name.startswith('upload-sftp-2'), True)
288 elif transport._user == 'ubuntu':
289 self.assertEqual(dir_name.startswith('upload-2'), True)
282 for upload in files:290 for upload in files:
283 wanted_path = self._uploadPath(291 wanted_path = self._uploadPath(
284 "~ppa-user/ppa/ubuntu/%s" % upload)292 "~ppa-user/ppa/ubuntu/%s" % upload)
285293
=== 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 @@
9import tempfile9import tempfile
10import unittest10import unittest
1111
12from twisted.trial.unittest import TestCase as TrialTestCase12from lp.codehosting.sftp import FileIsADirectory
13
14from lp.poppy.twistedsftp import SFTPServer13from lp.poppy.twistedsftp import SFTPServer
1514from lp.testing import TestCase
1615
17class TestSFTPServer(TrialTestCase):16
17class TestSFTPServer(TestCase):
1818
19 def setUp(self):19 def setUp(self):
20 self.fs_root = tempfile.mkdtemp()20 self.fs_root = tempfile.mkdtemp()
21 self.sftp_server = SFTPServer(None, self.fs_root)21 self.sftp_server = SFTPServer(None, self.fs_root)
22 super(TestSFTPServer, self).setUp()
2223
23 def test_gotVersion(self):24 def test_gotVersion(self):
24 # gotVersion always returns an empty dict, since the server does not25 # gotVersion always returns an empty dict, since the server does not
@@ -51,6 +52,13 @@
51 self.assertEqual(test_file.read(), "This is a test")52 self.assertEqual(test_file.read(), "This is a test")
52 test_file.close()53 test_file.close()
53 self.assertEqual(os.stat(file_name).st_mode, 0100654)54 self.assertEqual(os.stat(file_name).st_mode, 0100654)
55 dir_name = os.path.join(self.sftp_server._current_upload, 'bar/foo')
56 os.makedirs(dir_name)
57 upload_file = self.sftp_server.openFile('bar/foo', None, None)
58 self.assertRaisesWithContent(
59 FileIsADirectory,
60 "File is a directory: '%s'" % dir_name,
61 upload_file.writeChunk, 0, "This is a test")
5462
55def test_suite():63def test_suite():
56 return unittest.TestLoader().loadTestsFromName(__name__)64 return unittest.TestLoader().loadTestsFromName(__name__)
5765
=== 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 @@
9 'SFTPServer',9 'SFTPServer',
10 ]10 ]
1111
12import errno
12import logging13import logging
13import os14import os
14import stat
15import tempfile15import tempfile
1616
17from twisted.conch.interfaces import ISFTPFile, ISFTPServer17from twisted.conch.interfaces import ISFTPFile, ISFTPServer
@@ -19,6 +19,7 @@
19import zope.component.event19import zope.component.event
20from zope.interface import implements20from zope.interface import implements
2121
22from lp.codehosting.sftp import FileIsADirectory
22from lp.poppy.filesystem import UploadFileSystem23from lp.poppy.filesystem import UploadFileSystem
23from lp.poppy.hooks import Hooks24from lp.poppy.hooks import Hooks
24from lp.services.sshserver.events import SFTPClosed25from lp.services.sshserver.events import SFTPClosed
@@ -36,9 +37,10 @@
36 self._fs_root = fsroot37 self._fs_root = fsroot
37 self.uploadfilesystem = UploadFileSystem(tempfile.mkdtemp())38 self.uploadfilesystem = UploadFileSystem(tempfile.mkdtemp())
38 self._current_upload = self.uploadfilesystem.rootpath39 self._current_upload = self.uploadfilesystem.rootpath
39 os.chmod(self._current_upload, stat.S_IRWXU + stat.S_IRWXG)40 os.chmod(self._current_upload, 0770)
40 self._log = logging.getLogger("poppy-sftp")41 self._log = logging.getLogger("poppy-sftp")
41 self.hook = Hooks(self._fs_root, self._log, "ubuntu", perms='g+rws')42 self.hook = Hooks(
43 self._fs_root, self._log, "ubuntu", perms='g+rws', prefix='-sftp')
42 self.hook.new_client_hook(self._current_upload, 0, 0)44 self.hook.new_client_hook(self._current_upload, 0, 0)
43 self.hook.auth_verify_hook(self._current_upload, None, None)45 self.hook.auth_verify_hook(self._current_upload, None, None)
4446
@@ -104,6 +106,7 @@
104 return106 return
105 self.hook.client_done_hook(self._current_upload, 0, 0)107 self.hook.client_done_hook(self._current_upload, 0, 0)
106108
109
107class SFTPFile:110class SFTPFile:
108111
109 implements(ISFTPFile)112 implements(ISFTPFile)
@@ -124,7 +127,7 @@
124 except OSError, e:127 except OSError, e:
125 if e.errno != errno.EISDIR:128 if e.errno != errno.EISDIR:
126 raise129 raise
127 raise FileIsADirectory(name)130 raise FileIsADirectory(self.filename)
128 os.lseek(chunk_file, offset, 0)131 os.lseek(chunk_file, offset, 0)
129 os.write(chunk_file, data)132 os.write(chunk_file, data)
130 os.close(chunk_file)133 os.close(chunk_file)