[Freeipa-devel] [PATCHES 466-468] install: Add common base class for server and replica install

[Martin Babinsky]

On 09/16/2015 10:44 AM, Jan Cholasta wrote:
> On 16.9.2015 08:11, Jan Cholasta wrote:
>> On 15.9.2015 07:22, Jan Cholasta wrote:
>>> On 10.8.2015 16:58, Martin Babinsky wrote:
>>>> On 08/06/2015 08:22 AM, Jan Cholasta wrote:
>>>>> Hi,
>>>>>
>>>>> the attached patch fixes part of
>>>>> <https://fedorahosted.org/freeipa/ticket/4517>.
>>>>>
>>>>> See also Martin Babinsky's patch 51:
>>>>> <https://www.redhat.com/archives/freeipa-devel/2015-August/msg00012.html>.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Honza
>>>>>
>>>>
>>>> Sorry but NACK, see below:
>>>>
>>>> 1.) it seems that passing kwargs to Server components doesn't work as
>>>> expected. See these logs (install on fresh F22 VM):
>>>>
>>>> http://fpaste.org/253416/21363814/
>>>> http://fpaste.org/253419/43921374/
>>>
>>> Fixed.
>>>
>>>>
>>>> 2.) the following code blows up in BaseServers' __init__:
>>>> (http://fpaste.org/253400/21225314/)
>>>>
>>>> 392         if not self.dns.setup_dns:
>>>> 393             if self.dns.forwarders:
>>>> 394                 raise RuntimeError(
>>>> 395                     "You cannot specify a --forwarder option
>>>> without
>>>> the "
>>>> 396                     "--setup-dns option")
>>>>
>>>>
>>>> I think that the check should be:
>>>>
>>>> 392         if not self.setup_dns:
>>>> 393             if self.dns.forwarders:
>>>
>>> Fixed.
>>>
>>>>
>>>> IMHO BaseServerDNS class shouldn't have setup_dns knob, that should be
>>>> set in the parent class (BaseServer)
>>>
>>> Fixed.
>>>
>>>>
>>>> 3.) Is there any reason why BaseServer doesn't have 'master_password',
>>>> 'idmax' and 'idstart' knobs? I know that these are then brought in by
>>>> the derived Server class, but the check for them is in parent's
>>>> __init__() method and it is IMHO a bit confusing
>>>
>>> The check should be in Server, fixed.
>>>
>>>>
>>>> 4.) please add license header to the beginning of
>>>> 'ipaserver/install/server/common.py' file
>>>
>>> Added.
>>>
>>> Updated patches attached.
>>
>> Self-NACK, I broke ipa-server-install --uninstall.
>
> Fixed.
>

ACK to all three patches.

-- 
Martin^3 Babinsky