[Freeipa-devel] Re: [PATCHES] Make plugins use baseldap classes.

[Rob Crittenden]

Pavel Zuna wrote:
> Rob Crittenden wrote:
>> Some are good to go, others need some more work:
>>
>> 0001-config: ack
>>
>> 0002-group: ack
>>
>> 0003-hostgroups: nack, hostgroups-show doesn't show members by default.
> Fixed in attached patch.
> 
>> 0004-netgroup: nack, fails a ton of unit tests
> Fixed in patch I sent previously today.
> 
>> 0005-rolgroup: nack, rolegroup-find doesn't work (try serviceadmin). 
>> rolegroup-show doesn't include members by default.
>>
>> 0006-taskgroup: nack, taskgroup-find doesn't work (try ipa addhosts) 
>> taskgroup-show doesn't include members by default.
> Both "member thing" fixed, but *-find seems to work for me. Can you post 
> the exact commands that fail?

% ipa rolegroup-find serviceadmin

Looks like it is failing because nestedgroup isn't an objectclass in 
rolegroups.

[02/Oct/2009:13:22:05 -0400] conn=1271 op=3 SRCH 
base="cn=rolegroups,cn=accounts,dc=example,dc=com" scope=1 
filter="(&(|(memberOf=*serviceadmin*)(cn=*serviceadmin*)(description=*serviceadmin*))(&(objectClass=ipaobject)(objectClass=groupofnames)(objectClass=nestedgroup)))" 
attrs="cn description memberOf"
[02/Oct/2009:13:22:05 -0400] conn=1271 op=0 RESULT err=14 tag=97 
nentries=0 etime=0, SASL bind in progress
[02/Oct/2009:13:22:07 -0400] conn=1271 op=3 RESULT err=3 tag=101 
nentries=0 etime=2 notes=U

> 
>> 0007-tests: ack though we should do some non-raw tests as well IMHO. 
>> In test-hostgroup looks like there is some leftover debug output that 
>> should be removed before committing.
> Removed the leftover debug output in attached patch.
> 
>> 0008-pwpolicy: nack, have another patch floating around that covers 
>> most of this. I'll try to incorporate some of your ideas into my patch 
>> once committed.
> Ok.
> 
>> 0009 missing?
> Not missing, just wrong numbering. There was an unrelated patch that got 
> in between the plugin patches.
> 
>> 0010-service: ack, but we should try to avoid shadowing built-in names 
>> (filter)
> Sometimes it might be justified, but in this case we really should avoid 
> it, because it's actually in the base class and what if someone wants to 
> use python's filter function inside the callback? (They would have to 
> rename the param or do some hacking around.) It doesn't hurt anything 
> for now, but this will have to be patched at several places - I'll post 
> a patch on Monday.
> 
>> 0011-user: ack but changes like this seem to make it less readable:
>>
>> -        textui.print_dashed('Unlocked user "%s".' % uid)
>> +        textui.print_dashed('Unlocked user "%s".' % keys[-1])
>>
>> I realize that *keys is more generic than specifying one argument but 
>> there is no documentation saying was keys means.
> I'm really slow at documenting things, but it's in the works.
> 
>> Some general observations that don't necessarily need to be addressed 
>> here but do need to be done:
>>
>> - adding/removing member failures should return non-zero return code 
>> (so you can test $? on cmd-line)
>> - add/remove-member should probably prompt for things if not provided 
>> on command-line (and allow empty values). Right now if you do an 
>> add-member it prompts for group to modify and then submits an empty 
>> request
> Ok, I'll try to address these issues as soon as possible.
> 
>> Let me know if it is safe to push these patches individually.
> The user and *group plugins should be pushed at once. Everything else 
> should be safe individually.

Ok, I'll review the new patches and get the acked ones pushed out.

rob

> 
>> rob
>>
> 
> Pavel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3245 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20091002/5f94f1c2/attachment.bin>