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

[Stanislav Laznicka]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-slaznick-0048-2-Remove-sys.exit-from-install-modules-and-scripts.patch
Type: text/x-patch
Size: 39881 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160617/5fff4abf/attachment.bin>