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:
I wonder if yes/no should be replaced with KeyboardInterrupt or
This patch removes most sys.exits() from installer modules and
replaces them with ScriptError. I only left sys.exits at places
where the user
decides yes/no on continuation of the script.
I'm not sure, it seems more clear to just really exit if the user
and it's what we say we'll do (with possible cleanup
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
of having if (interrupt): cleanup; if (interrupt2): same_cleanup;
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
therefore needs cleanup. But that's just an opinion coming from
out loud as well. I went through the code to see if there's much
after these user actions and it seems that usually there's nothing
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
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.
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
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
Attached is the patch with correct modules with sys.exits replaced.
I double-checked the changes and I believe the behavior shouldn't
suprisingly, patch needs rebase :)
Is the script error the right Exception?
Can you use rather raise Exception(), instead of raise Exception
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
Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code