Code review comment for lp:~cbehrens/nova/servers-search

Revision history for this message
Brian Waldon (bcwaldon) wrote :

A branch recently went in that replaced all returns of fault-wrapped webob exceptions with raising of non-wrapped webob exceptions. Can you convert any of the "return faults.Fault(...)" to "raise ..."?

I think we may want to take the approach of ignoring unsupported query params rather than raising an exception (that's how Glance does it). We should be liberal in what we accept. A lot of the _get_items / _items code can get cleaned back up if we don't need to explicitly check filter params. Same goes for check_option_permissions.

Power states belong to the compute layer, and server statuses belong to the OSAPI. I think we should remove any OSAPI status-related code from the power_states module and place it in either api.openstack.common or api.openstack.servers. What do you think?

What is the 'fresh' query filter? I just started seeing that in novaclient requests.

Are there future plans to implement changes-since? I can see it is an accepted filter, just not respected.

In the DB API, why is there a _get_all_by_column_regexp and a _get_all_by_instance_name? Can't we use get_all_by_column_regexp to accomplish the name filter?

Can you change _get_all_by_column to _get_all_by_column_value? The former doesn't clearly say you are checking the value in a specific column when there is another function with column_regexp.

I don't see it necessary to list OSAPI-specific filters in the compute api. Can we fix that known_params?

I really want to be able to sort by multiple parameters, and I think most people using the API would expect to be able to do that. How hard would it be to support this?

Don't get me wrong here, I'm really excited to get this feature in. Having done this in Glance already, I know this isn't a small undertaking :)

review: Needs Fixing

« Back to merge proposal