[Freeipa-devel] [PATCH] 160,161 Trust Web UI

[Martin Kosek]

On 06/25/2012 06:18 PM, Alexander Bokovoy wrote:
> On Mon, 25 Jun 2012, Endi Sukma Dewata wrote:
>> On 6/25/2012 10:33 AM, Petr Vobornik wrote:
>>> On 06/25/2012 04:52 PM, Petr Vobornik wrote:
>>>> On 06/25/2012 04:37 PM, Alexander Bokovoy wrote:
>>>>> On Mon, 25 Jun 2012, Alexander Bokovoy wrote:
>>>>>> On Mon, 25 Jun 2012, Simo Sorce wrote:
>>>>>>> On Mon, 2012-06-25 at 12:43 +0200, Petr Vobornik wrote:
>>>>>>>> On 06/23/2012 01:44 AM, Endi Sukma Dewata wrote:
>>>>>>>>> On 6/22/2012 11:48 AM, Alexander Bokovoy wrote:
>>>>>>>>>> 2. First two chunks of
>>>>>>>>>> install/ui/test/data/ipa_init_commands.json
>>>>>>>>>> and
>>>>>>>>>> install/ui/test/data/ipa_init_objects.json changes look
>>>>>>>>>> unrelated to
>>>>>>>>>> this ticket.
>>>>>>>>>
>>>>>>>>> These files are snapshots of metadata used for demo/testing. I
>>>>>>>>> suppose
>>>>>>>>> Petr was updating the entire files which automatically includes
>>>>>>>>> recent
>>>>>>>>> changes to the metadata.
>>>>>>>>>
>>>>>>>>>> ACK
>>>>>>>>>
>>>>>>>>> Ditto. The UI code looks fine so it can be pushed. Btw, nice
>>>>>>>>> use of
>>>>>>>>> layout class.
>>>>>>>>>
>>>>>>>>> Some comments:
>>>>>>>>>
>>>>>>>>> 1. The CLI command to add trust is trust-add-ad. Should the UI
>>>>>>>>> button
>>>>>>>>> also say "Add AD"? If we later support additional trust types
>>>>>>>>> would
>>>>>>>>> that
>>>>>>>>> appear as separate buttons/dialogs or same button/dialog with
>>>>>>>>> maybe
>>>>>>>>> drop-down list to select the type?
>>>>>>>> "Add AD" label seems weird to me. Now we support only one type of
>>>>>>>> trust.
>>>>>>>> We should keep the 'Add'.
>>>>>>>
>>>>>>> I have to say I also find the trust-add-ad command really weird,
>>>>>>> difficult to use and to spell vaocally and to remember.
>>>>>>>
>>>>>>> Alexander can we change it to trust-add --type=ad
>>>>>>> where we can omit --type=ad for now as it is the only one, later
>>>>>>> on we
>>>>>>> can decide what to default to when --type is omitted.
>>>>>> Patch attached (not tested).
>>>>> Attached is tested patch.
>>
>> ACK abbra-53 & abbra-54. One thing though, the error message is not
>> very user friendly. Feel free to fix before push.
>>
>>  % ipa trust-add ad.test --type=asdf
>>  ipa: ERROR: invalid 'type': must be one of (u'ad',)
>>
>> The ValidationError specifies this message 'only "ad" is supported'
>> but it doesn't appear in the error message above.
> The message above comes from StrEnum() validator which is common one for
> all StrEnum()s. I made a ValidationError in execute() method to catch up
> any discrepancies when other types of trust will be added as Python
> doesn't have 'case/switch' so you are left alone with 'if' or list-based
> lambdas which don't look so clear.
> 
> What we probably want to add is more friendly way to display these
> StrEnum values, dropping u'' and simply showing proper unicode as we do
> for help already.
> 
> Endi, could you please file a minor bug for it?
> 

Since this will be filed in a separate bug, I went ahead and pushed all
3 acked patches in this thread to master:
2012-06-13 17:44 Petr Vobornik      o Trust Web UI
2012-06-25 16:41 Alexander Bokovoy  o Rename 'ipa trust-add-ad' to 'ipa
trust-add --type=ad'
2012-06-22 19:33 Alexander Bokovoy  o Use correct SID attribute for
trusted domains

Martin