On Sep 16, 2011, at 2:11 AM, Martin Kosek wrote: > On Thu, 2011-09-15 at 17:25 +0000, JR Aquino wrote: >> 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 >> > > Great, most bugs are fixed. I only saw these 2 minor bugs. If those are > fixed, I think we can ack&push. > > 1) Man pages: --list option is still not right, formating is wrong > +\fB\-l\fR, -\-list\fR
This typo is now corrected > > 2) Enable action is missing a notice for the user, like the disable > action has: > > # 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 > Disabling Plugin The output is now corrected. > # 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 I have now also corrected the --list / -e / --entry to support/display shorthand for the managed entries instead of the full DN. # ipa-managed-entries --list Available Managed Entry Definitions: UPG Definition NGP Definition # # ipa-managed-entries --entry="UPG Definition" status Plugin Enabled #
binDUXKL0365P.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