[Freeipa-devel] [PATCH 72] Validate DN & RDN parameters for migrate command

[Petr Viktorin]

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³