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

Wed Jun 25 14:28:01 UTC 2014

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

This problem is beyond this patchset. Observe that same thing happens
with ipa group-add-member --external. I'm not sure if there's a ticket
for this though.

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

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.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/d825fdca/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0225-2-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/d825fdca/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0187-4-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/d825fdca/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0188-4-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/d825fdca/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0189-4-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/d825fdca/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0190-4-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/d825fdca/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0191-4-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/d825fdca/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0192-4-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/d825fdca/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0193-4-sudorule-Make-sure-all-the-relevant-attributes-are-c.patch
Type: text/x-patch
Size: 4301 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/d825fdca/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0194-4-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/d825fdca/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0195-4-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/d825fdca/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0196-4-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/d825fdca/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0197-4-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/d825fdca/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0201-4-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/d825fdca/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0198-4-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/d825fdca/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0199-4-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/d825fdca/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0200-4-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/d825fdca/attachment-0015.bin>