launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> === modified file 'Makefile'
> --- Makefile	2015-01-19 08:58:30 +0000
> +++ Makefile	2015-02-25 07:29:02 +0000
> @@ -10,6 +10,8 @@
>  BUILDOUT_CFG := buildout.cfg
>  BUILDOUT = PYTHONPATH= $(BUILDOUT_BIN) -qc $(BUILDOUT_CFG)
>  
> +APP_PATH := ./turnip
> +
>  
>  default: check
>  
> @@ -54,6 +56,10 @@
>  	touch --no-create $@
>  
>  
> +bin/api:

This seems like an odd name for a target that starts a service, as opposed to a target that generates a file.  Maybe something like "run-api"?

> +	PYTHONPATH=$(APP_PATH) pserve api.ini --reload
> +
> +
>  bin/twistd: $(BUILDOUT_BIN) $(BUILDOUT_CFG) setup.py
>  	$(BUILDOUT) install runtime
>  
> @@ -87,7 +93,9 @@
>  
>  clean_all: clean_buildout clean_eggs
>  
> +lint:
> +	@flake8 turnip
>  
>  .PHONY: \
>      build build-update-paths check clean clean_all clean_buildout \
> -    clean_eggs default dist
> +    clean_eggs default dist lint
> 
> === added file 'api.ini'
> --- api.ini	1970-01-01 00:00:00 +0000
> +++ api.ini	2015-02-25 07:29:02 +0000
> @@ -0,0 +1,45 @@
> +[app:main]
> +use = egg:turnip
> +
> +pyramid.reload_templates = true
> +pyramid.debug_authorization = false
> +pyramid.debug_notfound = false
> +pyramid.debug_routematch = false
> +pyramid.debug_templates = true
> +pyramid.default_locale_name = en
> +
> +[server:main]
> +use = egg:waitress#main
> +host = 0.0.0.0
> +port = 19417
> +
> +# Begin logging configuration
> +
> +[loggers]
> +keys = root, api
> +
> +[handlers]
> +keys = console
> +
> +[formatters]
> +keys = generic
> +
> +[logger_root]
> +level = INFO
> +handlers = console
> +
> +[logger_api]
> +level = DEBUG
> +handlers =
> +qualname = api
> +
> +[handler_console]
> +class = StreamHandler
> +args = (sys.stderr,)
> +level = NOTSET
> +formatter = generic
> +
> +[formatter_generic]
> +format = %(asctime)s %(levelname)-5.5s [%(name)s][%(threadName)s] %(message)s
> +
> +# End logging configuration
> 
> === removed file 'apiserver.tac'
> --- apiserver.tac	2015-02-18 22:53:47 +0000
> +++ apiserver.tac	1970-01-01 00:00:00 +0000
> @@ -1,28 +0,0 @@
> -# You can run this .tac file directly with:
> -#    twistd -ny apiserver.tac
> -from __future__ import (
> -    absolute_import,
> -    print_function,
> -    unicode_literals,
> -    )
> -
> -from twisted.application import (
> -    service,
> -    internet,
> -    )
> -from twisted.web import server
> -
> -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)
> 
> === modified file 'requirements.txt'
> --- requirements.txt	2015-02-18 22:53:47 +0000
> +++ requirements.txt	2015-02-25 07:29:02 +0000
> @@ -1,11 +1,28 @@
>  argparse==1.2.1
> +cornice==0.17
>  envdir==0.7
> +flake8==2.3.0
>  lazr.sshserver==0.1.1
> +mccabe==0.3
> +pastedeploy==1.5.2
> +pep8==1.6.1
>  pyasn1==0.1.7
>  pycrypto==2.6.1
> -PyYAML==3.11
> -Twisted==14.0.2
> +pyflakes==0.8.1
> +pyramid==1.5.2
> +python-mimeparse==0.1.4
> +pyyaml==3.11
> +repoze.lru==0.6
> +setuptools==0.6c11
> +simplejson==3.6.5
> +translationstring==1.3
> +twisted==13.0.0
> +venusian==1.0
> +waitress==0.8.9
> +webob==1.4
>  wsgiref==0.1.2
>  zope.component==4.2.1
> +zope.deprecation==4.1.2
>  zope.event==4.0.3
>  zope.interface==4.1.2
> +zope.testrunner==4.0.3
> 
> === modified file 'setup.py'
> --- setup.py	2015-02-17 21:32:29 +0000
> +++ setup.py	2015-02-25 07:29:02 +0000
> @@ -4,18 +4,25 @@
>  
>  import os
>  
> -from setuptools import setup
> +from setuptools import (
> +    find_packages,
> +    setup,
> +)
>  
>  here = os.path.abspath(os.path.dirname(__file__))
>  
>  with open(os.path.join(here, 'README')) as f:
>      README = f.read()
>  
> +requires = ['cornice', 'lazr.sshserver', 'pygit2', 'PyYAML', 'Twisted',
> +            'waitress', 'zope.interface']
> +test_requires = ['fixtures', 'testtools']
>  
>  setup(
>      name='turnip',
>      version='0.1',
> -    packages=['turnip'],
> +    packages = ['turnip.%s' % package for package in find_packages(
> +        'turnip', exclude=['*.tests', 'tests'])],
>      include_package_data=True,
>      zip_safe=False,
>      maintainer='LAZR Developers',
> @@ -24,16 +31,13 @@
>      long_description=README,
>      url='https://launchpad.net/turnip',
>      download_url='https://launchpad.net/turnip/+download',
> -    install_requires=[
> -        'lazr.sshserver',
> -        'PyYAML',
> -        'Twisted',
> -        'zope.interface',
> -        ],
> +    install_requires=requires,
>      extras_require=dict(
> -        test=[
> -            'fixtures',
> -            'testtools',
> -            ]),
> +        test=test_requires),
>      test_suite='turnip.tests',
> +    entry_points = """\
> +    [paste.app_factory]
> +    main = turnip.api:main
> +    """,
> +    paster_plugins=['pyramid'],
>      )
> 
> === added directory 'turnip/api'
> === removed file 'turnip/api.py'
> --- turnip/api.py	2015-02-03 16:59:42 +0000
> +++ turnip/api.py	1970-01-01 00:00:00 +0000
> @@ -1,59 +0,0 @@
> -from __future__ import (
> -    absolute_import,
> -    print_function,
> -    unicode_literals,
> -    )
> -
> -import os
> -
> -from twisted.internet import defer
> -from twisted.internet.utils import getProcessValue
> -from twisted.web import (
> -    resource,
> -    server,
> -    static,
> -    )
> -
> -from turnip.helpers import compose_path
> -
> -
> -class TurnipAPIResource(resource.Resource):
> -
> -    def __init__(self, root):
> -        resource.Resource.__init__(self)
> -        self.root = root
> -
> -    def getChild(self, name, request):
> -        if name == b'':
> -            return static.Data(b'Turnip API endpoint', type=b'text/plain')
> -        if name == b'create':
> -            return CreateResource(self.root)
> -        else:
> -            return resource.NoResource(b'No such resource')
> -
> -    def render_GET(self, request):
> -        return b'Turnip API service endpoint'
> -
> -
> -class CreateResource(resource.Resource):
> -
> -    def __init__(self, root):
> -        resource.Resource.__init__(self)
> -        self.root = root
> -
> -    @defer.inlineCallbacks
> -    def createRepo(self, request, raw_path):
> -        repo_path = compose_path(self.root, raw_path)
> -        if os.path.exists(repo_path):
> -            raise Exception("Repository '%s' already exists" % repo_path)
> -        ret = yield getProcessValue('git', ('init', '--bare', repo_path))
> -        if ret != 0:
> -            raise Exception("'git init' failed")
> -        request.write(b'OK')
> -        request.finish()
> -
> -    def render_POST(self, request):
> -        path = request.args['path'][0]
> -        d = self.createRepo(request, path)
> -        d.addErrback(request.processingFailed)
> -        return server.NOT_DONE_YET
> 
> === added file 'turnip/api/__init__.py'
> --- turnip/api/__init__.py	1970-01-01 00:00:00 +0000
> +++ turnip/api/__init__.py	2015-02-25 07:29:02 +0000
> @@ -0,0 +1,12 @@
> +# Copyright 2015 Canonical Ltd.  All rights reserved.
> +
> +"""Main entry point
> +"""
> +from pyramid.config import Configurator
> +
> +
> +def main(global_config, **settings):
> +    config = Configurator(settings=settings)
> +    config.include("cornice")
> +    config.scan("turnip.api.views")
> +    return config.make_wsgi_app()
> 
> === added file 'turnip/api/store.py'
> --- turnip/api/store.py	1970-01-01 00:00:00 +0000
> +++ turnip/api/store.py	2015-02-25 07:29:02 +0000
> @@ -0,0 +1,31 @@
> +# Copyright 2015 Canonical Ltd.  All rights reserved.
> +
> +import os
> +import shutil
> +
> +import pygit2
> +
> +
> +class Store(object):
> +    """Provides methods for manipulating repos on disk with pygit2."""
> +
> +    @staticmethod
> +    def init(repo, isBare=True):

The LP style guide would spell this is_bare.  Likewise in callers.

> +        """Initialise a git repo with pygit2."""
> +        if os.path.exists(repo):
> +            raise Exception("Repository '%s' already exists" % repo)
> +        try:
> +            repo_path = pygit2.init_repository(repo, isBare)
> +        except pygit2.GitError:
> +            print('Unable to create repository.')

If the purpose of this (and likewise in delete) is to end up in a log, then it should print the arguments too.

> +            raise
> +        return repo_path
> +
> +    @staticmethod
> +    def delete(repo):
> +        """Permanently delete a git repository from repo store."""
> +        try:
> +            shutil.rmtree(repo)
> +        except (IOError, OSError) as e:
> +            print('Unable to delete repository: %s' % e)
> +            raise
> 
> === added directory 'turnip/api/tests'
> === added file 'turnip/api/tests/__init__.py'
> === added file 'turnip/api/tests/test_api.py'
> --- turnip/api/tests/test_api.py	1970-01-01 00:00:00 +0000
> +++ turnip/api/tests/test_api.py	2015-02-25 07:29:02 +0000
> @@ -0,0 +1,42 @@
> +# Copyright 2015 Canonical Ltd.  All rights reserved.
> +
> +from __future__ import print_function
> +
> +import json
> +import os
> +import unittest
> +import uuid
> +
> +from fixtures import (
> +      EnvironmentVariable,
> +      TempDir,
> +      )
> +from testtools import TestCase
> +from webtest import TestApp
> +
> +from turnip import api
> +
> +
> +class ApiTestCase(TestCase):
> +
> +    def setUp(self):
> +        super(ApiTestCase, self).setUp()
> +        repo_store = self.useFixture(TempDir()).path
> +        self.useFixture(EnvironmentVariable("REPO_STORE", repo_store))
> +        self.app = TestApp(api.main({}))
> +        self.repo_path = str(uuid.uuid1())
> +        self.repo_store = os.path.join(repo_store, self.repo_path)
> +
> +    def test_repo_post(self):
> +        resp = self.app.post('/repo', json.dumps({'repo_path': self.repo_path}))
> +        self.assertEqual(resp.status_code, 200)
> +
> +    def test_repo_delete(self):
> +        self.app.post('/repo', json.dumps({'repo_path': self.repo_path}))
> +        resp = self.app.delete('/repo/{}'.format(self.repo_path))
> +        self.assertEqual(resp.status_code, 200)
> +        self.assertFalse(os.path.exists(self.repo_store))
> +
> +
> +if __name__ == '__main__':
> +    unittest.main()
> 
> === added file 'turnip/api/views.py'
> --- turnip/api/views.py	1970-01-01 00:00:00 +0000
> +++ turnip/api/views.py	2015-02-25 07:29:02 +0000
> @@ -0,0 +1,46 @@
> +# Copyright 2015 Canonical Ltd.  All rights reserved.
> +
> +import os
> +
> +from cornice.resource import resource
> +from cornice.util import extract_json_data
> +import pyramid.httpexceptions as exc
> +
> +from turnip.config import TurnipConfig
> +from turnip.api.store import Store
> +
> +
> +@resource(collection_path='repo', path='/repo/{name}')
> +class RepoAPI(object):
> +    """Provides HTTP API for repository actions."""
> +
> +    def __init__(self, request):
> +        config = TurnipConfig()
> +        self.request = request
> +        self.repo_store = config.get('repo_store')
> +
> +    def collection_post(self):
> +        """Initialise a new git repository."""
> +        repo_path = extract_json_data(self.request).get('repo_path')
> +        if not repo_path:
> +            self.request.errors.add('body', 'repo_path',
> +                                    'repo_path is missing')
> +            return
> +        repo = os.path.join(self.repo_store, repo_path)
> +        isBare = extract_json_data(self.request).get('bare_repo')
> +        try:
> +            Store.init(repo, isBare)
> +        except Exception:
> +            return exc.HTTPConflict()  # 409
> +
> +    def delete(self):
> +        """Delete an existing git repository."""
> +        name = self.request.matchdict['name']
> +        if not name:
> +            self.request.errors.add('body', 'name', 'repo name is missing')
> +            return
> +        repo = os.path.join(self.repo_store, name)
> +        try:
> +            Store.delete(repo)
> +        except Exception:
> +            return exc.HTTPNotFound()  # 404
> 
> === modified file 'turnip/config.py'
> --- turnip/config.py	2015-02-18 22:53:47 +0000
> +++ turnip/config.py	2015-02-25 07:29:02 +0000
> @@ -1,3 +1,5 @@
> +# Copyright 2015 Canonical Ltd.  All rights reserved.
> +
>  from __future__ import unicode_literals
>  
>  import os
> 
> === modified file 'turnipserver.py'
> --- turnipserver.py	2015-02-18 22:53:47 +0000
> +++ turnipserver.py	2015-02-25 07:29:02 +0000
> @@ -9,7 +9,6 @@
>  from twisted.internet import reactor
>  from twisted.web import server
>  
> -from turnip.api import TurnipAPIResource
>  from turnip.config import TurnipConfig
>  from turnip.pack.git import (
>      PackBackendFactory,
> @@ -34,7 +33,7 @@
>  #
>  # 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.
> +# a smart SSH frontend on 9422.
>  reactor.listenTCP(PACK_BACKEND_PORT,
>                    PackBackendFactory(REPO_STORE))
>  reactor.listenTCP(PACK_VIRT_PORT,
> @@ -56,7 +55,4 @@
>      strport=b'tcp:{}'.format(config.get('smart_ssh_port')))
>  smartssh_service.startService()
>  
> -api_site = server.Site(TurnipAPIResource(REPO_STORE))
> -reactor.listenTCP(config.get('repo_api_port'), api_site)
> -
>  reactor.run()
> 
> === modified file 'versions.cfg'
> --- versions.cfg	2015-02-17 21:32:29 +0000
> +++ versions.cfg	2015-02-25 07:29:02 +0000
> @@ -6,11 +6,14 @@
>  [versions]
>  # Alphabetical, case-SENSITIVE, blank line after this comment
>  
> +cornice = 0.17
> +envdir = 0.7
>  extras = 0.0.3
>  fixtures = 0.3.12
>  lazr.sshserver = 0.1.1
>  pyasn1 = 0.1.6
>  pycrypto = 2.6
> +pygit2 = 0.21.4
>  python-mimeparse = 0.1.4
>  PyYAML = 3.11
>  setuptools = 0.6c11
> 


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