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

[Petr Viktorin]

On 06/25/2014 06:23 PM, Tomas Babej wrote:
>
> On 06/25/2014 05:46 PM, Petr Viktorin wrote:
>> 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?)
> That's some detective insight! Thanks for catching that.
>
>
>>
>>>> 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.
>>
> No, you're right, the function's redundant after all the refactoring
> that happened. Fixed.
>
>>>> 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!
>>
>

ACK, pushed to master: af4518b72882f88a01de0e5c23d423898ba894b4

-- 
Petr³