On Thu, 2012-04-12 at 10:03 -0400, John Dennis wrote: > On 04/12/2012 04:17 AM, Martin Kosek wrote: > > On Wed, 2012-04-11 at 22:05 -0400, John Dennis wrote: > >> Revised patch attached. We'll leave the DN parameter changes till later. > >> This is essentially the same as the original patch with the addition of > >> the fixes necessary to support passing an empty container arg, an issue > >> Martin discovered in his review. FWIW the answer was not to make the > >> param required (actually it would have been adding the flag 'nonempty') > >> because you should be able to say you don't want to introduce a > >> container into the search bases (see commit comment) > >> > > > > I don't agree with the removal of default values for the containers and > > allowing an empty value for them. Please, see my reasoning: > > > > 1) I don't think its unlikely to have ou=People and ou=groups as > > containers for users/groups as they are default containers in fresh LDAP > > installs. I think most of the small LDAP deployments will use these > > values. > > > > 2) I am also not sure if somebody would want to pass empty user and > > group container. Users and groups won't be shared in the same container > > and since we search with _ldap.SCOPE_ONELEVEL the migration would not > > find users or groups in containers nested under the search base anyway. > > OK. Patch is revised, restored the defaults, usercontainer and > groupcontainer are now required to be non-empty. Also, basedn had been > optional without a default which didn't make much sense, now basedn is a > required parameter. >
I still have few issues: 1) basedn should not be required by default. When it is not supplied, we proactively check remote LDAP DSE and try to either read nsslapd-defaultnamingcontext or pick the first naming context to make the migration process easier for the end user. This is what migration plugin doc says: If a base DN is not provided with --basedn then IPA will use either the value of defaultNamingContext if it is set or the first value in namingContexts set in the root of the remote LDAP server. 2) I don't understand what's the advantage of using an optional param for usercontainer/groupcontainer and flag 'nonempty' compared to using a required param. IMHO, using a required param for this use case is perfectly appropriate as we indeed require these containers. Besides, 'nonempty' flag is quite rare - in fact its not used anywhere but migration plugin. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel