Code review comment for lp:~blamar/nova/openstack-api-1-1-images

Revision history for this message
Rick Harris (rconradharris) wrote :

Nice work Brian. A few comments:

General
=======

* Not much we can do here, but looks like this is gonna conflict pretty heavily with `related_images`

* I'm seeing quite a bit of dependency-injection being used. This can usually be avoid by stubbing out using Fakes and Mocks. This keeps the implementation clean since it won't have to take `client`, `image_service`, `compute_service` args. This isn't a deal-breaker for this patch (by any means), but wanted to raise it for discussion.

Specifics
=========

> 235 + ex = webob.exc.HTTPNotFound(explanation="Image not found.")

Needs i18n treatment.

> 238 + return dict(image=self.get_builder(req).build(image, True))

Might be a little clearer if a kwarg is used, like:

    return dict(image=self.get_builder(req).build(image, detail=True))

> 464 now = datetime.datetime.utcnow()

It's usually not a good idea to use now-now in tests, since the tests, in a way, change each time. For example, you can see a test pass everyday but fail on Feb. 29th. Rather, you might want to do something like:

    NOW_DATETIME = datetime.datetime(2010, 10, 11, 10, 32, 23)
    NOW_STR = "2010-10-11T10:32:23Z"

review: Needs Fixing

« Back to merge proposal