<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 07/28/2016 10:57 AM, Martin Basti
wrote:<br>
</div>
<blockquote
cite="mid:92bbb69d-cba7-a820-8254-00b684cdbada@redhat.com"
type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
On 17.06.2016 13:56, Stanislav Laznicka wrote:<br>
<blockquote
cite="mid:3b048d9c-03c8-1675-0a3f-20e4b8e293c2@redhat.com"
type="cite">On 06/17/2016 01:01 PM, Petr Vobornik wrote: <br>
<blockquote type="cite">On 17.6.2016 12:12, Stanislav Laznicka
wrote: <br>
<blockquote type="cite">On 06/17/2016 09:51 AM, Petr Vobornik
wrote: <br>
<blockquote type="cite">On 17.6.2016 09:24, Stanislav
Laznicka wrote: <br>
<blockquote type="cite">On 06/17/2016 08:48 AM, Petr
Spacek wrote: <br>
<blockquote type="cite">On 17.6.2016 08:43, Stanislav
Laznicka wrote: <br>
<blockquote type="cite">On 06/17/2016 07:45 AM, Petr
Spacek wrote: <br>
<blockquote type="cite">On 16.6.2016 17:33,
Stanislav Laznicka wrote: <br>
<blockquote type="cite">Hello, <br>
<br>
This patch removes most sys.exits() from
installer modules and <br>
scripts and <br>
replaces them with ScriptError. I only left
sys.exits at places <br>
where the user <br>
decides yes/no on continuation of the script. <br>
</blockquote>
I wonder if yes/no should be replaced with
KeyboardInterrupt or some <br>
other <br>
exception, too... <br>
<br>
</blockquote>
I'm not sure, it seems more clear to just really
exit if the user <br>
desires it <br>
and it's what we say we'll do (with possible cleanup
beforehand). Do <br>
you think <br>
we could benefit somehow by raising an exception
here? <br>
</blockquote>
I'm just thinking out loud. <br>
<br>
It seemed to me that it is easier to share cleanup on
one except block <br>
instead <br>
of having if (interrupt): cleanup; if (interrupt2):
same_cleanup; <br>
<br>
etc. <br>
<br>
Again, just wondering out loud. <br>
<br>
</blockquote>
If the cleanup is the same, or similar it might be more
beneficial to <br>
have it in a function where you could pass what was set
up already and <br>
therefore needs cleanup. But that's just an opinion
coming from thinking <br>
out loud as well. I went through the code to see if
there's much cleanup <br>
after these user actions and it seems that usually
there's nothing much <br>
if anything. However, thinking in advance may save us
much trouble in <br>
the future, of course. <br>
<br>
</blockquote>
Btw the original scope of the ticket is to replace
sys.exit calls ONLY <br>
in installer modules. Please don't waste time with
debugging other use <br>
cases before 4.4 is out. <br>
<br>
</blockquote>
I might have gotten carried away a bit. Would you suggest
keeping the <br>
sys.exits replaced only in
ipaserver/install/server/replicainstall.py, <br>
ipaserver/install/server/install.py modules which are the
installer <br>
modules managed by AdminTool? I considered the modules in <br>
ipaserver/install/ to also be installer modules as they are
heavily used <br>
during installation and the sys.exits there mainly occur in
functions <br>
called from install()/install_check() methods. The *-install
scripts <br>
were perhaps really obviously over the scope. <br>
</blockquote>
Yes, modules: <br>
ipaserver/install/bindinstance.py | 2 +- <br>
ipaserver/install/ca.py | 19 +++--- <br>
ipaserver/install/cainstance.py | 5 +- <br>
ipaserver/install/dns.py | 5 +- <br>
ipaserver/install/dsinstance.py | 3 +- <br>
ipaserver/install/installutils.py | 16 +++--- <br>
ipaserver/install/ipa_ldap_updater.py | 2 +- <br>
ipaserver/install/krainstance.py | 3 +- <br>
ipaserver/install/replication.py | 10 ++-- <br>
ipaserver/install/server/install.py | 64
+++++++++++---------- <br>
ipaserver/install/server/replicainstall.py | 92 <br>
<br>
not modules: <br>
install/tools/ipa-adtrust-install | 17 +++--- <br>
install/tools/ipa-ca-install | 23 ++++---- <br>
install/tools/ipa-compat-manage | 11 ++-- <br>
install/tools/ipa-dns-install | 3 +- <br>
<br>
<blockquote type="cite">I'll keep the sys.exit replaces that
won't make it here on the side, we <br>
may want to do them later. <br>
</blockquote>
I'm a bit worried that the patch might change some behavior.
Maybe I'm <br>
wrong. <br>
<br>
</blockquote>
Attached is the patch with correct modules with sys.exits
replaced. <br>
<br>
I double-checked the changes and I believe the behavior
shouldn't really change. <br>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
<br>
Hello,<br>
<br>
suprisingly, patch needs rebase :)<br>
<br>
1)<br>
Is the script error the right Exception?<br>
</blockquote>
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.<br>
<blockquote
cite="mid:92bbb69d-cba7-a820-8254-00b684cdbada@redhat.com"
type="cite"> <br>
2)<br>
Can you use rather raise Exception(), instead of raise Exception<br>
<br>
</blockquote>
Sure, please see the modified attached patch.<br>
<blockquote
cite="mid:92bbb69d-cba7-a820-8254-00b684cdbada@redhat.com"
type="cite"> 3)<br>
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')</blockquote>
Did that, only kept those prints printing directly to stderr. Not
sure if those should be changed as well.<br>
</body>
</html>