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

[Ana Krivokapic]

On 06/07/2013 12:15 PM, Tomas Babej wrote:
> On 05/31/2013 12:08 PM, Ana Krivokapic wrote:
>> Hello,
>>
>> This patch addresses ticket https://fedorahosted.org/freeipa/ticket/3635.
>>
>>
>>
>> _______________________________________________
>> Freeipa-devel mailing list
>> Freeipa-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>
> Hi, the patch itself works as it should, I have only some refactoring comments:
>
> 1.) There already is a add_range method that is called from within trust_add's
> execute()
> and handles some range validation (currently only whether domain's SID of new and
> existing range are equal, my patch 67 add some more).
>
> Your patch introduces a new method validate_range() that is called from execute(),
> which leads to some duplication of the logic, mainly two same calls to the API
> (getting
> the info about the old range via idrange_show)
>
> I'd rather we keep all the validation in one place, preferably inside the
> add_range method
> or refactored out of it in the new method that is called only from add_range().
>
> 2.) That being said, other parts of refactoring are nice and make code more
> clear, +1.
>
> Tomas

My first instinct was to place this validation in the add_range() method.
However, add_range() is called after the trust itself is added, and validation
introduced by this patch needs to happen before the trust is added (we need to
prevent the addition of trust in case the validation fails).

As for the code duplication, you are right about that. I tried to minimize
duplication - resulting updated patch attached.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130607/7e2d6cc4/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0033-02-Fail-when-adding-a-trust-with-a-different-range.patch
Type: text/x-patch
Size: 4641 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130607/7e2d6cc4/attachment.bin>