launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

As well as the inline comments, you should probably remove apiserver.tac and do something with turnipserver.py, since neither will work as-is now.

Diff comments:

> === modified file 'Makefile'
> --- Makefile	2015-01-19 08:58:30 +0000
> +++ Makefile	2015-02-24 00:10:37 +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:
> +	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-24 00:10:37 +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
> 
> === modified file 'requirements.txt'
> --- requirements.txt	2015-02-18 22:53:47 +0000
> +++ requirements.txt	2015-02-24 00:10:37 +0000
> @@ -1,11 +1,26 @@
> +PasteDeploy==1.5.2
> +Twisted==15.0.0

You also have Twisted==14.0.2 below; both can't be right.  We should probably continue to maintain this sorted case-insensitively, as was previously the case.

> +WebOb==1.4
>  argparse==1.2.1
> +cornice==0.17
>  envdir==0.7
> +flake8==2.3.0
>  lazr.sshserver==0.1.1
> +mccabe==0.3
> +pep8==1.6.1
>  pyasn1==0.1.7
>  pycrypto==2.6.1
> +pyflakes==0.8.1
> +pyramid==1.5.2
>  PyYAML==3.11
> +repoze.lru==0.6
> +simplejson==3.6.5
> +translationstring==1.3
>  Twisted==14.0.2
> +venusian==1.0
> +waitress==0.8.9
>  wsgiref==0.1.2
>  zope.component==4.2.1
> +zope.deprecation==4.1.2
>  zope.event==4.0.3
>  zope.interface==4.1.2
> 
> === modified file 'setup.py'
> --- setup.py	2015-02-17 21:32:29 +0000
> +++ setup.py	2015-02-24 00:10:37 +0000
> @@ -4,18 +4,23 @@
>  
>  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', 'Twisted', 'waitress',
> +            'zope.interface']

You don't seem to use this anywhere.

>  
>  setup(
>      name='turnip',
>      version='0.1',
> -    packages=['turnip'],
> +    packages=find_packages(),

I seem to remember finding that this was very slow in combination with a virtualenv/buildout in the current directory.  How about find_packages('turnip') to limit the search?

>      include_package_data=True,
>      zip_safe=False,
>      maintainer='LAZR Developers',
> @@ -36,4 +41,9 @@
>              'testtools',
>              ]),
>      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-24 00:10:37 +0000
> @@ -0,0 +1,10 @@
> +"""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-24 00:10:37 +0000
> @@ -0,0 +1,24 @@
> +import shutil
> +
> +import pygit2
> +
> +
> +class Store(object):
> +    """Provides methods for manipulating repos on disk with pygit2."""
> +    @classmethod
> +    def init(self, repo, isBare=True):

classmethods would conventionally take cls as their first argument, not self.  In either case, though, you don't actually use the class context so it doesn't need to be bound and could use @staticmethod instead (with no first argument), or even just be a function.

> +        """Initialise a git repo with pygit2."""
> +        try:
> +            repo_path = pygit2.init_repository(repo, isBare)

Does this ensure that the directory tree leading up to the new repository exists?  turnip shouldn't apply a particular policy to repository paths (other than perhaps forbidding a leading ../), and we might for example want to namespace them in subdirectories.

> +        except pygit2.GitError as e:
> +            print('Unable to create repository: %s' % e)
> +            return

The previous code raised an exception in this case.  Would it be possible to do the same here, and map that back to some suitable HTTP error code, perhaps 409 Conflict?

> +        return repo_path
> +
> +    @classmethod
> +    def delete(self, repo):

Same classmethod comment as above.

> +        """Permanently delete a git repository from repo store."""
> +        try:
> +            shutil.rmtree(repo)
> +        except (IOError, OSError) as e:
> +            print('Unable to delete repository: %s' % e)

Same exception comment as above, although the error code would of course need to be different.

> 
> === added file 'turnip/api/views.py'
> --- turnip/api/views.py	1970-01-01 00:00:00 +0000
> +++ turnip/api/views.py	2015-02-24 00:10:37 +0000
> @@ -0,0 +1,37 @@
> +import os
> +
> +from cornice.resource import resource
> +from cornice.util import extract_json_data
> +
> +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')
> +        Store.init(repo, isBare)
> +
> +    def delete(self):
> +
> +        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)
> +        Store.delete(repo)
> 
> === added file 'turnip/tests/test_api.py'
> --- turnip/tests/test_api.py	1970-01-01 00:00:00 +0000
> +++ turnip/tests/test_api.py	2015-02-24 00:10:37 +0000
> @@ -0,0 +1,42 @@
> +from __future__ import print_function
> +
> +import json
> +import os
> +import shutil
> +import unittest
> +import uuid
> +
> +from webtest import TestApp
> +
> +from turnip import api
> +from turnip.config import TurnipConfig
> +
> +
> +class ApiTestCase(unittest.TestCase):
> +
> +    def setUp(self):
> +        self.config = TurnipConfig()
> +        repo_store = self.config.get('repo_store')

Test files should go in a temporary directory rather than (a subdirectory of) the configured repo_store, and then it would make sense to just override repo_store in the environment.  That's pretty low-effort with fixtures and testtools, something like this, also obsoleting remove_store:

  from fixtures import (
      EnvironmentVariable,
      TempDir,
      )
  from testtools import TestCase

  class ApiTestCase(TestCase):

      def setUp(self):
          self.repo_store = self.useFixture(TempDir()).path
          self.useFixture(EnvironmentVariable("REPO_STORE", self.repo_store))

> +        self.repo_path = str(uuid.uuid1())
> +        self.repo_store = os.path.join(repo_store, self.repo_path)
> +
> +    def remove_store(self):
> +        if os.path.exists(self.repo_store):
> +            shutil.rmtree(self.repo_store)
> +
> +    def test_repo_post(self):
> +        self.addCleanup(self.remove_store)
> +        app = TestApp(api.main({}))
> +        resp = app.post('/repo', json.dumps({'repo_path': self.repo_path}))
> +        self.assertEquals(resp.status_code, 200)

That's a deprecated alias for assertEqual.

> +
> +    def test_repo_delete(self):
> +        self.addCleanup(self.remove_store)
> +        app = TestApp(api.main({}))
> +        app.post('/repo', json.dumps({'repo_path': self.repo_path}))
> +        resp = app.delete('/repo/{}'.format(self.repo_path))
> +        self.assertEquals(resp.status_code, 200)
> +        self.assertFalse(os.path.exists(self.repo_store))
> +
> +if __name__ == '__main__':
> +    unittest.main()
> 
> === modified file 'versions.cfg'
> --- versions.cfg	2015-02-17 21:32:29 +0000
> +++ versions.cfg	2015-02-24 00:10:37 +0000
> @@ -6,11 +6,13 @@
>  [versions]
>  # Alphabetical, case-SENSITIVE, blank line after this comment
>  
> +cornice = 0.17
>  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.