[Freeipa-devel] [PATCH 0048] Remove sys.exit() from installer modules

[Petr Vobornik]

On 17.6.2016 12:12, Stanislav Laznicka wrote:
> On 06/17/2016 09:51 AM, Petr Vobornik wrote:
>> On 17.6.2016 09:24, Stanislav Laznicka wrote:
>>> On 06/17/2016 08:48 AM, Petr Spacek wrote:
>>>> On 17.6.2016 08:43, Stanislav Laznicka wrote:
>>>>> On 06/17/2016 07:45 AM, Petr Spacek wrote:
>>>>>> On 16.6.2016 17:33, Stanislav Laznicka wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> This patch removes most sys.exits() from installer modules and
>>>>>>> scripts and
>>>>>>> replaces them with ScriptError. I only left sys.exits at places
>>>>>>> where the user
>>>>>>> decides yes/no on continuation of the script.
>>>>>> I wonder if yes/no should be replaced with KeyboardInterrupt or some
>>>>>> other
>>>>>> exception, too...
>>>>>>
>>>>> I'm not sure, it seems more clear to just really exit if the user
>>>>> desires it
>>>>> and it's what we say we'll do (with possible cleanup beforehand). Do
>>>>> you think
>>>>> we could benefit somehow by raising an exception here?
>>>> I'm just thinking out loud.
>>>>
>>>> It seemed to me that it is easier to share cleanup on one except block
>>>> instead
>>>> of having if (interrupt): cleanup; if (interrupt2): same_cleanup;
>>>>
>>>> etc.
>>>>
>>>> Again, just wondering out loud.
>>>>
>>> If the cleanup is the same, or similar it might be more beneficial to
>>> have it in a function where you could pass what was set up already and
>>> therefore needs cleanup. But that's just an opinion coming from thinking
>>> out loud as well. I went through the code to see if there's much cleanup
>>> after these user actions and it seems that usually there's nothing much
>>> if anything. However, thinking in advance may save us much trouble in
>>> the future, of course.
>>>
>> Btw the original scope of the ticket is to replace sys.exit calls ONLY
>> in installer modules. Please don't waste time with debugging other use
>> cases before 4.4 is out.
>>
> I might have gotten carried away a bit. Would you suggest keeping the
> sys.exits replaced only in ipaserver/install/server/replicainstall.py,
> ipaserver/install/server/install.py modules which are the installer
> modules managed by AdminTool? I considered the modules in
> ipaserver/install/ to also be installer modules as they are heavily used
> during installation and the sys.exits there mainly occur in functions
> called from install()/install_check() methods. The *-install scripts
> were perhaps really obviously over the scope.

Yes, modules:
 ipaserver/install/bindinstance.py          |  2 +-
 ipaserver/install/ca.py                    | 19 +++---
 ipaserver/install/cainstance.py            |  5 +-
 ipaserver/install/dns.py                   |  5 +-
 ipaserver/install/dsinstance.py            |  3 +-
 ipaserver/install/installutils.py          | 16 +++---
 ipaserver/install/ipa_ldap_updater.py      |  2 +-
 ipaserver/install/krainstance.py           |  3 +-
 ipaserver/install/replication.py           | 10 ++--
 ipaserver/install/server/install.py        | 64 +++++++++++----------
 ipaserver/install/server/replicainstall.py | 92

not modules:
install/tools/ipa-adtrust-install | 17 +++---
install/tools/ipa-ca-install | 23 ++++----
install/tools/ipa-compat-manage | 11 ++--
install/tools/ipa-dns-install | 3 +-

> 
> I'll keep the sys.exit replaces that won't make it here on the side, we
> may want to do them later.

I'm a bit worried that the patch might change some behavior. Maybe I'm
wrong.

-- 
Petr Vobornik