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

[Martin Kosek]

On 06/12/2013 11:40 AM, Tomas Babej wrote:
> 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.

You can even use just "value = dictionary.get(keyword)" as None is the default
return value:

>>> print {}.get("foo")
None

Martin