Launchpad itself

Merge lp:~rockstar/launchpad/fix-windmill-canonicalurl into lp:launchpad

  1. fix-windmill-canonicalurl
  2. Merge into devel
Proposed by Paul Hummer
Status: Merged
Merged at revision: not available
Proposed branch: lp:~rockstar/launchpad/fix-windmill-canonicalurl
Merge into: lp:launchpad
Diff against target: 168 lines (+36/-24)
7 files modified
lib/canonical/config/__init__.py (+10/-1)
lib/canonical/launchpad/webapp/vhosts.py (+5/-0)
lib/canonical/testing/layers.py (+16/-0)
lib/lp/bugs/windmill/tests/test_bug_tags_entry.py (+1/-3)
lib/lp/code/windmill/testing.py (+0/-18)
lib/lp/code/windmill/tests/test_branch_merge_proposal.py (+2/-1)
lib/lp/code/windmill/tests/test_merge_proposal_commenting.py (+2/-1)
To merge this branch: bzr merge lp:~rockstar/launchpad/fix-windmill-canonicalurl
Reviewer Review Type Date Requested Status
Björn Tillenius (community) Approve
Review via email: mp+17328@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Paul Hummer (rockstar) wrote :

This branch fixes bug #504479 - Basically, using canonical_url in the windmill
layers didn't work because of lack of http and needing to specify the port.
This works, and all the windmill tests still pass.

Revision history for this message
Björn Tillenius (bjornt) wrote :

On Thu, Jan 14, 2010 at 12:10:25AM -0000, Paul Hummer wrote:
> === modified file 'lib/canonical/testing/layers.py'
> --- lib/canonical/testing/layers.py 2009-12-16 09:53:34 +0000
> +++ lib/canonical/testing/layers.py 2010-01-14 00:10:24 +0000
> @@ -89,6 +89,7 @@
> from zope.server.logger.pythonlogger import PythonLogger
> from zope.testing.testrunner.runner import FakeInputContinueGenerator
>
> +from canonical.launchpad.webapp.vhosts import allvhosts
> from canonical.lazr import pidfile
> from canonical.config import CanonicalConfig, config, dbconfig
> from canonical.database.revision import (
> @@ -1832,6 +1833,20 @@
> os.environ['WINDMILL_CONFIG_FILE'] = cls.config_file.name
> cls.shell_objects = start_windmill()
>
> + # Patch the config to provide the port number and not use https.
> + sites = (
> + ('vhost.mainsite', 'rooturl: http://launchpad.dev:8085/'),
> + ('vhost.answers', 'rooturl: http://answers.launchpad.dev:8085/'),
> + ('vhost.blueprints',
> + 'rooturl: http://blueprints.launchpad.dev:8085/'),
> + ('vhost.bugs', 'rooturl: http://bugs.launchpad.dev:8085/'),
> + ('vhost.code', 'rooturl: http://code.launchpad.dev:8085/'),
> + ('vhost.translations',
> + 'rooturl: http://translations.launchpad.dev:8085/'))
> + for site in sites:
> + config.push('windmillsettings', "\n[%s]\n%s\n" % site)
> + allvhosts.reload()
> +
> @classmethod
> @profiled
> def tearDown(cls):
> @@ -1840,6 +1855,7 @@
> if cls.config_file is not None:
> # Close the file so that it gets deleted.
> cls.config_file.close()
> + config.pop('windmillsettings')

As discussed in person, this looked a bit suspicious to me. I'm not sure
it will remove all the config section that were added. Let's confirm
that this actually works. Everything else looks good.

    review approve

--
Björn Tillenius | https://launchpad.net/~bjornt

review: Approve
Revision history for this message
Francis J. Lacoste (flacoste) wrote :

On January 13, 2010, Björn Tillenius wrote:
> Review: Approve
>
> On Thu, Jan 14, 2010 at 12:10:25AM -0000, Paul Hummer wrote:
> > === modified file 'lib/canonical/testing/layers.py'
> > --- lib/canonical/testing/layers.py 2009-12-16 09:53:34 +0000
> > +++ lib/canonical/testing/layers.py 2010-01-14 00:10:24 +0000
> > @@ -89,6 +89,7 @@
> > from zope.server.logger.pythonlogger import PythonLogger
> > from zope.testing.testrunner.runner import FakeInputContinueGenerator
> >
> > +from canonical.launchpad.webapp.vhosts import allvhosts
> > from canonical.lazr import pidfile
> > from canonical.config import CanonicalConfig, config, dbconfig
> > from canonical.database.revision import (
> > @@ -1832,6 +1833,20 @@
> > os.environ['WINDMILL_CONFIG_FILE'] = cls.config_file.name
> > cls.shell_objects = start_windmill()
> >
> > + # Patch the config to provide the port number and not use https.
> > + sites = (
> > + ('vhost.mainsite', 'rooturl: http://launchpad.dev:8085/'),
> > + ('vhost.answers', 'rooturl:
> > http://answers.launchpad.dev:8085/'), + ('vhost.blueprints',
> > + 'rooturl: http://blueprints.launchpad.dev:8085/'),
> > + ('vhost.bugs', 'rooturl: http://bugs.launchpad.dev:8085/'),
> > + ('vhost.code', 'rooturl: http://code.launchpad.dev:8085/'),
> > + ('vhost.translations',
> > + 'rooturl: http://translations.launchpad.dev:8085/'))
> > + for site in sites:
> > + config.push('windmillsettings', "\n[%s]\n%s\n" % site)
> > + allvhosts.reload()
> > +
> > @classmethod
> > @profiled
> > def tearDown(cls):
> > @@ -1840,6 +1855,7 @@
> > if cls.config_file is not None:
> > # Close the file so that it gets deleted.
> > cls.config_file.close()
> > + config.pop('windmillsettings')
>
> As discussed in person, this looked a bit suspicious to me. I'm not sure
> it will remove all the config section that were added. Let's confirm
> that this actually works. Everything else looks good.
>
> review approve
>

It won't. Only the last one added. You should either push all the config in
one go:

config.push("windmillsettings", """
[vhost.bugs]
rooturl: ...

[vhost.translations]
rooturl: ...
...
""")

Or start by pushing a maker config.push("windmillsettings-marker", "") and
then rewing the config stack to that point using config.pop("windmillsettings-
marker")

I think the former is better, but you may disagree.

--
Francis J. Lacoste
<email address hidden>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/config/__init__.py'
2--- lib/canonical/config/__init__.py 2009-08-21 19:13:05 +0000
3+++ lib/canonical/config/__init__.py 2010-01-14 02:06:15 +0000
4@@ -141,8 +141,17 @@
5 self._instance_name = instance_name
6 os.environ[LPCONFIG] = instance_name
7 # Need to reload the config.
8+ self._invalidateConfig()
9+
10+ def _invalidateConfig(self):
11+ """Invalidate the config, causing the config to be regenerated."""
12 self._config = None
13
14+ def reloadConfig(self):
15+ """Reload the config."""
16+ self._invalidateConfig()
17+ self._getConfig()
18+
19 @property
20 def process_name(self):
21 """Return or set the current process's name to select a conf.
22@@ -162,7 +171,7 @@
23 """
24 self._process_name = process_name
25 # Need to reload the config.
26- self._config = None
27+ self._invalidateConfig()
28
29 def _getConfig(self):
30 """Get the schema and config for this environment.
31
32=== modified file 'lib/canonical/launchpad/webapp/vhosts.py'
33--- lib/canonical/launchpad/webapp/vhosts.py 2009-06-25 05:30:52 +0000
34+++ lib/canonical/launchpad/webapp/vhosts.py 2010-01-14 02:06:15 +0000
35@@ -101,6 +101,11 @@
36 self._hostnames.update(config.althostnames)
37 self._has_vhost_data = True
38
39+ def reload(self):
40+ """Reload the VHost data."""
41+ self._has_vhost_data = False
42+ self._getVHostData()
43+
44 @property
45 def use_https(self):
46 """Do the vhosts use HTTPS?"""
47
48=== modified file 'lib/canonical/testing/layers.py'
49--- lib/canonical/testing/layers.py 2009-12-16 09:53:34 +0000
50+++ lib/canonical/testing/layers.py 2010-01-14 02:06:15 +0000
51@@ -89,6 +89,7 @@
52 from zope.server.logger.pythonlogger import PythonLogger
53 from zope.testing.testrunner.runner import FakeInputContinueGenerator
54
55+from canonical.launchpad.webapp.vhosts import allvhosts
56 from canonical.lazr import pidfile
57 from canonical.config import CanonicalConfig, config, dbconfig
58 from canonical.database.revision import (
59@@ -1832,6 +1833,20 @@
60 os.environ['WINDMILL_CONFIG_FILE'] = cls.config_file.name
61 cls.shell_objects = start_windmill()
62
63+ # Patch the config to provide the port number and not use https.
64+ sites = (
65+ ('vhost.mainsite', 'rooturl: http://launchpad.dev:8085/'),
66+ ('vhost.answers', 'rooturl: http://answers.launchpad.dev:8085/'),
67+ ('vhost.blueprints',
68+ 'rooturl: http://blueprints.launchpad.dev:8085/'),
69+ ('vhost.bugs', 'rooturl: http://bugs.launchpad.dev:8085/'),
70+ ('vhost.code', 'rooturl: http://code.launchpad.dev:8085/'),
71+ ('vhost.translations',
72+ 'rooturl: http://translations.launchpad.dev:8085/'))
73+ for site in sites:
74+ config.push('windmillsettings', "\n[%s]\n%s\n" % site)
75+ allvhosts.reload()
76+
77 @classmethod
78 @profiled
79 def tearDown(cls):
80@@ -1840,6 +1855,7 @@
81 if cls.config_file is not None:
82 # Close the file so that it gets deleted.
83 cls.config_file.close()
84+ config.reloadConfig()
85
86 @classmethod
87 @profiled
88
89=== modified file 'lib/lp/bugs/windmill/tests/test_bug_tags_entry.py'
90--- lib/lp/bugs/windmill/tests/test_bug_tags_entry.py 2009-11-08 22:43:09 +0000
91+++ lib/lp/bugs/windmill/tests/test_bug_tags_entry.py 2010-01-14 02:06:15 +0000
92@@ -33,9 +33,7 @@
93 u'eenie', u'meenie', u'meinie', u'moe']
94 bug = self.factory.makeBug(product=product)
95 removeSecurityProxy(bug).tags = ['unofficial-tag']
96- # XXX Make the testing canonical_url available as a global utility
97- bug_url = canonical_url(bug).replace(
98- 'https', 'http').replace('.dev/', '.dev:8085/')
99+ bug_url = canonical_url(bug)
100 transaction.commit()
101
102
103
104=== modified file 'lib/lp/code/windmill/testing.py'
105--- lib/lp/code/windmill/testing.py 2009-11-10 21:49:38 +0000
106+++ lib/lp/code/windmill/testing.py 2010-01-14 02:06:15 +0000
107@@ -9,9 +9,6 @@
108 ]
109
110
111-import windmill
112-
113-from canonical.launchpad.webapp import canonical_url as real_canonical_url
114 from canonical.testing.layers import BaseWindmillLayer
115
116
117@@ -19,18 +16,3 @@
118 """Layer for Code Windmill tests."""
119
120 base_url = 'http://code.launchpad.dev:8085/'
121-
122-
123-# XXX: rockstar 2009-11-10 bug=480137 - This function really needs to go
124-# away. I'm going to fix this in a followup branch.
125-def canonical_url(*args, **kwargs):
126- """Wrapper for canonical_url that ensures test URLs are returned.
127-
128- Real canonical URLs require SSL support, but our test clients don't
129- trust the certificate.
130- """
131- url = real_canonical_url(*args, **kwargs)
132- url = url.replace(
133- 'http://code.launchpad.dev', windmill.settings['TEST_URL'])
134- assert url.startswith(windmill.settings['TEST_URL'])
135- return url
136
137=== modified file 'lib/lp/code/windmill/tests/test_branch_merge_proposal.py'
138--- lib/lp/code/windmill/tests/test_branch_merge_proposal.py 2009-11-27 01:39:07 +0000
139+++ lib/lp/code/windmill/tests/test_branch_merge_proposal.py 2010-01-14 02:06:15 +0000
140@@ -11,10 +11,11 @@
141
142 from windmill.authoring import WindmillTestClient
143
144+from canonical.launchpad.webapp import canonical_url
145 from canonical.launchpad.windmill.testing.constants import (
146 FOR_ELEMENT, PAGE_LOAD, SLEEP)
147 from canonical.launchpad.windmill.testing.lpuser import login_person
148-from lp.code.windmill.testing import canonical_url, CodeWindmillLayer
149+from lp.code.windmill.testing import CodeWindmillLayer
150 from lp.testing import TestCaseWithFactory
151
152
153
154=== modified file 'lib/lp/code/windmill/tests/test_merge_proposal_commenting.py'
155--- lib/lp/code/windmill/tests/test_merge_proposal_commenting.py 2009-11-27 15:05:02 +0000
156+++ lib/lp/code/windmill/tests/test_merge_proposal_commenting.py 2010-01-14 02:06:14 +0000
157@@ -11,9 +11,10 @@
158 import transaction
159 from windmill.authoring import WindmillTestClient
160
161+from canonical.launchpad.webapp import canonical_url
162 from canonical.launchpad.windmill.testing import lpuser
163 from canonical.uuid import generate_uuid
164-from lp.code.windmill.testing import canonical_url, CodeWindmillLayer
165+from lp.code.windmill.testing import CodeWindmillLayer
166 from lp.testing import login_person, TestCaseWithFactory
167
168 WAIT_PAGELOAD = u'30000'