[Freeipa-devel] [PATCH] 267 Filter groups by type (normal, posix, external)

[Martin Kosek]

On 04/09/2013 06:45 PM, Petr Vobornik wrote:
> On 04/09/2013 05:06 PM, Martin Kosek wrote:
>> On 04/09/2013 04:38 PM, Petr Vobornik wrote:
>>> On 04/04/2013 12:02 PM, Martin Kosek wrote:
>>>> Thanks Tomas for your opinion, I can agree with that. To make it more in an
>>>> actual design, this is API following this discussion that I would propose:
>>>>
>>>> This is API we already have in IPA:
>>>> ipa group-add --external
>>>> ipa group-add --nonposix
>>>> ipa group-find --private
>>>>
>>>> This is API that I would propose to add to be consistent with what we already
>>>> have:
>>>> ipa group-find --nonposix
>>>> ipa group-find --posix
>>>> ipa group-find --external
>>>>
>>>> --nonposix would only match groups added with --nonposix flag in group-add,
>>>> i.e. no --external groups.
>>>>
>>>> As Tomas said, these should also be close together. We can even add a specific
>>>> option group for them, like there are with ipa dnsrecord-add, named for
>>>> example
>>>> "Group Types". We may also raise OptionError when these option are used
>>>> together to make this less confusing - e.g. OptionError("group type options
>>>> (--nonposix, --posix and --external) are mutually exclusive").
>>>>
>>>> Martin
>>>>
>>> New version attached.
>>>
>>
>> 1) default=False parameter for Flag is redundant:
>>
>> +        Flag('external',
>> +             cli_name='external',
>> +             doc=_('search for groups with support of external non-IPA members
>> from trusted domains'),
>> +             default=False,
>> +        ),
>>
>>
>> 2) No need to import StrEnum:
>> +from ipalib import Int, Str, StrEnum
>>
>> 3) This can be simplified:
>> +        if len(filters):
>> TO:
>> +        if filters:
>>
>>
>> Besides these minor issues, that patch works fine and I think we can push a
>> fixed version.
>>
>> Thanks,
>> Martin
>>
>
> Additional self-nack.
>
> 4) original filter is ignored when some of the new options is used. It prevents
> from effective searching and also shows private groups when --posix is used.
>
> All fixed, new unit test added. New version attached.
>
> The fix doesn't touch usage of --private because it's a special case - private
> groups don't have ipausergroup oc and therefore they are incompatible with
> original filter.

ACK, thanks for the fix. Pushed to master.

Please also make sure that you update design page 
(http://www.freeipa.org/page/V3/Filtering_groups_by_type) with respect to the 
API change we have discussed above.

Thanks,
Martin