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

[Martin Basti]

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?

2)
Can you use rather raise Exception(), instead of raise Exception

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')

Martin

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160728/bae7267d/attachment.htm>