Code review comment for lp:~dooferlad/linaro-ci-dashboard/add_api_tastypie

Revision history for this message
Milo Casagrande (milo) wrote :

Hello James!

Nice work here! :-)

On Fri, Feb 1, 2013 at 12:57 PM, James Tunnicliffe
<email address hidden> wrote:
>
> The current API is very limited. The intent of this MP is to get some feedback on if this is a sensible way to implement the API. Future merges will fill it out.

So, some feedback.
Keep in mind that I never created RESTful API with Python nor Django
(only worked with Java on this front).

Overall, it looks a good Django-nic way of creating RESTful API
without decorators. I hove no idea of the other frameworks around, but
docs for this are well written and seems also up-to-date (a big plus I
would say).

> If you are running Ubuntu 12.04 you can run the application, just not unit test the API because it relies on LiveServerTestCase, which was introduced with Django 1.4. To work around this, instructions are provided for setting up an isolated Python environment using virtualenvwrapper are included in the README.

Regarding this, since we will deploy or install this on a 12.04
server, how are we going to handle/maintain it?
Using virtualenv? Backport? Default backport channel for 12.04 does
not have any newest version of Django available... That is something
we need to keep in mind, because I think we will be using LTS versions
(and next one will be out in 1 year time).

Anyway, using virtualenvwrapper everything worked perfectly, and tests run fine.

> === added file 'dashboard/frontend/api/urls.py'
> --- dashboard/frontend/api/urls.py 1970-01-01 00:00:00 +0000
> +++ dashboard/frontend/api/urls.py 2013-02-01 11:56:24 +0000
> @@ -0,0 +1,13 @@
> +from django.conf.urls.defaults import patterns, include
> +from dashboard.frontend.api.api import *
> +from tastypie.api import Api
> +
> +v1_api = Api(api_name='v1')
> +
> +# Register new resources here (probably defined in api.py)
> +v1_api.register(LoginTestResource())
> +v1_api.register(LoopsResource())
> +
> +urlpatterns = patterns('',
> + (r'^api/', include(v1_api.urls)),
> +)

I have to say I'm more in favor of using a normal numbering scheme
(like "1.0") for API versions (at least for stable ones, maybe a "dev"
one for bleeding-edge stuff).
I guess that if we need to change API version and bump the number, it
would be as easy as this piece of code, as long as Djano does not
complain for the double entry with the same search-pattern (r'^api/').
Did you try if it works? Can't remember out of my mind now...

Ciao!

--
Milo Casagrande | Infrastructure Team
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

« Back to merge proposal