<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p><br>
</p>
<br>
<div class="moz-cite-prefix">On 17.06.2016 13:56, Stanislav Laznicka
wrote:<br>
</div>
<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>
<br>
2)<br>
Can you use rather raise Exception(), instead of raise Exception<br>
<br>
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')<br>
<br>
Martin<br>
<br>
</body>
</html>