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

Wed Jun 25 16:23:36 UTC 2014

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

-- 
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0225-3-sudorule-Refactor-add-and-remove-external_post_callb.patch
Type: text/x-patch
Size: 22803 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0187-5-sudorule-PEP8-fixes-in-sudorule.py.patch
Type: text/x-patch
Size: 18390 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0188-5-sudorule-Allow-using-hostmasks-for-setting-allowed-h.patch
Type: text/x-patch
Size: 11271 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0189-5-sudorule-Allow-using-external-groups-as-groups-of-ru.patch
Type: text/x-patch
Size: 14011 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0190-5-sudorule-Make-sure-sudoRunAsGroup-is-dereferencing-t.patch
Type: text/x-patch
Size: 2864 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0191-5-sudorule-Include-externalhost-and-ipasudorunasextgro.patch
Type: text/x-patch
Size: 1215 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0192-5-sudorule-Allow-adding-deny-commands-when-command-cat.patch
Type: text/x-patch
Size: 1137 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0193-5-sudorule-Make-sure-all-the-relevant-attributes-are-c.patch
Type: text/x-patch
Size: 4037 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0194-5-sudorule-Fix-the-order-of-the-parameters-to-have-les.patch
Type: text/x-patch
Size: 2984 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0195-5-sudorule-Enforce-category-ALL-checks-on-dirsrv-level.patch
Type: text/x-patch
Size: 4943 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0196-5-ipatests-test_sudo-Add-tests-for-allowing-hosts-via-.patch
Type: text/x-patch
Size: 2625 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0197-5-ipatests-test_sudo-Add-coverage-for-external-entries.patch
Type: text/x-patch
Size: 6890 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0198-5-ipatests-test_sudo-Add-coverage-for-category-ALL-val.patch
Type: text/x-patch
Size: 15765 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0199-5-ipatests-test_sudo-Fix-assertions-not-assuming-runas.patch
Type: text/x-patch
Size: 4708 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0200-5-ipatests-test_sudo-Do-not-expect-enumeration-of-runa.patch
Type: text/x-patch
Size: 1089 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0201-5-ipatests-test_sudo-Expect-root-listed-out-if-no-RunA.patch
Type: text/x-patch
Size: 1544 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/f53c7f92/attachment-0015.bin>