[Freeipa-devel] [PATCH 424] install: Introduce installer framework ipapython.install

[Martin Basti]

On 08/06/15 12:12, Jan Cholasta wrote:
> Dne 3.6.2015 v 15:02 Martin Basti napsal(a):
>> On 02/06/15 15:21, Jan Cholasta wrote:
>>> Dne 11.5.2015 v 13:41 Jan Cholasta napsal(a):
>>>> Dne 6.5.2015 v 08:22 Jan Cholasta napsal(a):
>>>>> Dne 6.5.2015 v 08:11 Martin Kosek napsal(a):
>>>>>> On 04/29/2015 06:25 PM, Jan Cholasta wrote:
>>>>>>> Dne 20.4.2015 v 16:56 Jan Cholasta napsal(a):
>>>>>>>> Dne 20.4.2015 v 15:14 Martin Basti napsal(a):
>>>>>>>>> On 17/04/15 16:15, Jan Cholasta wrote:
>>>>>>>>>> Dne 16.4.2015 v 16:46 Jan Cholasta napsal(a):
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> the attached patch adds the basics of the new installer
>>>>>>>>>>> framework.
>>>>>>>>>>>
>>>>>>>>>>> As a next step, I plan to convert the install scripts to use 
>>>>>>>>>>> the
>>>>>>>>>>> framework with their old code (the old code will be gradually
>>>>>>>>>>> ported to
>>>>>>>>>>> the framework later).
>>>>>>>>>>>
>>>>>>>>>>> (Note I didn't manage to write docstrings today, expect update
>>>>>>>>>>> tomorrow.)
>>>>>>>>>>
>>>>>>>>>> Added some docstrings.
>>>>>>>>>>
>>>>>>>>>> Also updated the patch to reflect little brainstorming David 
>>>>>>>>>> and I
>>>>>>>>>> had
>>>>>>>>>> this morning.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Honza
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> Hello, see comments bellow:
>>>>>>>>>
>>>>>>>>> 1) We started using new shorter License header in files:
>>>>>>>>> #
>>>>>>>>> # Copyright (C) 2015  FreeIPA Contributors see COPYING for 
>>>>>>>>> license
>>>>>>>>> #
>>>>>>>>
>>>>>>>> OK.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 2) IMO this will not work, NoneType has no 'obj' attribute
>>>>>>>>> +        else:
>>>>>>>>> +            if isinstance(value, from_):
>>>>>>>>> +                value = None
>>>>>>>>> +                stack.append(value.obj)
>>>>>>>>> +                continue
>>>>>>>>
>>>>>>>> Right.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 3) Multiple inheritance. I do not like it much.
>>>>>>>>> +class CompositeInstaller(Installer, CompositeConfigurator):
>>>>>>>>
>>>>>>>> I guess you are antagonistic to multiple inheritance because of 
>>>>>>>> how
>>>>>>>> other languages (like C++) do it. In Python it can be pretty 
>>>>>>>> elegant
>>>>>>>> and
>>>>>>>> is basis for e.g. the mixin design pattern.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Installer and CompositeConfigurator inherites from Configurator
>>>>>>>>> class,
>>>>>>>>> and all of them implements _generator method.
>>>>>>>>
>>>>>>>> Both of them call super()._generator(), so it's no problem 
>>>>>>>> (same for
>>>>>>>> other methods).
>>>>>>>>
>>>>>>>>>
>>>>>>>>> If I understand correctly
>>>>>>>>> (https://www.python.org/download/releases/2.3/mro/) the
>>>>>>>>> Installer._generator method will be used in this case.
>>>>>>>>> However in case when CompositeConfigurator has more levels
>>>>>>>>> (respectively
>>>>>>>>> it is more specialized) of inheritance, it could take precedence
>>>>>>>>> and its
>>>>>>>>> _generator method may be used instead.
>>>>>>>>
>>>>>>>> The order of precedence is defined by the order of base classes
>>>>>>>> in the
>>>>>>>> class definition.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> I'm afraid this may suddenly stop working.
>>>>>>>>> Maybe I'm wrong, please fix me.
>>>>>>>>
>>>>>>>> As long as you call the super class, it will work fine.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> And Multiple inheritance is not easily readable, this is even a
>>>>>>>>> diamond
>>>>>>>>> inheritance model.
>>>>>>>>
>>>>>>>> Cooperative inheritance is used by design and IMHO is easily
>>>>>>>> readable if
>>>>>>>> you know how to read it. Every class defines a single bit of
>>>>>>>> behavior.
>>>>>>>> Without cooperative inheritance, it would have to be hardcoded
>>>>>>>> and/or
>>>>>>>> hacked around, which I wanted to avoid.
>>>>>>>>
>>>>>>>> This blog post explains it nicely:
>>>>>>>> <https://rhettinger.wordpress.com/2011/05/26/super-considered-super/>. 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> Updated patch attached.
>>>>>>>
>>>>>>> Also attached is patch 425 which migrates ipa-server-install to the
>>>>>>> install
>>>>>>> framework.
>>>>>>
>>>>>> Good job there. I am just curious, will this framework and new 
>>>>>> option
>>>>>> processing be friendly to other types of option passing than just 
>>>>>> via
>>>>>> options?
>>>>>> I mean tickets
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/4517
>>>>>> https://fedorahosted.org/freeipa/ticket/4468
>>>>>>
>>>>>> Especially 4517 is important, we need to be able to run
>>>>>>
>>>>>> # cat install.conf
>>>>>> ds_password=Secret123
>>>>>> admin_password=Secret456
>>>>>> ip_address=123456
>>>>>> setup_dns=False
>>>>>>
>>>>>> # ipa-server-install --unattended --conf install.conf
>>>>>>
>>>>>> I assume yes, but I am just making sure.
>>>>>
>>>>> Yes, definitely.
>>>>>
>>>>
>>>> Updated patches attached.
>>>
>>> Another update, patches attached.
>>>
>> thank you,
>>
>> 1)
>> ipa-server-install --uninstall prints 0
>> ...
>> Unconfiguring ipa_memcached
>> Unconfiguring ipa-otpd
>> 0
>> The ipa-server-install command was successful
>
> Fixed.
>
>>
>>
>> 2)
>> ipa-server-install --setup-dns
>> 'ServerOptions' object has no attribute 'dnssec_master'
>
> Fixed.
>
>>
>> 3)
>> For record, this will be fixed in extra patch.
>> info messages from ldapupdate are printed to console
>
> Could you provide the patch?
>
>>
>> 4)
>> +    if default is not _missing:
>> +        class_dict['default'] = default
>>
>> Why is new _missing object needed? Isn't NoneType enough?
>
> None is a valid value here, there needs to be a distinction between 
> "value is not set" and "value is set to None".
>
> Updated patches attached. Note you first have to apply my patches 
> 436-438.
>

NACK

Please provide a realm name [ABC.EXAMPLE.COM]:
'installer(Server)' object has no attribute 'unattended'

also please fix private_ccache import. This function should be imported 
directly from ipautil, not via installutils

-- 
Martin Basti