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

[Stanislav Laznicka]

On 07/28/2016 10:57 AM, Martin Basti wrote:
> On 17.06.2016 13:56, Stanislav Laznicka wrote:
>> On 06/17/2016 01:01 PM, Petr Vobornik wrote:
>>> 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.
>>>
>> Attached is the patch with correct modules with sys.exits replaced.
>>
>> I double-checked the changes and I believe the behavior shouldn't 
>> really change.
>>
>>
>>
>
> Hello,
>
> suprisingly, patch needs rebase :)
>
> 1)
> Is the script error the right Exception?
I chose ScriptError because it's able to change the return value of the 
script, which is necessary sometimes. RuntimeError, which may seem more 
suitable, would not be able to do that. However, I am open for ideas on 
which exception type to use.
>
> 2)
> Can you use rather raise Exception(), instead of raise Exception
>
Sure, please see the modified attached patch.
> 3)
> I really hate to print errors to STDOUT from modules and then just 
> call exit(1) (duplicated evil), could you replace print('xxx') with 
> raise AnException('xxx')
Did that, only kept those prints printing directly to stderr. Not sure 
if those should be changed as well.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160802/72808c49/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-slaznick-0048-3-Remove-sys.exit-from-install-modules-and-scripts.patch
Type: text/x-patch
Size: 43236 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160802/72808c49/attachment.bin>