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

[Petr Viktorin]

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?


>> 0187: OK

>> (Speaking of PEP8, if you could remove the baseldap star import from
>> sudorule.py, it would be great...)
>>
>>
> This one did hurt, but the star disappeared.

Thank you, much appreciated.
(Especially the fact that Int is no longer imported from baseldap)

>> General thoughts:
>>
>> Would it be possible to merge schema_compat.uldif and
>> install/updates/10-schema_compat.update into one file? Is the uldif
>> special somehow? I guess this is a question for Rob.
>> It would be nice to add a link to some schema-compat-entry-attribute
>> documentation to these files.
>>
> I added Rob to cc. Rob, can you elaborate on this?


>> 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.

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'}

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

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?


-- 
Petr³