Martin Kosek wrote:
On Thu, 2011-09-15 at 00:47 +0000, JR Aquino wrote:
On Jul 22, 2011, at 7:05 AM, Martin Kosek wrote:

5) I was thinking if there is a better solution to enabling/disabling of
the plugin. Likes setting something like "managedEntryEnabled" attribute
to on/off as we do with compat plugin. Current concept with disabling
the definition by damaging the originFilter and then restoring it from
an LDIF seems a bit awkward to me.

This has been completely changed:
Instead of looking to ldif files, an ldap look up is now performed to 
dynamically list the available managed entries.

Now we are talking :-) I like the new approach.

I have reviewed your patch, basic functionality looks good. But I still
have few (nitpicking) comments:

1) There are parts from the previous file that are no longer needed
since you switched to different approach:

+import os

+    from ipaserver.install.ldapupdate import LDAPUpdate, BadSyntax

+    import StringIO

+    import ldif

+except BadSyntax, e:
+    print "There is a syntax error in this update file:"
+    print "  %s" % e
+    sys.exit(1)


2) I saw few whitespace errors on following lines of the patch: 419, 433
and 453

3) Output of the --list method is confusing:

# ipa-managed-entries --list
Directory Manager password:

Available Managed Entry Plugins:
cn=upg definition,cn=definitions,cn=managed
entries,cn=etc,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com
cn=ngp definition,cn=definitions,cn=managed
entries,cn=etc,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com

You must specify a managed entry definition<<<
# echo $?
1<<<

a) I shouldn't be asked to specify a managed entry definition for --list
b) The listing was successful, so we shouldn't return error code

4) Return code for disabling an already disabled entry should be 2
(according to man pages):

# ipa-managed-entries -e 'cn=upg definition,cn=definitions,cn=managed 
entries,cn=etc,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com' disable
Directory Manager password:

Plugin already disabled
# echo $?
0

5) Strange is, that enabling a disabled plugin gives me return code 2:
# ipa-managed-entries -e 'cn=upg definition,cn=definitions,cn=managed 
entries,cn=etc,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com' enable
Directory Manager password:

Enabling Plugin
# echo $?
2

Return codes for these actions should fit the man pages.

6) I would improve working with LDAP filters, current solution is error
prone. Try disabling&enabling NGP Defition, we end up with this
originFilter:

originfilter: (&(objectclass=ipahostgroup))

This is actually a legal filter.


I think the cleanest solution would be to use ldap.make_filter and
ldap.combine_filters functions to play with these filter. You can
inspire yourself in this example I wrote for DNS plugin:

rev_zone_filter = ldap.make_filter(search_kw, rules=ldap.MATCH_NONE, 
exact=False,
                     trailing_wildcard=False)
filter = ldap.combine_filters((rev_zone_filter, filter), rules=ldap.MATCH_ALL)

I talked to JR about this in irc yesterday and talked him out of using make_filter. We already know what every permutation of these filters is going to look like, building them dynamically seems like overkill.

7) Entering Directory Manager every time may be a bit tedious. Could we
use current Kerberos credentials and fall-back to asking Directory
Manager password if it doesn't work? Its already done this way in
ipa-replica-manage for example.

We could fix this, however, as an enhancement in another patch.

8) Man page - please use the new united FreeIPA man page header. Instead
of

+.TH "ipa-managed-entries" "1" "Sept 15 2011" "freeipa" ""

use:

+.TH "ipa-managed-entries" "1" "Sept 15 2011" "FreeIPA" "FreeIPA Manual
Pages"


9) Man page - comma is missing for --list option:

+\fB\-l\-\-list\fR


10) install/po/Makefile.in should be updated to: there is still
reference to ipa-host-net-manage and ipa-managed-entries reference is
missing


Martin

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

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

Reply via email to