launchpad-reviewers team mailing list archive

[Bayard 'kit' Randel <kit.randel@xxxxxxxxxxxxx>]

The intention was to have the charm set environment variables (maintaining
some agnosticism about turnip itself), but provide defaults in config.yaml.


On Wed, Feb 18, 2015 at 11:53 AM, Colin Watson <cjwatson@xxxxxxxxxxxxx>
wrote:

> 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
> You are the owner of lp:~blr/turnip/turnip-config.
>



-- 
Kit Randel
Canonical - Ubuntu Engineering - Launchpad & Continuous Integration Team

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