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

[Tomas Babej]

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