On 03/07/2013 08:27 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 03/06/2013 09:52 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
[...]
On new installs, the ACI on cn=Posix IDs,cn=Distributed Numeric
Assignment Plugin,cn=plugins,cn=config is added before the entry
itself.
I didn't test everything as I didn't get the access.


It shouldn't make a difference. What isn't working?

I get a CRITICAL message in the log:

add aci:
         (targetattr=dnaNextRange || dnaNextValue ||
dnaMaxValue)(version 3.0;acl "permission:Modify DNA Range";allow (write)
groupdn = "ldap:///cn=Modify DNA
Range,cn=permissions,cn=pbac,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com";)


modifying entry "cn=Posix IDs,cn=Distributed Numeric Assignment
Plugin,cn=plugins,cn=config"

2013-03-07T11:01:48Z DEBUG stderr=ldap_initialize(
ldap://vm-081.idm.lab.eng.brq.redhat.com:389/??base )
ldap_modify: No such object (32)

2013-03-07T11:01:48Z CRITICAL Failed to load replica-acis.ldif: Command
'/usr/bin/ldapmodify -v -f /tmp/tmpT55upM -H
ldap://vm-081.idm.lab.eng.brq.redhat.com:389 -x -D cn=Directory Manager
-y /tmp/tmplFeere' returned non-zero exit status 32


Gotcha. I moved where the replica acis are loaded.

Please attach the patch :)

[...]
+        failed = 0
+        for ent in entries:


This loops more than necessary and is somewhat hard to follow. Consider
using for-else here:

for ...:
     ...
     if okay:
         break
else:
     raise error

I simplified things a bit but a for/else won't work here as we need to
check all ranges all the time. It is perfectly fine to not fit into a
range, as long as it fits into SOME range.

Well, that's how for's (not if's) else clause works -- it's executed
after all the looping's done if you didn't `break` out.
http://docs.python.org/2/tutorial/controlflow.html#break-and-continue-statements-and-else-clauses-on-loops


Maybe I'm just used to it and it's too esoteric to the average reader,
though.

Thanks for the vote of confidence. Like I said, I wanted it to check all
the ranges. A for/else quits on the break, which I guess is really
probably ok assuming we trust that nothing else is going to stuff bad
ranges in. I can go ahead and make the change.

Your code also breaks out as soon as a good range is found.
Anyway this is a small nitpick; the loop works fine as it is.

[...]
Ok, I'll drop this since it doesn't affect things with the new LDAP
backend.

Please do, you left it in by mistake.

Yeah, there it is sitting unsquashed in my tree :-(
>
I also added one change related to the LDAP core changes. In the past if
you did not have a ticket it would prompt for DM password. This was
broken after the updates. I added an additional except in
test_connection().

This should also be fixed soon in ipaldap. Thanks for putting up with
the changes.


So should I drop this in my patch then? I don't really like having to
import ldap.

You can if you're fine with waiting until my patches are pushed.
Otherwise it's covered by https://fedorahosted.org/freeipa/ticket/3499

--
PetrĀ³

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to