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

[Tomas Babej]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130619/c22d3e8b/attachment.htm>