Code review comment for lp://qastaging/~chemikadze/nova/contrib-extention-networks

Revision history for this message
Matt Dietz (cerberus) wrote :

I'm not really a fan of this being an admin route, exactly. Currently there are no other extensions that use the "admin" route or any standard spec controllers that do. I would actually suggest we simply remove "admin" from the route, because I think it could be confusing to the end user trying to discover functional
ity about the API. Specifically, they might wonder why other "admin" looking actions are routed under existing controllers, and what about "networks" makes it specifically admin related.

7 +# Copyright 2011 OpenStack LLC.
8 +# Copyright 2011 Grid Dynamics
9 +# Copyright 2011 Nikolay Sokolov

If you're adding a new file, you only need whichever copyright is most relevant to the work you're doing. I'm guessing this would primarily be Grid Dynamics in this case?

I'm concerned about the unit tests in this patch, too. While I endorse the idea that we don't use the database for unit tests, I'm afraid that, given your overriding every single database API method, if the networks tables were to change in any way, your tests would continue to pass. What do you think?

review: Needs Fixing

« Back to merge proposal