launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> === modified file 'lib/lp/services/config/schema-lazr.conf'
> --- lib/lp/services/config/schema-lazr.conf	2015-12-02 11:27:02 +0000
> +++ lib/lp/services/config/schema-lazr.conf	2016-02-16 02:08:45 +0000
> @@ -781,6 +781,11 @@
>  # datatype: string
>  maps_api_key:
>  
> +[gpg_service]

It's called "gpgservice" on the next line!

> +# The ip_address:port we can contact the gpgservice http api.
> +# datatype: string
> +service_location:

This is classically "SOMETHING_endpoint". "api_endpoint" might make sense here.

> +
>  [gpghandler]
>  # Should we allow uploading keys to the keyserver?
>  # datatype: boolean
> 
> === added directory 'lib/lp/testing/gpgservice'
> === added file 'lib/lp/testing/gpgservice/__init__.py'
> --- lib/lp/testing/gpgservice/__init__.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/testing/gpgservice/__init__.py	2016-02-16 02:08:45 +0000
> @@ -0,0 +1,10 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the

The future is now.

> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +from __future__ import absolute_import
> +
> +from . _fixture import GPGKeyServiceFixture
> +
> +__all__ = [
> +    'GPGKeyServiceFixture'
> +    ]
> 
> === added file 'lib/lp/testing/gpgservice/_fixture.py'
> --- lib/lp/testing/gpgservice/_fixture.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/testing/gpgservice/_fixture.py	2016-02-16 02:08:45 +0000
> @@ -0,0 +1,160 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +from __future__ import absolute_import
> +
> +from ConfigParser import SafeConfigParser
> +import httplib
> +import json
> +import os.path
> +import socket
> +from StringIO import StringIO
> +import subprocess
> +from tempfile import NamedTemporaryFile
> +from textwrap import dedent
> +import time
> +
> +from fixtures import Fixture
> +
> +from lp.testing.keyserver import KeyServerTac
> +from lp.services.config import config
> +
> +__metaclass__ = type
> +
> +
> +class GPGKeyServiceFixture(Fixture):
> +
> +    """Run the gpgservice webapp and test key server."""
> +
> +    def __init__(self, config_fixture=None):
> +        """Create a new GPGKeyServiceFixture.
> +
> +        :param config_fixture: If specified, this must be a ConfigFixture
> +            instance. It will be updated with the relevant GPG service config
> +            details.
> +        """
> +        self._config_fixture = config_fixture
> +
> +    def setUp(self):
> +        super(GPGKeyServiceFixture, self).setUp()
> +        # Figure out if the keyserver is running,and if not, run it:
> +        keyserver = KeyServerTac()
> +        if not os.path.exists(keyserver.pidfile):
> +            self.useFixture(KeyServerTac())

It seems pretty weird to make this conditional.

Since tests that need the keyserver already start the keyserver themselves, and the keyserver isn't a dependency of the DB bits of the service, we could leave it out of the gpgservice fixture.

> +
> +        # Write service config to a file on disk. This file gets deleted when the
> +        # fixture ends.
> +        service_config = _get_default_service_config()
> +        self._config_file = NamedTemporaryFile()
> +        self.addCleanup(self._config_file.close)
> +        service_config.write(self._config_file)
> +        self._config_file.flush()
> +
> +        # Set the environment variable that tells gpgservice where to read it's

apostrophe

> +        # config file from:
> +        env = os.environ.copy()
> +        env['GPGSERVICE_CONFIG_PATH'] = self._config_file.name
> +
> +        gunicorn_path = os.path.join(
> +            config.root, 'bin', 'gunicorn-for-gpgservice')
> +        self.interface = '127.0.0.1'
> +        self.port = _get_unused_port()
> +        gunicorn_options = ['-b', self.bind_address]
> +        wsgi_app_name = 'gpgservice.webapp:app'
> +
> +        self._process = subprocess.Popen(
> +            args=[gunicorn_path] + gunicorn_options + [wsgi_app_name],
> +            stdout=subprocess.PIPE,
> +            stderr=subprocess.PIPE,
> +            env=env)
> +        self.addCleanup(self._kill_server)
> +        self._wait_for_service_start()
> +        self.reset_service_database()
> +        if self._config_fixture is not None:
> +            config_section = service_config = dedent("""\
> +                [gpg_service]
> +                service_location: %s
> +                """ % self.bind_address)
> +            self._config_fixture.add_section(config_section)
> +            config.reloadConfig()
> +
> +    def _kill_server(self):
> +        self._process.terminate()
> +        stdout, stderr = self._process.communicate()
> +        self.addDetail('gunicorn-stdout', stdout)

Probably "gpgservice" rather than "gunicorn". It's the only gunicorn instance at the moment, but probably not for long.

> +        self.addDetail('gunicorn-stderr', stderr)
> +
> +    def _wait_for_service_start(self):
> +        errors = []
> +        for i in range(10):
> +            conn = httplib.HTTPConnection(self.bind_address)
> +            try:
> +                conn.request('GET', '/')
> +            except socket.error as e:
> +                errors.append(e)
> +            else:
> +                resp = conn.getresponse()
> +                if resp.status == 200:
> +                    return
> +            time.sleep(0.1)
> +        raise RuntimeError("Service not responding: %r" % errors)
> +
> +    def reset_service_database(self):
> +        """Reset the gpgservice instance database to the launchpad test data set."""

The Launchpad term is "sampledata".

> +        conn = httplib.HTTPConnection(self.bind_address)
> +        test_data = {
> +            'keys': [
> +                {
> +                    'owner': 'name16_oid',
> +                    'id': '12345678',
> +                    'fingerprint': 'ABCDEF0123456789ABCDDCBA0000111112345678',
> +                    'size': 1024,
> +                    'algorithm': 'D',
> +                    'can_encrypt': True,
> +                    'enabled': True,
> +                }
> +            ]
> +        }
> +        headers = {'Content-Type': 'application/json'}
> +        conn.request('POST', '/test/reset_db', json.dumps(test_data), headers)
> +        resp = conn.getresponse()
> +        body = resp.read()
> +        if resp.status != 200:
> +            raise RuntimeError("Could not reset database: %s" % body)
> +
> +    @property
> +    def bind_address(self):
> +        return '%s:%d' % (self.interface, self.port)
> +
> +
> +def _get_default_service_config():
> +    config = SafeConfigParser()
> +    config.readfp(StringIO(dedent("""\
> +        [gpghandler]
> +        host: localhost
> +        public_host: keyserver.ubuntu.com
> +        upload_keys: True
> +        port: 11371
> +        timeout: 5.0
> +        maximum_upload_size: 16777216
> +        enable_test_endpoint: true
> +
> +        [database]
> +        type: sqlite

Does this imply sqlite in memory? That's possibly slightly implicit and dangerous (all my data is gone after I restarted my service! My load was low enough that I didn't think I needed PostgreSQL :'()

> +    """)))
> +    return config
> +
> +
> +def _get_unused_port():
> +    """Find and return an unused port
> +
> +    There is a small race condition here (between the time we allocate the
> +    port, and the time it actually gets used), but for the purposes for which
> +    this function gets used it isn't a problem in practice.
> +    """
> +    s = socket.socket()
> +    try:
> +        s.bind(('localhost', 0))
> +        return s.getsockname()[1]
> +    finally:
> +        s.close()
> 
> === added directory 'lib/lp/testing/gpgservice/tests'
> === added file 'lib/lp/testing/gpgservice/tests/__init__.py'
> === added file 'lib/lp/testing/gpgservice/tests/test_fixture.py'
> --- lib/lp/testing/gpgservice/tests/test_fixture.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/testing/gpgservice/tests/test_fixture.py	2016-02-16 02:08:45 +0000
> @@ -0,0 +1,63 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +from __future__ import absolute_import
> +
> +import json
> +import httplib
> +
> +from testtools import TestCase
> +from testtools.matchers import (
> +    Contains,
> +    HasLength,
> +    Not,
> +    PathExists,
> +)
> +
> +from lp.services.config import config
> +from lp.services.config.fixture import (
> +    ConfigFixture,
> +    ConfigUseFixture,
> +    )
> +from lp.testing.layers import BaseLayer
> +from lp.testing.gpgservice import GPGKeyServiceFixture
> +
> +
> +class GPGServiceFixtureTests(TestCase):
> +
> +    layer = BaseLayer
> +
> +    def test_fixture_writes_and_deletes_service_config_file(self):
> +        fixture = GPGKeyServiceFixture()
> +        with fixture:
> +            config_file_path = fixture._config_file.name
> +            self.assertThat(config_file_path, PathExists())
> +        self.assertThat(config_file_path, Not(PathExists()))
> +
> +    def test_fixture_starts_gpgservice(self):
> +        fixture = self.useFixture(GPGKeyServiceFixture())
> +        conn = httplib.HTTPConnection(fixture.bind_address)
> +        conn.request('GET', '/')
> +        resp = conn.getresponse()
> +        self.assertEqual(200, resp.status)
> +        self.assertEqual('gpgservice - Copyright 2015 Canonical', resp.read())

This is both out of date and fragile :)

> +
> +    def test_fixture_can_create_test_data(self):
> +        fixture = self.useFixture(GPGKeyServiceFixture())
> +        conn = httplib.HTTPConnection(fixture.bind_address)
> +        conn.request('GET', '/users/name16_oid/keys')
> +        resp = conn.getresponse()
> +        self.assertEqual(200, resp.status)
> +        data = json.loads(resp.read())
> +        self.assertThat(data, Contains('keys'))
> +        self.assertThat(data['keys'], HasLength(1))
> +
> +    def test_fixture_can_set_config_data(self):
> +        config_name = self.getUniqueString()
> +        config_fixture = self.useFixture(
> +            ConfigFixture(config_name, BaseLayer.config_fixture.instance_name))
> +        self.useFixture(ConfigUseFixture(config_name))
> +        gpg_fixture = self.useFixture(GPGKeyServiceFixture(config_fixture))
> +
> +        self.assertEqual(
> +            gpg_fixture.bind_address, config.gpg_service.service_location)


-- 
https://code.launchpad.net/~thomir/launchpad/devel-gpgservice-fixture/+merge/286101
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.