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

Revision history for this message
James Tunnicliffe (dooferlad) wrote :

On 4 February 2013 16:31, Milo Casagrande <email address hidden> 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).

It is only the testing that requires Django 1.4, not running the API
library. Using LiveServerTestCase instead of TestCase starts up a
server per-test that the test can interact with. This was introducted
with 1.4. It shouldn't change the functionality of the API at all. Of
course, there is a risk that a bug will be present running under 1.3
that we don't see with 1.4, but so far I haven't encountered any.

> 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...

I haven't tried it. I am sure we can find something that works and we
all like. For now I will change the api_name to "dev" and when we
release we can create a duplicate interface under 1.0, v1.0 or
whatever works.

--
James Tunnicliffe

« Back to merge proposal