On Sep 15, 2011, at 1:47 AM, 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.
<high five> > > 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) Removed > > 2) I saw few whitespace errors on following lines of the patch: 419, 433 > and 453 Fixed whitespace errors > > 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 Fixed > b) The listing was successful, so we shouldn't return error code Corrected 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 Fixed error code > > 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. Fixed error code > > 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)) > > 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) Rob and you addressed this in the mailing list. For the record, I do agree that we are lacking a method for reading and modifying existing ldap filters. We will continue with the simple string method here for this iteration. > > 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. Fixed. We now will use gssapi if available, and prompt for password if there is no ticket. > > 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" Fixed > > > 9) Man page - comma is missing for --list option: > > +\fB\-l\-\-list\fR > Fixed > > 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 Fixed
bin1u2MkpIsph.bin
Description: freeipa-jraquino-0025-Create-Tool-for-Enabling-Disabling-Managed-Entries.patch
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel