launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Information

I think I like it!  A few questions ...

Diff comments:

> diff --git a/lib/lp/scripts/tests/test_runlaunchpad.py b/lib/lp/scripts/tests/test_runlaunchpad.py
> index 5dd6977..c460b8f 100644
> --- a/lib/lp/scripts/tests/test_runlaunchpad.py
> +++ b/lib/lp/scripts/tests/test_runlaunchpad.py
> @@ -165,3 +167,27 @@ class ServersToStart(testtools.TestCase):
>  
>      def test_launchpad_systems_red(self):
>          self.assertFalse(config.launchpad.launch)
> +
> +
> +class TestAppServerStart(testtools.TestCase):

Ideally lp.testing.TestCase, if you can manage it - that produces better test IDs.  (I have an unlanded commit somewhere in my py3 branch that converts this whole file to that TestCase implementation for other reasons.)

> +    @mock.patch('lp.scripts.runlaunchpad.zope_main')
> +    @mock.patch('lp.scripts.runlaunchpad.gunicorn_main')
> +    @mock.patch('lp.scripts.runlaunchpad.make_pidfile')
> +    def test_call_correct_method(self, make_pidfile, gmain, zmain):
> +        # Makes sure zope_main or gunicorn_main is called according to
> +        # launchpad configuration.
> +        patched_cfg = mock.patch(
> +            'lp.services.config.LaunchpadConfig.use_gunicorn',
> +            new_callable=mock.PropertyMock)
> +        with patched_cfg as mock_use_gunicorn:
> +            mock_use_gunicorn.return_value = True
> +            start_launchpad([])
> +            self.assertEqual(1, gmain.call_count)
> +            self.assertEqual(0, zmain.call_count)
> +        gmain.reset_mock()
> +        zmain.reset_mock()
> +        with patched_cfg as mock_use_gunicorn:
> +            mock_use_gunicorn.return_value = False
> +            start_launchpad([])
> +            self.assertEqual(0, gmain.call_count)
> +            self.assertEqual(1, zmain.call_count)
> diff --git a/lib/lp/startwsgi.py b/lib/lp/startwsgi.py
> new file mode 100644
> index 0000000..6786263
> --- /dev/null
> +++ b/lib/lp/startwsgi.py
> @@ -0,0 +1,39 @@
> +from zope.app.wsgi import getWSGIApplication
> +from zope.app.wsgi import interfaces
> +import zope.processlifetime
> +from zope.app.publication.requestpublicationregistry import (
> +    factoryRegistry as publisher_factory_registry,
> +    )
> +from zope.interface import implementer
> +from zope.event import notify

Could you sort imports, and add the usual header from standard_template.py to this file?

> +
> +from lp.services.config import config
> +
> +
> +@implementer(interfaces.IWSGIApplication)
> +class RegistryLookupFactory(object):
> +
> +    def __init__(self, db):
> +        self._db = db
> +        self._publication_cache = {}
> +        return
> +
> +    def __call__(self, input_stream, env):
> +        factory = publisher_factory_registry.lookup('*', '*', env)
> +        request_class, publication_class = factory()
> +        publication = self._publication_cache.get(publication_class)
> +        if publication is None:
> +            publication = publication_class(self._db)
> +            self._publication_cache[publication_class] = publication
> +
> +        request = request_class(input_stream, env)
> +        request.setPublication(publication)
> +        return request

This looks roughly like zope.app.publication.httpfactory.HTTPPublicationRequestFactory, but there are some differences: in particular, this doesn't seem to honour HTTP_METHOD or CONTENT_TYPE from the environment.  Is this a problem?  If it isn't, I think some kind of comment would be useful, as it looks a bit odd to me.

> +
> +
> +application = getWSGIApplication(
> +    config.zope_config_file,
> +    requestFactory=RegistryLookupFactory
> +)
> +
> +notify(zope.processlifetime.ProcessStarting())
> diff --git a/setup.py b/setup.py
> index b5696f3..d622ecd 100644
> --- a/setup.py
> +++ b/setup.py
> @@ -233,6 +233,8 @@ setup(
>          'Sphinx',
>          'statsd',
>          'storm',
> +        'subvertpy',

I think this is a leftover from conflict resolution somewhere: subvertpy is only supposed to be used by lp-codeimport now.

> +        'talisker[gunicorn]',
>          'tenacity',
>          'testscenarios',
>          'testtools',
> @@ -333,6 +335,7 @@ setup(
>              'version-info = lp.scripts.utilities.versioninfo:main',
>              'watch_jsbuild = lp.scripts.utilities.js.watchjsbuild:main',
>              'with-xvfb = lp.scripts.utilities.withxvfb:main',
> -        ]
> +        ],
> +        wsgi=["wsigogogo = lp.startwsgi.gogogo"]

lp.startwsgi.gogogo doesn't seem to exist.  Is this a leftover from local debugging?

>      ),
>  )


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/396615
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:initial-gunicorn-setup.