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:

the following set of patches fixes:


and additional minor issues.

The improvemed CI test coverage for the sudorule plugin is added as a

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

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

Looks good. Could you also document the arguments & return value in
*_external_post_callback docstrings?

I did. The updated patchset attached.



Freeipa-devel mailing list

Reply via email to