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

[Tomas Babej]

On 06/12/2013 10:23 AM, Alexander Bokovoy wrote:
> 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.
>
I tested the patch and it works fine.

There's a lot of refactoring done, but the changes preserve equal state. 
Aside from
Alexander's comment up here, I have but one nitpick.

There are few constructs of the form:

try:
    value = dictionary['keyword']
except KeyError:
    value = None

or

if 'keyword' in dictionary:
     value = dictionary['keyword']
else:
     value = None

Can you please substitute these with "value = dictionary.get(keyword, 
None)"?
Not everywhere, but there are places where it can improve readability of 
the code.

All from me for now,

Tomas