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

Tue Apr 9 15:06:38 UTC 2013

On 04/09/2013 04:38 PM, Petr Vobornik wrote:
> On 04/04/2013 12:02 PM, Martin Kosek wrote:
>> On 04/04/2013 11:48 AM, Tomas Babej wrote:
>>> On 03/22/2013 03:03 PM, Martin Kosek wrote:
>>>> On 03/21/2013 06:10 PM, Petr Vobornik wrote:
>>>>> On 03/21/2013 05:10 PM, Martin Kosek wrote:
>>>>>> On 03/16/2013 03:32 AM, Endi Sukma Dewata wrote:
>>>>>>> On 3/12/2013 11:28 AM, Petr Vobornik wrote:
>>>>>>>> Here's a patch for filtering groups by type.
>>>>>>>> Design page: http://www.freeipa.org/page/V3/Filtering_groups_by_type
>>>>>>>>
>>>>>>>> The interface is:
>>>>>>>>> StrEnum('type?',
>>>>>>>>>        cli_name='type',
>>>>>>>>>        label=_('Type'),
>>>>>>>>>        doc=_('Group type'),
>>>>>>>>>        values=(u'posix', u'normal', u'external'),
>>>>>>>>> ),
>>>>>>>> I have two design questions.
>>>>>>>> 1. Is --type the right option name?
>>>>>>> Fine by me, it matches the label and description.
>>>>>>>
>>>>>>>> 2. Is `normal` the right name for non-posix, non-external group? The
>>>>>>>> default group type (when adding group) is posix. Should the name be
>>>>>>>> something else: `simple`, `plain`, `ordinary`?
>>>>>>> We also use 'normal' in the group adder dialog, so it's consistent. Other
>>>>>>> options are 'basic', 'standard', 'regular'.
>>>>>>>
>>>>>>>> I didn't want to create an option for each type. IMO it brings more
>>>>>>>> complexity.
>>>>>>> Maybe the group-add/mod command should use the same --type option?
>>>>>>>
>>>>>>>> https://fedorahosted.org/freeipa/ticket/3483
>>>>>>> ACK from me, but maybe others might have some comments.
>>>>>>>
>>>>>> I am just thinking about if the new API is right. For example, when we
>>>>>> add an
>>>>>> external group, we use ipa group-add --external. But when we search for
>>>>>> external groups, we suddenly use
>>>>>> # ipa group-find --type=external
>>>>>> and not
>>>>>> # ipa group-find --external
>>>>>> or
>>>>>> # ipa group-find --nonposix
>>>>>>
>>>>>> Wouldn't that cause confusion? I am looking for same second opinion on this
>>>>>> one.
>>>>>>
>>>>>> I also did not like "normal" group type very much, maybe we should just
>>>>>> call it
>>>>>> "nonposix"? As that's the option you use when you are creating such group:
>>>>>> # ipa group-add --nonposix foo
>>>>>>
>>>>>> Otherwise, the patch looks good functionally.
>>>>>>
>>>>>> Martin
>>>>>>
>>>>> I have to note that external group is also non-posix. Following command is
>>>>> valid:
>>>>>     # ipa group-add foo --desc=a --external --nonposix
>>>>>
>>>>> By that logic
>>>>>     # ipa group-find --nonposix
>>>>>
>>>>> Would also list external groups.
>>>>>
>>>>> I fine with renaming 'normal' to something better (will also require Web UI
>>>>> change), but it is not 'nonposix'.
>>>> I think this logic is flawed as well. Then you could say that posix group is
>>>> also nonposix, because it contains the same objectclasses as nonpoxis group +
>>>> posixGroup objectclass.
>>>>
>>>> "nonposix" is the term we already use (see --nonposix), not something
>>>> artificial or new, so I would not be afraid of it.
>>>>
>>>> Martin
>>>>
>>>> _______________________________________________
>>>> Freeipa-devel mailing list
>>>> Freeipa-devel at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>> Let us try to move on with this, here are my 2 cents:
>>>
>>> 1.) normal is not a suitable name for non-posix, non-external group. As a user,
>>> I would assume that
>>>    # ipa group-find --type=normal
>>>
>>> would return the groups that I created using simple
>>>    # ipa group-add testgroup
>>>
>>> command. By that logic, any other suggested synonym implying there's nothing
>>> special about this
>>> group is not suitable.
>>>
>>> 2.) If not normal (or any other synonym implying there's nothing special about
>>> this group) then what?
>>> We can either:
>>>    - use exact but complicated --non-posix-non-external
>>>    - use --nonposix and deal with the fact that sets defined by the type are
>>> not
>>> disjunct
>>>    - make up our own new term and define it
>>>
>>> While none of these options are fortunate, let's look for the least resistance:
>>>    - exact, but complicated names are ugly and do not keep interface simple
>>>    - nonposix groups are superset of external groups
>>>    - confuses the user and makes the learning curve steeper
>>>
>>>  From this I would go for option 2, indeed, if you think about --nonposix /
>>> --external as flags, where
>>> the external takes priority before nonposix, this kind of makes sense. If the
>>> user does not think
>>> about the implementation (that every external group is nonposix), he may indeed
>>> find himself in this mindset.
>>>
>>> 3.) I'm fine both with --type=external and --external approaches. The latterr
>>> is more consistent with the way we do things,
>>> *-find commands search mainly on selected subset of attributes, so using the
>>> flag analogy I mentioned an paragraph ago,
>>> you would expect --external to behave as an attribute, especially if group-add
>>> command accepts it in this form.
>>>
>>> Having 3 options instead of one will clutter things a bit more, but if we keep
>>> them in the same place (in the list of options)
>>> it should not cause much confusion, more so if the descriptions would be nearly
>>> the same, one would quickly see that these
>>> belong together.
>>>
>>> Tomas
>>>
>>
>> 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