On 10.4.2012 17:03, John Dennis wrote:
On 04/06/2012 10:11 AM, John Dennis wrote:
On 04/06/2012 04:40 AM, Martin Kosek wrote:
1) We still crash when the parameter is empty. We may want to make it
required (the same fix Rob did for cert rejection reason):

# echo "secret123" | ipa migrate-ds ldap://vm-054.idm.lab.bos.redhat.com
--with-compat --base-dn="dc=greyoak,dc=com" --user-container=
ipa: ERROR: cannot connect to
u'http://vm-022.idm.lab.bos.redhat.com/ipa/xml': Internal Server Error

Good point, will fix.

2) Do you think it would make sense to create a special Param for DN?
Its quite general type and I bet there are other Params that could use
DN instead of Str. It could look like that:
DN('usercontainer?',
rdn=True,<<<< can be RDN, not DN

Yes, I considered introducing a new DN parameter type as well. I think
this is a good approach and will have payoff down the road. I will make
that change. However I'm inclined to introduce both a DN parameter and a
RDN parameter, they really are different entities, if you use a flag to
indicate you require a RDN then you lose the type of the parameter, it
could be either, and it really should be one or the other and knowing
which it is has value.

3) We should not restrict users from passing a user/group container with
more than one RDN:

Yeah, I wasn't too sure about that. The parameter distinctly called for
an RDN, but it seemed to me it should support any container below the
base, which would make it a DN not a RDN.

FWIW, a DN is an ordered sequence of 1 or more RDN's. DN's do not have
to be "absolute", they can be any subset of a "path sequence". A DN
with exactly one RDN is equivalent to an RDN, a special case).

I will change the parameter to be a DN since that is what was the
original intent.

All good suggestions, a revised patch will follow shortly.

Hmm... I'm rethinking this. The suggestion of adding a new DN parameter
is the right thing to do and I tried to implement it, but as I was
working I realized I needed to touch a fair amount of code to support
the new parameter type and to modify multiple places in the migration
code to work with the new type. That's more code changes at the very
last minute for this release than I'm comfortable with, we're too close
the freeze date for invasive modifications.

We have a work item open to introduce DN types throughout the code
including as a new Param type. I think it's best to make just a small
tweak to fix the traceback today and wait till the 3.0 work to "do it
right". Therefore I'm no longer in favor of the new Param approach. I
will fix the problem you reported when the parameter is empty, but merge
that into the original patch.


Just for the record, there are some related tickets: <https://fedorahosted.org/freeipa/ticket/2033>, <https://fedorahosted.org/freeipa/ticket/2265>.

Honza

--
Jan Cholasta

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to