On 04/13/2012 01:53 PM, Martin Kosek wrote:
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.
Update commands automatically replace required arguments with
`nonempty`-flagged ones. The flag should have no other use.
I added it in one of my early patches but didn't include documentation.
I'll send a docstring patch to make clear what I meant.
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
