Open Stack

On 2016/04/05 22:23, Ihar Hrachyshka wrote:
> Hirofumi Ichihara <ichihara.hirofumi at lab.ntt.co.jp> wrote:
>
>> Hi Ihar,
>>
>> On 2016/04/05 7:57, Ihar Hrachyshka wrote:
>>> Hi all,
>>>
>>> in neutron, we have a bunch of configuration options to control 
>>> advanced filtering features for API, f.e. allow_sorting, 
>>> allow_pagination, allow_bulk, etc. Those options have default False 
>>> values.
>> I saw allow_bulk option is set default True in 
>> https://github.com/openstack/neutron/blob/master/neutron/common/config.py#L66
>> Well, I don't think there's someone sets False to the option.
>
> Yes, indeed only allow_sorting and allow_pagination are disabled by 
> default.
>
>>
>>> In the base API controller class, we have support for both native 
>>> sorting/pagination/bulk operations [implemented by the plugin 
>>> itself], as well as a generic implementation for plugins without 
>>> native support. But if corresponding allow_* options are left with 
>>> their default False values, those advanced search/filtering criteria 
>>> just don’t work, no matter whether the plugin support native 
>>> filters, or not.
>>>
>>> It seems weird to me that our API behaves differently depending on 
>>> configuration options, and that we have those useful features 
>>> disabled by default.
>>>
>>> My immediate interest is to add native support for 
>>> sorting/pagination for QoS service plugin; I have a patch for that, 
>>> and I planned to add some API tests to validate that the features 
>>> work, but I hit failures because those features are not enabled for 
>>> the -api job.
>>>
>>> Some questions:
>>> - can we enable those features in -api job?
>>> - is there any reason to keep default values for allow_* as False, 
>>> and if not, can we switch to True?
>>> - why do we even need to control those features with configuration 
>>> options? can we deprecate and remove them?
>> I agree we will deprecate and remove the option but I think that we 
>> need more tests if we support it as default.
>> It looks like there are very few tests(UT only).
>
> That’s a good suggestion. I started a patch to enable those two 
> options, plus add first tests for the feature:
>
> https://review.openstack.org/#/c/301634/
>
> For now it covers only for networks. I wonder how we envision the 
> coverage. Do we want to have test cases per resource? Any ideas on how 
> to make the code more generic to avoid code duplication? For example, 
> I could move those test cases into a base class that would require 
> some specialization for each resource that we want to cover 
> (get/create methods, primary key, …).
The patch is reasonable for me as first step. Second, I agree to make it 
more generic. I think that we should have test per resource but we will 
do in future work.

>
> Also, do we maybe want to split the patch into two pieces:
> - first one adding tests [plus enabling those features for API job];
> - second one changing the default value for the options.
+1

Thanks,
Hirofumi

>
> Ihar
>
> __________________________________________________________________________ 
>
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: 
> OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev