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 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
