[Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range

[Alexander Bokovoy]

On Mon, 10 Jun 2013, Ana Krivokapic wrote:
>> And once here(added by your patch):
>>
>> +        if not self.validate_range(*keys, **options):
>> +            raise errors.ValidationError(
>> +                name=_('id range'),
>> +                error=_(
>> +                    'An id range already exists for this trust. '
>> +                    'You should either delete the old range, or '
>> +                    'exclude --base-id/--range-size options from the
>> command.'
>>
>> I'd suggest we have one common place, say validate_range() function as
>> we have now,
>> that contains all the checks (more coming in the future for sure).
>>
>> That would mean that the whole trust-add command will fail in the case
>> that "ID range
>> with the same name but different domain SID already exists", since we
>> now call validate_range()
>> within execute() method. I checked with Alexander and we agreed that
>> this is more desired behaviour.
>>
>> This would also result to reducement of the number of API calls, which
>> is a nice benefit.
>>
>> Tomas
>
>This updated patch completely separates validation logic and object
>creation logic of the trust_add command. I added two new methods:
>
>* validate_range(), which ensures that the options and environment
>related to idrange is valid, and
>* validate_options, which takes care of other general validation
>
>One change introduced in this patch is making the
>__populate_remote_domain() method of TrustDomainJoins class public, and
>calling it from trust_add. This was necessary in order to enable
>discovering details of the trusted domain in validation phase.
Looks good overall but I'd suggest to wrap populate_remote_domain()
calls in join_ad_* methods instead of removing them. If remote domain is
not initialized (i.e. not(isinstance(self.remote_domain,TrustedDomainInstance)),
then call populate_remote_domain() as it was before.

-- 
/ Alexander Bokovoy