Code review comment for lp:~mwhudson/launchpad/no-hosted-area

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

On 19/04/10 16:20, Tim Penhey wrote:
> lib/lp/code/xmlrpc/codehosting.py
> in: def branchChanged(self, branch_id, stacked_on_location, last_revision_id):
> + branch.last_mirrored = datetime.datetime.now(pytz.UTC)
> should probably be using UTC_NOW
>
> lib/lp/codehosting/inmemory.py branchChanged event should use UTC_NOW too.
>
> then:
> test_branchChanged_sets_last_mirrored
> can use:
> self.assertSqlAttributeEqualsDate(
> branch, 'last_mirrored', UTC_NOW)

Yeah OK.

> def test_branchChanged_doesnt_create_scan_job_for_noop_change(self):
> # XXX Is this even the right thing to do?
> It will be with your next pipe.

Right, I'll delete the comment.

> lib/lp/codehosting/vfs/branchfs.py
> class LaunchpadServer (I think - around line 558)
> the __init__ method still refers to the authserver, also there is a
> XXX comment that I'm wondering whether we can remove it or not.

Grar, can I fix this in the later pipe that combines the two endpoints?

> lib/lp/codehosting/vfs/tests/test_branchfs.py - still refers to an
> authserver too.

This too.

> # XXX Maaaybe we could complain on stderr here?
> - Not following our XXX format, and is this something we want to do?

Nah, I'll delete the comment.

> # XXX: JonathanLange 2007-05-29: The 'chroot' line lacks a unit test.
> I don't suppose you want to add a unit test for this?

OK. It's a bit terrible though.

> General Note: we probably want to rename config.codehosting.mirrored_branches
> at some stage.

Given recent edge rollout funnies, I don't know how we'd do this :/ But
yeah, it doesn't really make sense.

Interdiff attached.

Cheers,
mwh

1=== modified file 'lib/lp/code/xmlrpc/codehosting.py'
2--- lib/lp/code/xmlrpc/codehosting.py 2010-04-19 10:45:14 +0000
3+++ lib/lp/code/xmlrpc/codehosting.py 2010-04-19 21:57:33 +0000
4@@ -23,6 +23,16 @@
5 from zope.security.proxy import removeSecurityProxy
6 from zope.security.management import endInteraction
7
8+from canonical.database.constants import UTC_NOW
9+from canonical.launchpad.validators import LaunchpadValidationError
10+from canonical.launchpad.webapp import LaunchpadXMLRPCView
11+from canonical.launchpad.webapp.authorization import check_permission
12+from canonical.launchpad.webapp.interaction import setupInteractionForPerson
13+from canonical.launchpad.webapp.interfaces import (
14+ NameLookupFailed, NotFoundError)
15+from canonical.launchpad.xmlrpc import faults
16+from canonical.launchpad.xmlrpc.helpers import return_fault
17+
18 from lp.code.errors import UnknownBranchTypeError
19 from lp.code.enums import BranchType
20 from lp.code.interfaces.branch import BranchCreationException
21@@ -38,14 +48,6 @@
22 from lp.registry.interfaces.product import NoSuchProduct
23 from lp.services.scripts.interfaces.scriptactivity import IScriptActivitySet
24 from lp.services.utils import iter_split
25-from canonical.launchpad.validators import LaunchpadValidationError
26-from canonical.launchpad.webapp import LaunchpadXMLRPCView
27-from canonical.launchpad.webapp.authorization import check_permission
28-from canonical.launchpad.webapp.interaction import setupInteractionForPerson
29-from canonical.launchpad.webapp.interfaces import (
30- NameLookupFailed, NotFoundError)
31-from canonical.launchpad.xmlrpc import faults
32-from canonical.launchpad.xmlrpc.helpers import return_fault
33
34
35 UTC = pytz.timezone('UTC')
36@@ -262,7 +264,7 @@
37 branch.mirror_status_message = (
38 'Invalid stacked on location: ' + stacked_on_location)
39 branch.stacked_on = stacked_on_branch
40- branch.last_mirrored = datetime.datetime.now(pytz.UTC)
41+ branch.last_mirrored = UTC_NOW
42 if branch.last_mirrored_id != last_revision_id:
43 branch.last_mirrored_id = last_revision_id
44 getUtility(IBranchScanJobSource).create(branch)
45
46=== modified file 'lib/lp/code/xmlrpc/tests/test_codehosting.py'
47--- lib/lp/code/xmlrpc/tests/test_codehosting.py 2010-04-19 04:06:23 +0000
48+++ lib/lp/code/xmlrpc/tests/test_codehosting.py 2010-04-19 22:09:34 +0000
49@@ -15,7 +15,6 @@
50 from zope.component import getUtility
51 from zope.security.proxy import removeSecurityProxy
52
53-from lp.codehosting.inmemory import InMemoryFrontend
54 from canonical.database.constants import UTC_NOW
55 from canonical.launchpad.ftests import ANONYMOUS, login, logout
56 from lp.services.scripts.interfaces.scriptactivity import (
57@@ -40,6 +39,8 @@
58 BranchFileSystem, BranchPuller, LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES,
59 run_with_login)
60
61+from lp.codehosting.inmemory import InMemoryFrontend
62+
63
64 UTC = pytz.timezone('UTC')
65
66@@ -739,9 +740,11 @@
67 # current time.
68 branch = self.factory.makeAnyBranch()
69 self.branchfs.branchChanged(branch.id, '', '')
70- # We can't test "now" precisely, but lets check that last_mirrored was
71- # set to _something_.
72- self.assertIsNot(None, branch.last_mirrored)
73+ if self.frontend == LaunchpadDatabaseFrontend:
74+ self.assertSqlAttributeEqualsDate(
75+ branch, 'last_mirrored', UTC_NOW)
76+ else:
77+ self.assertIs(UTC_NOW, branch.last_mirrored)
78
79 def test_branchChanged_records_bogus_stacked_on_url(self):
80 # If a bogus location is passed in as the stacked_on parameter,
81@@ -772,7 +775,7 @@
82
83 def test_branchChanged_creates_scan_job(self):
84 # branchChanged() creates a scan job for the branch.
85- if not isinstance(self.frontend, LaunchpadDatabaseFrontend):
86+ if self.frontend != LaunchpadDatabaseFrontend:
87 return
88 branch = self.factory.makeAnyBranch()
89 jobs = list(getUtility(IBranchScanJobSource).iterReady())
90@@ -782,8 +785,7 @@
91 self.assertEqual(1, len(jobs))
92
93 def test_branchChanged_doesnt_create_scan_job_for_noop_change(self):
94- # XXX Is this even the right thing to do?
95- if not isinstance(self.frontend, LaunchpadDatabaseFrontend):
96+ if self.frontend != LaunchpadDatabaseFrontend:
97 return
98 branch = self.factory.makeAnyBranch()
99 removeSecurityProxy(branch).last_mirrored_id = 'rev1'
100
101=== modified file 'lib/lp/codehosting/inmemory.py'
102--- lib/lp/codehosting/inmemory.py 2010-04-19 04:06:23 +0000
103+++ lib/lp/codehosting/inmemory.py 2010-04-19 22:04:00 +0000
104@@ -9,18 +9,18 @@
105 'XMLRPCWrapper',
106 ]
107
108-import datetime
109 import operator
110 from xmlrpclib import Fault
111
112 from bzrlib.urlutils import escape, unescape
113
114-import pytz
115-
116 from zope.component import adapter, getSiteManager
117 from zope.interface import implementer
118
119 from canonical.database.constants import UTC_NOW
120+from canonical.launchpad.validators import LaunchpadValidationError
121+from canonical.launchpad.xmlrpc import faults
122+
123 from lp.code.errors import UnknownBranchTypeError
124 from lp.code.model.branchnamespace import BranchNamespaceSet
125 from lp.code.model.branchtarget import (
126@@ -31,12 +31,10 @@
127 from lp.code.interfaces.codehosting import (
128 BRANCH_TRANSPORT, CONTROL_TRANSPORT, LAUNCHPAD_ANONYMOUS,
129 LAUNCHPAD_SERVICES)
130+from lp.code.xmlrpc.codehosting import datetime_from_tuple
131 from lp.registry.interfaces.pocket import PackagePublishingPocket
132 from lp.services.utils import iter_split
133 from lp.testing.factory import ObjectFactory
134-from canonical.launchpad.validators import LaunchpadValidationError
135-from lp.code.xmlrpc.codehosting import datetime_from_tuple
136-from canonical.launchpad.xmlrpc import faults
137
138
139 class FakeStore:
140@@ -637,7 +635,7 @@
141 branch.mirror_status_message = (
142 'Invalid stacked on location: ' + stacked_on_location)
143 branch.stacked_on = stacked_on_branch
144- branch.last_mirrored = datetime.datetime.now(pytz.UTC)
145+ branch.last_mirrored = UTC_NOW
146 if branch.last_mirrored_id != last_revision_id:
147 branch.last_mirrored_id = last_revision_id
148 return True
149
150=== modified file 'lib/lp/codehosting/vfs/branchfs.py'
151--- lib/lp/codehosting/vfs/branchfs.py 2010-04-16 00:55:24 +0000
152+++ lib/lp/codehosting/vfs/branchfs.py 2010-04-19 22:33:52 +0000
153@@ -639,7 +639,6 @@
154 # Assume it's a relative path.
155 return stacked_on_url
156 uri = URI(stacked_on_url)
157- # XXX Maaaybe we could complain on stderr here?
158 if uri.scheme not in ['http', 'bzr+ssh', 'sftp']:
159 return stacked_on_url
160 launchpad_domain = config.vhost.mainsite.hostname
161@@ -710,7 +709,6 @@
162
163 branch_url = urlutils.local_path_to_url(branch_directory)
164 branchfs_client = xmlrpclib.ServerProxy(branchfs_endpoint_url)
165- # XXX: JonathanLange 2007-05-29: The 'chroot' line lacks a unit test.
166 branch_transport = get_chrooted_transport(branch_url)
167 lp_server = LaunchpadServer(
168 BlockingProxy(branchfs_client), user_id, branch_transport,
169
170=== modified file 'lib/lp/codehosting/vfs/tests/test_branchfs.py'
171--- lib/lp/codehosting/vfs/tests/test_branchfs.py 2010-04-09 06:28:34 +0000
172+++ lib/lp/codehosting/vfs/tests/test_branchfs.py 2010-04-19 22:33:25 +0000
173@@ -17,6 +17,7 @@
174 from bzrlib.transport import (
175 get_transport, _get_protocol_handlers, register_transport, Server,
176 unregister_transport)
177+from bzrlib.transport.chroot import ChrootTransport
178 from bzrlib.transport.memory import MemoryServer, MemoryTransport
179 from bzrlib.urlutils import escape, local_path_to_url
180
181@@ -26,7 +27,7 @@
182 from lp.codehosting.vfs.branchfs import (
183 AsyncLaunchpadTransport, BranchTransportDispatch,
184 DirectDatabaseLaunchpadServer, LaunchpadInternalServer, LaunchpadServer,
185- TransportDispatch, UnknownTransportType, branch_id_to_path)
186+ TransportDispatch, UnknownTransportType, branch_id_to_path, get_lp_server)
187 from lp.codehosting.inmemory import InMemoryFrontend, XMLRPCWrapper
188 from lp.codehosting.sftp import FatLocalTransport
189 from lp.codehosting.vfs.transport import AsyncVirtualTransport
190@@ -1005,6 +1006,17 @@
191 '/%s/.bzr/goodbye.txt' % self.read_only_branch)
192
193
194+class TestGetLPServer(TestCase):
195+ """Tests for `get_lp_server`."""
196+
197+ def test_chrooting(self):
198+ # Test that get_lp_server return a server that ultimately backs onto a
199+ # ChrootTransport.
200+ lp_server = get_lp_server(1, 'http://xmlrpc.example.invalid', '')
201+ transport = lp_server._transport_dispatch._rw_dispatch.base_transport
202+ self.assertIsInstance(transport, ChrootTransport)
203+
204+
205 def test_suite():
206 return unittest.TestLoader().loadTestsFromName(__name__)
207

« Back to merge proposal