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

[Martin Kosek]

On 05/30/2013 04:35 PM, Tomas Babej wrote:
> On 05/29/2013 12:25 PM, Martin Kosek wrote:
>> 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
> 
> Sure, I changed the error class in idrange.py and tests accordingly.
> 
> I ran the unit tests again to verify the changes.
> 
> Here is the updated patch.
> 
> Tomas

ACK. Pushed to master, ipa-3-2.

Martin