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

[Stanislav Laznicka]

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.

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