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

[Paul Hummer]

[Paul Hummer (rockstar)]

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.

[Björn Tillenius (bjornt)]

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

[Francis J. Lacoste (flacoste)]

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>

=== modified file 'lib/canonical/config/__init__.py'
--- lib/canonical/config/__init__.py	2009-08-21 19:13:05 +0000
+++ lib/canonical/config/__init__.py	2010-01-14 02:06:15 +0000
@@ -141,8 +141,17 @@
         self._instance_name = instance_name
         os.environ[LPCONFIG] = instance_name
         # Need to reload the config.
+        self._invalidateConfig()
+
+    def _invalidateConfig(self):
+        """Invalidate the config, causing the config to be regenerated."""
         self._config = None
 
+    def reloadConfig(self):
+        """Reload the config."""
+        self._invalidateConfig()
+        self._getConfig()
+
     @property
     def process_name(self):
         """Return or set the current process's name to select a conf.
@@ -162,7 +171,7 @@
         """
         self._process_name = process_name
         # Need to reload the config.
-        self._config = None
+        self._invalidateConfig()
 
     def _getConfig(self):
         """Get the schema and config for this environment.

=== modified file 'lib/canonical/launchpad/webapp/vhosts.py'
--- lib/canonical/launchpad/webapp/vhosts.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/webapp/vhosts.py	2010-01-14 02:06:15 +0000
@@ -101,6 +101,11 @@
             self._hostnames.update(config.althostnames)
         self._has_vhost_data = True
 
+    def reload(self):
+        """Reload the VHost data."""
+        self._has_vhost_data = False
+        self._getVHostData()
+
     @property
     def use_https(self):
         """Do the vhosts use HTTPS?"""

=== 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 02:06:15 +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.reloadConfig()
 
     @classmethod
     @profiled

=== modified file 'lib/lp/bugs/windmill/tests/test_bug_tags_entry.py'
--- lib/lp/bugs/windmill/tests/test_bug_tags_entry.py	2009-11-08 22:43:09 +0000
+++ lib/lp/bugs/windmill/tests/test_bug_tags_entry.py	2010-01-14 02:06:15 +0000
@@ -33,9 +33,7 @@
             u'eenie', u'meenie', u'meinie', u'moe']
         bug = self.factory.makeBug(product=product)
         removeSecurityProxy(bug).tags = ['unofficial-tag']
-        # XXX Make the testing canonical_url available as a global utility
-        bug_url = canonical_url(bug).replace(
-            'https', 'http').replace('.dev/', '.dev:8085/')
+        bug_url = canonical_url(bug)
         transaction.commit()
 
 

=== modified file 'lib/lp/code/windmill/testing.py'
--- lib/lp/code/windmill/testing.py	2009-11-10 21:49:38 +0000
+++ lib/lp/code/windmill/testing.py	2010-01-14 02:06:15 +0000
@@ -9,9 +9,6 @@
     ]
 
 
-import windmill
-
-from canonical.launchpad.webapp import canonical_url as real_canonical_url
 from canonical.testing.layers import BaseWindmillLayer
 
 
@@ -19,18 +16,3 @@
     """Layer for Code Windmill tests."""
 
     base_url = 'http://code.launchpad.dev:8085/'
-
-
-# XXX: rockstar 2009-11-10  bug=480137 - This function really needs to go
-# away.  I'm going to fix this in a followup branch.
-def canonical_url(*args, **kwargs):
-    """Wrapper for canonical_url that ensures test URLs are returned.
-
-    Real canonical URLs require SSL support, but our test clients don't
-    trust the certificate.
-    """
-    url = real_canonical_url(*args, **kwargs)
-    url = url.replace(
-        'http://code.launchpad.dev', windmill.settings['TEST_URL'])
-    assert url.startswith(windmill.settings['TEST_URL'])
-    return url

=== modified file 'lib/lp/code/windmill/tests/test_branch_merge_proposal.py'
--- lib/lp/code/windmill/tests/test_branch_merge_proposal.py	2009-11-27 01:39:07 +0000
+++ lib/lp/code/windmill/tests/test_branch_merge_proposal.py	2010-01-14 02:06:15 +0000
@@ -11,10 +11,11 @@
 
 from windmill.authoring import WindmillTestClient
 
+from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.windmill.testing.constants import (
     FOR_ELEMENT, PAGE_LOAD, SLEEP)
 from canonical.launchpad.windmill.testing.lpuser import login_person
-from lp.code.windmill.testing import canonical_url, CodeWindmillLayer
+from lp.code.windmill.testing import CodeWindmillLayer
 from lp.testing import TestCaseWithFactory
 
 

=== modified file 'lib/lp/code/windmill/tests/test_merge_proposal_commenting.py'
--- lib/lp/code/windmill/tests/test_merge_proposal_commenting.py	2009-11-27 15:05:02 +0000
+++ lib/lp/code/windmill/tests/test_merge_proposal_commenting.py	2010-01-14 02:06:14 +0000
@@ -11,9 +11,10 @@
 import transaction
 from windmill.authoring import WindmillTestClient
 
+from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.windmill.testing import lpuser
 from canonical.uuid import generate_uuid
-from lp.code.windmill.testing import canonical_url, CodeWindmillLayer
+from lp.code.windmill.testing import CodeWindmillLayer
 from lp.testing import login_person, TestCaseWithFactory
 
 WAIT_PAGELOAD = u'30000'