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
1=== modified file 'lib/lp/codehosting/sftp.py'
2--- lib/lp/codehosting/sftp.py 2010-04-19 10:56:23 +0000
3+++ lib/lp/codehosting/sftp.py 2010-05-28 13:44:40 +0000
4@@ -16,6 +16,7 @@
5 __all__ = [
6 'avatar_to_sftp_server',
7 'TransportSFTPServer',
8+ 'FileIsADirectory',
9 ]
10
11
12
13=== modified file 'lib/lp/poppy/hooks.py'
14--- lib/lp/poppy/hooks.py 2010-03-17 11:43:01 +0000
15+++ lib/lp/poppy/hooks.py 2010-05-28 13:44:40 +0000
16@@ -21,13 +21,14 @@
17 clients = {}
18
19 def __init__(self, targetpath, logger, allow_user, cmd=None,
20- targetstart=0, perms=None):
21+ targetstart=0, perms=None, prefix=''):
22 self.targetpath = targetpath
23 self.logger = logging.getLogger("%s.Hooks" % logger.name)
24 self.cmd = cmd
25 self.allow_user = allow_user
26 self.targetcount = targetstart
27 self.perms = perms
28+ self.prefix = prefix
29
30 def new_client_hook(self, fsroot, host, port):
31 """Prepare a new client record indexed by fsroot..."""
32@@ -81,7 +82,8 @@
33 try:
34 self.targetcount += 1
35 timestamp = time.strftime("%Y%m%d-%H%M%S")
36- path = "upload-%s-%06d" % (timestamp, self.targetcount)
37+ path = "upload%s-%s-%06d" % (
38+ self.prefix, timestamp, self.targetcount)
39 target_fsroot = os.path.join(self.targetpath, path)
40
41 # Create file to store the distro used.
42
43=== modified file 'lib/lp/poppy/tests/test_poppy.py'
44--- lib/lp/poppy/tests/test_poppy.py 2010-05-27 07:11:05 +0000
45+++ lib/lp/poppy/tests/test_poppy.py 2010-05-28 13:44:40 +0000
46@@ -133,7 +133,9 @@
47 pass
48
49 def waitForClose(self):
50- # XXX: Eww
51+ # XXX: Steve Kowalik 2010-05-24 bug=586695 There has to be a
52+ # better way to wait for the SFTP server to process our upload
53+ # rather than sleeping for 10 seconds.
54 time.sleep(10)
55
56 def getTransport(self):
57@@ -278,7 +280,13 @@
58 self.server.disconnect(transport)
59 self.server.waitForClose()
60
61- self.assertEqual(os.stat(self._uploadPath('')).st_mode, 042770)
62+ upload_path = self._uploadPath('')
63+ self.assertEqual(os.stat(upload_path).st_mode, 042770)
64+ dir_name = upload_path.split('/')[-2]
65+ if transport._user == 'joe':
66+ self.assertEqual(dir_name.startswith('upload-sftp-2'), True)
67+ elif transport._user == 'ubuntu':
68+ self.assertEqual(dir_name.startswith('upload-2'), True)
69 for upload in files:
70 wanted_path = self._uploadPath(
71 "~ppa-user/ppa/ubuntu/%s" % upload)
72
73=== modified file 'lib/lp/poppy/tests/test_twistedsftp.py'
74--- lib/lp/poppy/tests/test_twistedsftp.py 2010-05-27 07:11:05 +0000
75+++ lib/lp/poppy/tests/test_twistedsftp.py 2010-05-28 13:44:40 +0000
76@@ -9,16 +9,17 @@
77 import tempfile
78 import unittest
79
80-from twisted.trial.unittest import TestCase as TrialTestCase
81-
82+from lp.codehosting.sftp import FileIsADirectory
83 from lp.poppy.twistedsftp import SFTPServer
84-
85-
86-class TestSFTPServer(TrialTestCase):
87+from lp.testing import TestCase
88+
89+
90+class TestSFTPServer(TestCase):
91
92 def setUp(self):
93 self.fs_root = tempfile.mkdtemp()
94 self.sftp_server = SFTPServer(None, self.fs_root)
95+ super(TestSFTPServer, self).setUp()
96
97 def test_gotVersion(self):
98 # gotVersion always returns an empty dict, since the server does not
99@@ -51,6 +52,13 @@
100 self.assertEqual(test_file.read(), "This is a test")
101 test_file.close()
102 self.assertEqual(os.stat(file_name).st_mode, 0100654)
103+ dir_name = os.path.join(self.sftp_server._current_upload, 'bar/foo')
104+ os.makedirs(dir_name)
105+ upload_file = self.sftp_server.openFile('bar/foo', None, None)
106+ self.assertRaisesWithContent(
107+ FileIsADirectory,
108+ "File is a directory: '%s'" % dir_name,
109+ upload_file.writeChunk, 0, "This is a test")
110
111 def test_suite():
112 return unittest.TestLoader().loadTestsFromName(__name__)
113
114=== modified file 'lib/lp/poppy/twistedsftp.py'
115--- lib/lp/poppy/twistedsftp.py 2010-05-27 07:07:24 +0000
116+++ lib/lp/poppy/twistedsftp.py 2010-05-28 13:44:40 +0000
117@@ -9,9 +9,9 @@
118 'SFTPServer',
119 ]
120
121+import errno
122 import logging
123 import os
124-import stat
125 import tempfile
126
127 from twisted.conch.interfaces import ISFTPFile, ISFTPServer
128@@ -19,6 +19,7 @@
129 import zope.component.event
130 from zope.interface import implements
131
132+from lp.codehosting.sftp import FileIsADirectory
133 from lp.poppy.filesystem import UploadFileSystem
134 from lp.poppy.hooks import Hooks
135 from lp.services.sshserver.events import SFTPClosed
136@@ -36,9 +37,10 @@
137 self._fs_root = fsroot
138 self.uploadfilesystem = UploadFileSystem(tempfile.mkdtemp())
139 self._current_upload = self.uploadfilesystem.rootpath
140- os.chmod(self._current_upload, stat.S_IRWXU + stat.S_IRWXG)
141+ os.chmod(self._current_upload, 0770)
142 self._log = logging.getLogger("poppy-sftp")
143- self.hook = Hooks(self._fs_root, self._log, "ubuntu", perms='g+rws')
144+ self.hook = Hooks(
145+ self._fs_root, self._log, "ubuntu", perms='g+rws', prefix='-sftp')
146 self.hook.new_client_hook(self._current_upload, 0, 0)
147 self.hook.auth_verify_hook(self._current_upload, None, None)
148
149@@ -104,6 +106,7 @@
150 return
151 self.hook.client_done_hook(self._current_upload, 0, 0)
152
153+
154 class SFTPFile:
155
156 implements(ISFTPFile)
157@@ -124,7 +127,7 @@
158 except OSError, e:
159 if e.errno != errno.EISDIR:
160 raise
161- raise FileIsADirectory(name)
162+ raise FileIsADirectory(self.filename)
163 os.lseek(chunk_file, offset, 0)
164 os.write(chunk_file, data)
165 os.close(chunk_file)