[Freeipa-devel] [PATCHES 187-201] Improvements and coverage for sudorule plugin

[Petr Viktorin]

On 06/25/2014 04:28 PM, Tomas Babej wrote:
>
> On 06/18/2014 09:54 AM, Petr Viktorin wrote:
>> On 06/17/2014 12:25 PM, Tomas Babej wrote:
>>>
>>> On 05/26/2014 06:20 PM, Petr Viktorin wrote:
>>>> On 05/20/2014 06:15 PM, Tomas Babej wrote:
>>>>> Hi,
>>>>>
>>>>> the following set of patches fixes:
>>>>>
>>>>> https://fedorahosted.org/freeipa/ticket/4274
>>>>> https://fedorahosted.org/freeipa/ticket/4263
>>>>> https://fedorahosted.org/freeipa/ticket/4324
>>>>> https://fedorahosted.org/freeipa/ticket/4340
>>>>> https://fedorahosted.org/freeipa/ticket/4341
>>>>>
>>>>> and additional minor issues.
>>>>>
>>>>> The improvemed CI test coverage for the sudorule plugin is added as a
>>>>> bonus.
>>
>> You've dropped most of the long commit messages and ticket URLs. Why?
>
> Sorry about that.. fixed!
>
>>
>>
>>>> 0187: OK
>>>> 0188 - sudorule: Allow using hostmasks for setting allowed hosts
>>
>> If I run sudorule-add-host / sudorule-remove-host with a hostmask, but
>> not host/hostgroup, I get prompted for host and hostgroup. I don't
>> think that's the intended behavior.
>
> This problem is beyond this patchset. Observe that same thing happens
> with ipa group-add-member --external.

I was afraid it wouldn't bee that easy.

> I'm not sure if there's a ticket for this though.

There is now. https://fedorahosted.org/freeipa/ticket/4400

>> 0189: OK
>> 0190: OK
>> 0191: OK
>> 0192: OK
>>
>>>> 0193 sudorule: Make sure all the relevant attributes are checked when
>>>> setting category to ALL
>>
>>>> You're missing the `_` for the hostcategory error message.
>>>> Did you think about using something like _("%s cannot be set to 'all'
>>>> while there are %s")?
>>>>
>>> Fixed. Initially, I changed the message as you suggested, but then I
>>> realized, that this might pose a problem for translations that do not
>>> follow the word order in the sentence as it is defined in English
>>> language.
>>
>> Right, sorry for the incorrect example. You can use named
>> substitutions for that:
>>     _("can't %(action)s while %(state)s") % {'action': 'move',
>> 'state': 'asleep'}

The placeholder syntax is %(xyz)s, not %{xyz}.
(Have you been using format() too much?)

>> One more thing - the function is only called once, could you move it
>> to the for loop?

Well, I meant put the function's body in the for loop, getting rid of 
the function. But that's just a nitpick.

>> 0194: OK
>> 0195: OK
>> 0196-0198: OK
>> 0199-0201: OK
>>
>> 0225:
>> Looks good. Could you also document the arguments & return value in
>> *_external_post_callback docstrings?
>>
>>
>
> I did. The updated patchset attached.

Thanks!

-- 
Petr³