launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve

Looks generally sensible, although there are a few things I'd suggest tweaking before landing this.  Are you intending to have the charm write out a config.yaml or to have it set environment variables?

Diff comments:

> === modified file '.bzrignore'
> --- .bzrignore	2015-01-19 08:58:30 +0000
> +++ .bzrignore	2015-02-17 21:40:52 +0000
> @@ -10,3 +10,4 @@
>  build
>  *.egg
>  dist
> +*.log
> \ No newline at end of file
> 
> === added file 'apiserver.tac'
> --- apiserver.tac	1970-01-01 00:00:00 +0000
> +++ apiserver.tac	2015-02-17 21:40:52 +0000
> @@ -0,0 +1,25 @@
> +# You can run this .tac file directly with:
> +#    twistd -ny apiserver.tac
> +from __future__ import (
> +    absolute_import,
> +    print_function,
> +    unicode_literals,
> +    )
> +
> +from twisted.web import server
> +from twisted.application import service, internet

The Launchpad style guide would have this as:

from twisted.application import (
    service,
    internet,
    )

We should probably follow the same style in turnip throughout.

> +
> +from turnip.api import TurnipAPIResource
> +from turnip.config import TurnipConfig
> +
> +
> +def getAPIService():
> +    """Return an API service."""
> +
> +    config = TurnipConfig()
> +    api_site = server.Site(TurnipAPIResource(config.get('REPO_STORE')))
> +    return internet.TCPServer(config.get('REPO_API_PORT'), api_site)
> +
> +application = service.Application("Turnip API Service")
> +service = getAPIService()
> +service.setServiceParent(application)
> 
> === added file 'config.yaml'
> --- config.yaml	1970-01-01 00:00:00 +0000
> +++ config.yaml	2015-02-17 21:40:52 +0000
> @@ -0,0 +1,9 @@
> +AUTHENTICATION_ENDPOINT: http://xmlrpc-private.launchpad.dev:8087/authserver
> +PACK_BACKEND_PORT: 19418
> +PACK_FRONTEND_PORT: 9418
> +PACK_VIRT_PORT: 19419
> +REPO_API_PORT: 19417
> +SMART_HTTP_PORT: 9419
> +SMART_SSH_PORT: 9422
> +REPO_STORE: /var/tmp/git.launchpad.dev
> +VIRTINFO_ENDPOINT: http://localhost:6543/githosting
> \ No newline at end of file

Maybe downcase all these; YAML style usually tends towards lower-case, I think, and it matches juju style better too.  This would probably entail having TurnipConfig upper-case key names before checking for them in the environment.

> 
> === added file 'httpserver.tac'
> --- httpserver.tac	1970-01-01 00:00:00 +0000
> +++ httpserver.tac	2015-02-17 21:40:52 +0000
> @@ -0,0 +1,24 @@
> +# You can run this .tac file directly with:
> +#    twistd -ny httpserver.tac
> +from __future__ import unicode_literals
> +
> +from twisted.application import service, internet
> +from twisted.web import server
> +
> +from turnip.config import TurnipConfig
> +from turnip.pack.http import SmartHTTPFrontendResource
> +
> +
> +def getSmartHTTPService():
> +    """Return a SmartHTTP frontend service."""
> +
> +    config = TurnipConfig()
> +    smarthttp_site = server.Site(
> +        SmartHTTPFrontendResource(b'localhost',
> +                                  config.get('PACK_VIRT_PORT'),
> +                                  config.get('VIRTINFO_ENDPOINT')))
> +    return internet.TCPServer(config.get('SMART_HTTP_PORT'), smarthttp_site)
> +
> +application = service.Application("Turnip SmartHTTP Service")
> +service = getSmartHTTPService()
> +service.setServiceParent(application)
> 
> === added file 'packbackendserver.tac'
> --- packbackendserver.tac	1970-01-01 00:00:00 +0000
> +++ packbackendserver.tac	2015-02-17 21:40:52 +0000
> @@ -0,0 +1,25 @@
> +# You can run this .tac file directly with:
> +#    twistd -ny packbackendserver.tac
> +
> +from __future__ import (
> +    absolute_import,
> +    print_function,
> +    unicode_literals,
> +    )
> +
> +from twisted.application import service, internet
> +
> +from turnip.config import TurnipConfig
> +from turnip.pack.git import PackBackendFactory
> +
> +
> +def getPackBackendService():
> +    """Return a PackBackendFactory service."""
> +
> +    config = TurnipConfig()
> +    return internet.TCPServer(config.get('PACK_BACKEND_PORT'),
> +                              PackBackendFactory(config.get('REPO_STORE')))
> +
> +application = service.Application("Turnip Pack Backend Service")
> +service = getPackBackendService()
> +service.setServiceParent(application)
> 
> === added file 'packfrontendserver.tac'
> --- packfrontendserver.tac	1970-01-01 00:00:00 +0000
> +++ packfrontendserver.tac	2015-02-17 21:40:52 +0000
> @@ -0,0 +1,27 @@
> +# You can run this .tac file directly with:
> +#    twistd -ny packfrontendserver.tac
> +
> +from __future__ import (
> +    absolute_import,
> +    print_function,
> +    unicode_literals,
> +    )
> +
> +from twisted.application import service, internet
> +
> +from turnip.config import TurnipConfig
> +from turnip.pack.git import PackFrontendFactory
> +
> +
> +def getPackFrontendService():
> +    """Return a PackFrontend Service."""
> +
> +    config = TurnipConfig()
> +    return internet.TCPServer(
> +        config.get('PACK_FRONTEND_PORT'),
> +        PackFrontendFactory('localhost',
> +                            config.get('PACK_VIRT_PORT')))
> +
> +application = service.Application("Turnip Pack Frontend Service")
> +service = getPackFrontendService()
> +service.setServiceParent(application)
> 
> === modified file 'setup.py'
> --- setup.py	2015-01-19 09:51:28 +0000
> +++ setup.py	2015-02-17 21:40:52 +0000
> @@ -26,6 +26,7 @@
>      download_url='https://launchpad.net/turnip/+download',
>      install_requires=[
>          'lazr.sshserver',
> +        'PyYAML',
>          'Twisted',
>          'zope.interface',
>          ],
> 
> === added file 'sshserver.tac'
> --- sshserver.tac	1970-01-01 00:00:00 +0000
> +++ sshserver.tac	2015-02-17 21:40:52 +0000
> @@ -0,0 +1,37 @@
> +# You can run this .tac file directly with:
> +#    twistd -ny sshserver.tac
> +
> +from __future__ import (
> +    absolute_import,
> +    print_function,
> +    unicode_literals,
> +    )
> +
> +import os
> +
> +from twisted.application import service
> +
> +from turnip.config import TurnipConfig
> +from turnip.pack.ssh import SmartSSHService
> +
> +
> +def getSmartSSHService():
> +
> +    config = TurnipConfig()
> +    data_dir = os.path.join(
> +        os.path.dirname(__file__), "turnip", "pack", "tests", "data")
> +    log_path = config.get('TURNIP_LOG_DIR')

TURNIP_LOG_DIR doesn't seem to be in config.yaml.  I know TurnipConfig falls back to the empty string, but it should probably be in config.yaml anyway for explicitness.

> +
> +    return SmartSSHService(
> +        b'localhost', config.get('PACK_VIRT_PORT'),
> +        config.get('AUTHENTICATION_ENDPOINT'),
> +        private_key_path=os.path.join(data_dir, "ssh-host-key"),
> +        public_key_path=os.path.join(data_dir, "ssh-host-key.pub"),
> +        main_log='turnip', access_log=os.path.join(log_path, 'turnip.access'),
> +        access_log_path=os.path.join(log_path, 'turnip-access.log'),
> +        strport=b'tcp:{}'.format(config.get('SMART_SSH_PORT')))
> +
> +
> +application = service.Application("Turnip SmartSSH Service")
> +service = getSmartSSHService()
> +service.setServiceParent(application)
> 
> === added file 'turnip/config.py'
> --- turnip/config.py	1970-01-01 00:00:00 +0000
> +++ turnip/config.py	2015-02-17 21:40:52 +0000
> @@ -0,0 +1,18 @@
> +from __future__ import unicode_literals
> +
> +import os
> +
> +import yaml
> +
> +
> +class TurnipConfig(object):
> +    """Return configuration from environment or defaults."""
> +
> +    def __init__(self):
> +        """Load default configuration from config.yaml"""
> +        config_file = open(os.path.join(
> +            os.path.dirname(os.path.dirname(__file__)), 'config.yaml'))
> +        self.defaults = yaml.load(config_file)
> +
> +    def get(self, key):
> +        return os.getenv(key) or self.defaults.get(key) or ''
> 
> === modified file 'turnipserver.py'
> --- turnipserver.py	2015-02-04 10:28:19 +0000
> +++ turnipserver.py	2015-02-17 21:40:52 +0000
> @@ -10,6 +10,7 @@
>  from twisted.web import server
>  
>  from turnip.api import TurnipAPIResource
> +from turnip.config import TurnipConfig
>  from turnip.pack.git import (
>      PackBackendFactory,
>      PackFrontendFactory,
> @@ -18,34 +19,44 @@
>  from turnip.pack.http import SmartHTTPFrontendResource
>  from turnip.pack.ssh import SmartSSHService
>  
> -AUTHENTICATION_ENDPOINT = (
> -    b'http://xmlrpc-private.launchpad.dev:8087/authserver')
> -LOG_PATH = os.getenv('TURNIP_LOG_DIR', '')
> -REPO_STORE = '/var/tmp/git.launchpad.dev'
> -VIRTINFO_ENDPOINT = b'http://localhost:6543/githosting'
>  data_dir = os.path.join(
>      os.path.dirname(__file__), "turnip", "pack", "tests", "data")
> -
> +config = TurnipConfig()
> +
> +LOG_PATH = config.get('TURNIP_LOG_DIR')
> +PACK_VIRT_PORT = config.get('PACK_VIRT_PORT')
> +PACK_BACKEND_PORT = config.get('PACK_BACKEND_PORT')
> +REPO_STORE = config.get('REPO_STORE')
> +VIRTINFO_ENDPOINT = config.get('VIRTINFO_ENDPOINT')
> +
> +# turnipserver.py is preserved for convenience in development, services
> +# in production are run in separate processes.
> +#
>  # Start a pack storage service on 19418, pointed at by a pack frontend
>  # on 9418 (the default git:// port), a smart HTTP frontend on 9419, and
>  # a smart SSH frontend on 9422.  An API service runs on 19417.
> -reactor.listenTCP(19418, PackBackendFactory(REPO_STORE))
> -reactor.listenTCP(
> -    19419, PackVirtFactory('localhost', 19418, VIRTINFO_ENDPOINT))
> -reactor.listenTCP(9418, PackFrontendFactory('localhost', 19419))
> +reactor.listenTCP(PACK_BACKEND_PORT,
> +                  PackBackendFactory(REPO_STORE))
> +reactor.listenTCP(PACK_VIRT_PORT,
> +                  PackVirtFactory('localhost',
> +                                  PACK_BACKEND_PORT,
> +                                  VIRTINFO_ENDPOINT))
> +reactor.listenTCP(config.get('PACK_FRONTEND_PORT'),
> +                  PackFrontendFactory('localhost',
> +                                      PACK_VIRT_PORT))
>  smarthttp_site = server.Site(
> -    SmartHTTPFrontendResource(b'localhost', 19419, VIRTINFO_ENDPOINT))
> -reactor.listenTCP(9419, smarthttp_site)
> +    SmartHTTPFrontendResource(b'localhost', PACK_VIRT_PORT, VIRTINFO_ENDPOINT))
> +reactor.listenTCP(config.get('SMART_HTTP_PORT'), smarthttp_site)
>  smartssh_service = SmartSSHService(
> -    b'localhost', 19419, AUTHENTICATION_ENDPOINT,
> +    b'localhost', 19419, config.get('AUTHENTICATION_ENDPOINT'),

19419 should be unhardcoded too.

>      private_key_path=os.path.join(data_dir, "ssh-host-key"),
>      public_key_path=os.path.join(data_dir, "ssh-host-key.pub"),
>      main_log='turnip', access_log=os.path.join(LOG_PATH, 'turnip.access'),
>      access_log_path=os.path.join(LOG_PATH, 'turnip-access.log'),
> -    strport=b'tcp:9422')
> +    strport=b'tcp:{}'.format(config.get('SMART_SSH_PORT')))
>  smartssh_service.startService()
>  
>  api_site = server.Site(TurnipAPIResource(REPO_STORE))
> -reactor.listenTCP(19417, api_site)
> +reactor.listenTCP(config.get('REPO_API_PORT'), api_site)
>  
>  reactor.run()
> 
> === modified file 'versions.cfg'
> --- versions.cfg	2015-01-22 03:16:48 +0000
> +++ versions.cfg	2015-02-17 21:40:52 +0000
> @@ -12,6 +12,7 @@
>  pyasn1 = 0.1.6
>  pycrypto = 2.6
>  python-mimeparse = 0.1.4
> +PyYAML = 3.11
>  setuptools = 0.6c11
>  testtools = 0.9.30
>  Twisted = 13.0.0

You probably need to change requirements.txt similarly, for pip.

> 
> === added file 'virtserver.tac'
> --- virtserver.tac	1970-01-01 00:00:00 +0000
> +++ virtserver.tac	2015-02-17 21:40:52 +0000
> @@ -0,0 +1,25 @@
> +from __future__ import (
> +    absolute_import,
> +    print_function,
> +    unicode_literals,
> +    )
> +
> +from twisted.application import service, internet
> +
> +from turnip.config import TurnipConfig
> +from turnip.pack.git import PackVirtFactory
> +
> +
> +def getPackVirtService():
> +    """Return a PackVirt Service."""
> +
> +    config = TurnipConfig()
> +    return internet.TCPServer(
> +        config.get('PACK_VIRT_PORT'),
> +        PackVirtFactory('localhost',
> +                        config.get('PACK_BACKEND_PORT'),
> +                        config.get('VIRTINFO_ENDPOINT')))
> +
> +application = service.Application("Turnip Pack Virt Service")
> +service = getPackVirtService()
> +service.setServiceParent(application)
> 


-- 
https://code.launchpad.net/~blr/turnip/turnip-config/+merge/250076
Your team Launchpad code reviewers is subscribed to branch lp:turnip.