[Freeipa-devel] [PATCH 0057] Do not allow removal of ID range of an active trust

[Martin Kosek]

On 05/28/2013 03:48 PM, Alexander Bokovoy wrote:
> On Tue, 28 May 2013, Tomas Babej wrote:
>> On 05/28/2013 02:35 PM, Alexander Bokovoy wrote:
>>> On Mon, 27 May 2013, Tomas Babej wrote:
>>>>>>> We got rid of openldap utilities now. While using python.ldap module, I
>>>>>>> also made the tests much more robust and added a new test case.
>>>>>> In general patches look fine, there is one small nitpick.
>>>>>> I'll run tests on Monday and then will provide final ACK.
>>>>>>
>>>>>>> --- a/tests/test_xmlrpc/test_range_plugin.py
>>>>>>> +++ b/tests/test_xmlrpc/test_range_plugin.py
>>>>>>> @@ -22,66 +22,171 @@ Test the `ipalib/plugins/idrange.py` module, and
>>>>>>> XML-RPC in general.
>>>>>>> """
>>>>>>>
>>>>>>> from ipalib import api, errors, _
>>>>>>> +from ipapython.ipautil import run
>>>>>> This import is unused, can be removed.
>>>>>>
>>>>> Fixed, thanks for catching that.
>>>>>
>>>>> Updated patch attached.
>>> So I tried to run this test on a machine where there is already trust
>>> established and I think there should be done some changes.
>>
>> I perused the log. Seems that the failures you're experiencing are not
>> relevant to the patch itself,
>> since the newly added tests passed.
>>
>> This is problem with test_range_plugin.py tests that has been there for quite
>> a while, the parameters
>> of the ranges such as size, and base ID/RID/secondary RID are hardcoded in
>> the test case.
> Yep.
> 
> 
>>> Probably it would be wise to add pre-start procedure to pull existing
>>> ranges and define constants for the ranges so that they don't overlap
>>> with existing ones. Perhaps selecting something from a top of the range
>>> space...
>>>
>>> Attached is the log
>>
>> I agree. This has not been relevant until now, since we did not do much
>> testing on IPA instances
>> with trusts set up, and even then there's random factor in having the overlap
>> with the already created
>> trust range.
>>
>> I'd propose fixing this in a separate effort as a part of continouous
>> integration improvements. I see it
>> as a separate issue of its own.
>>
>> What do you think?
> Please file a separate ticket then.
> 
> ACK for this one.
> 

May-be-NACK.

Would it make sense to replace the error with DependentEntry error? We use in
cases like this elsewhere and I think it makes more sense in this case too.

Martin