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

Revision history for this message
Brian Lamar (blamar) wrote :

Thanks for the input Rick.

Unfortunately you're right in that this is going to conflict very heavily with the work you have been doing. I'd love to discuss how this can be avoided in the future. I try to keep up on Nova blueprints and the associated branches when they sound API specific but I think that you were potentially not working off a blueprint?

> I'm seeing quite a bit of dependency-injection being used.

Wow, I've never been told that DI is bad, can you potentially give me more details?

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

Updated, good catch.

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

I don't agree in the general case, but it's the second time someone has mentioned this specific case to me. This has been fixed.

> It's usually not a good idea to use now-now in tests.

I'm not quite sure I follow the logic here as none of the tests do time-based comparison (which I agree is a no-no). Can you elaborate?

Thanks!

« Back to merge proposal