On 2016/04/05 22:23, Ihar Hrachyshka wrote:
Hirofumi Ichihara <ichihara.hirof...@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-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
