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

[Petr Viktorin]

On 06/19/2013 10:07 AM, Tomas Babej wrote:
> On 06/13/2013 12:17 PM, Ana Krivokapic wrote:
>> On 06/12/2013 12:14 PM, Martin Kosek wrote:
>>> 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.
>> Fixed.
>>
>>>> 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")
>> Fixed.
>>
>>> None
>>>
>>> Martin
>>>
>>> _______________________________________________
>>> Freeipa-devel mailing list
>>> Freeipa-devel at redhat.com
>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>> Updated patch attached.
>>
>>
>>
>> _______________________________________________
>> Freeipa-devel mailing list
>> Freeipa-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>
> ACK
>
> Tomas

Pushed to master.


-- 
Petr³