[Freeipa-devel] [PATCH] 356 Add trusconfig-show and trustconfig-mod commands

[Martin Kosek]

On 02/08/2013 05:30 PM, Alexander Bokovoy wrote:
> On Fri, 01 Feb 2013, Martin Kosek wrote:
>> On 02/01/2013 03:55 PM, Alexander Bokovoy wrote:
>>> On Tue, 29 Jan 2013, Martin Kosek wrote:
>>>> trust_output_params = (
>>>> @@ -482,3 +499,158 @@ api.register(trust_mod)
>>>> api.register(trust_del)
>>>> api.register(trust_find)
>>>> api.register(trust_show)
>>>> +
>>>> +
>>>> +_trust_type_option = (
>>>> +    StrEnum('trust_type',
>>>> +        cli_name='type',
>>>> +        label=_('Trust type (ad for Active Directory, default)'),
>>>> +        values=(u'ad',),
>>>> +        default=u'ad',
>>>> +        autofill=True,
>>>> +    ),
>>>> +)
>>> We already have various trust type definitions in the same file. Maybe
>>> it makes sense to unify those somehow?
>>
>> Right, I unified those 2 separate trust_type option definitions.
>>
>>>
>>>> +    def get_dn(self, *keys, **kwargs):
>>>> +        trust_type = kwargs.get('trust_type')
>>>> +        if trust_type is None:
>>>> +            raise errors.RequirementError(name='trust_type')
>>>> +        if kwargs['trust_type'] == u'ad':
>>> Perhaps better to define constants for the trust type values...
>>
>> I changed it a bit and now it uses a dict instead. I think its now more general
>> and extensible.
>>
>>>
>>>> +        except ValueError:
>>>> +            # The search is performed for groups with "posixgroup"
>>>> objectclass
>>>> +            # and not "ipausergroup" so that it can also match groups like
>>>> +            # "Default SMG Group" which does not have this objectclass.
>>> 'Default SM_B_ Group'
>>
>> Fixed.
>>
>>>
>>> Thanks for the unit tests too!
>>>
>>
>> You are welcome! I also generated API.txt which I forgot to do last time.
>> Updated patch attached.
> ACK for the code but please add more documentation (below).
> 
> Works like sharm. I tried also changing default fallback group to
> some IPA group, then back to Default SMB Group and it worked well. Also
> specifying non-existing group was noted and rejected.
> 
> Please make sure to mention in the design page magic value 'Default SMB
> Group' and also that you can use any group with 'posixgroup'
> objectclass, and that 'Default SMB Group' is not visible through normal
> IPA tools.
> 
> We need to write better documentation (online help) for trustconfig-mod.
> Basically, right now it helps no one to understand what is supposed to
> be done here.
> 
> Once help is added, ACK.

Thanks for the review! RFE updated with information you mentioned.

I also added more info to trust online help (which you verified off-list).

Pushed to master, ipa-3-1.

Martin