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

[Stanislav Laznicka]

On 08/16/2016 01:03 PM, Martin Basti wrote:



On 16.08.2016 12:06, Stanislav Laznicka wrote:

On 08/16/2016 08:36 AM, Stanislav Laznicka wrote:

On 08/02/2016 01:08 PM, Stanislav Laznicka wrote:

On 07/28/2016 10:57 AM, Martin Basti wrote:

Hello,

suprisingly, patch needs rebase :)

1)
Is the script error the right Exception?
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.


2)
Can you use rather raise Exception(), instead of raise Exception


Sure, please see the modified attached patch.

3)
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')
Did that, only kept those prints printing directly to stderr. Not 
sure if those should be changed as well.



Bumping for review/opinions.

Also a self-NACK, there were some sys imports that should not have 
been there anymore (thanks, mbasti). Attaching a fixed version.




You use RuntimeError in bindinstance.py, in other files you use 
ScriptError, is this RuntimeError on purpose there?


Martin^2


I used it there as there was one other function in the module that would 
raise it, too. Adding a patch that raises ScriptError in 
bindinstance.check_reverse_zones() as well if you think raising 
ScriptError here would be more appropriate.


From 844b22add517477df49162a73c70b0358e08d0eb Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Fri, 17 Jun 2016 13:14:49 +0200
Subject: [PATCH] Remove sys.exit from install modules and scripts

sys.exit() calls sometimes make it hard to find bugs and mask code that
does not always work properly.

https://fedorahosted.org/freeipa/ticket/5750
---
 ipaserver/install/bindinstance.py  |   3 +-
 ipaserver/install/ca.py|  42 +-
 ipaserver/install/cainstance.py|   5 +-
 ipaserver/install/dns.py   |   5 +-
 ipaserver/install/dsinstance.py|   4 +-
 ipaserver/install/installutils.py  |  20 ++---
 ipaserver/install/ipa_ldap_updater.py  |   4 +-
 ipaserver/install/krainstance.py   |   4 +-
 ipaserver/install/replication.py   |  10 ++-
 ipaserver/install/server/install.py|  75 -
 ipaserver/install/server/replicainstall.py | 129 ++---
 11 files changed, 149 insertions(+), 152 deletions(-)

diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 7538e145cbe37dfc21963d97dea0e835e3bd5072..5c3bdac1a48d9d7e149babf0193d57b522aa9988 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -44,6 +44,7 @@ from ipapython import dnsutil
 from ipapython.dnsutil import DNSName
 from ipapython.ipa_log_manager import root_logger
 from ipapython.dn import DN
+from ipapython.admintool import ScriptError
 import ipalib
 from ipalib import api, errors
 from ipalib.constants import IPA_CA_RECORD
@@ -473,7 +474,7 @@ def check_reverse_zones(ip_addresses, reverse_zones, options, unattended,
 except ValueError as e:
 msg = "Reverse zone %s will not be used: %s" % (rz, e)
 if unattended:
-sys.exit(msg)
+raise ScriptError(msg)
 else:
 root_logger.warning(msg)
 continue
diff --git a/ipaserver/install/ca.py b/ipaserver/install/ca.py
index bce804ac1c4e3eaf8dd08bed894ad45ea2d73ae1..00e0b038ca03320fd7b8268fb3eb96c5bc50a3ac 100644
--- a/ipaserver/install/ca.py
+++ b/ipaserver/install/ca.py
@@ -4,10 +4,9 @@
 
 from __future__ import print_function
 
-import sys
-
 from ipaserver.install import cainstance, dsinstance, bindinstance
 from ipapython import ipautil, certdb
+from ipapython.admintool import ScriptError
 from ipaplatform import services
 from ipaplatform.paths import paths
 from ipaserver.install import installutils, certs
@@ -30,12 +29,11 @@ def install_check(standalone, replica_config, options):
 
 if replica_config is not None:
 if standalone and api.env.ra_plugin == 'selfsign':
-sys.exit('A selfsign CA can not be added')
+raise ScriptError('A selfsign CA can not be added')
 
 if ((not options.promote
  and not ipautil.file_exists(replica_config.dir + "/cacert.p12"))):
-print('CA cannot be installed in CA-less setup.')
-sys.exit(1)
+raise ScriptError('CA cannot be installed in CA-less setup.')
 
 if standalone and not options.skip_conncheck:
 principal = options.principal
@@ -53,7 +51,7 @@ def install_check(standalone, replica_config, options):
 
 if standalone:
 if api.Command.ca_is_enabled()['result']:
-sys.exit(
+raise ScriptError(
  

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

[Martin Basti]

On 16.08.2016 12:06, Stanislav Laznicka wrote:

On 08/16/2016 08:36 AM, Stanislav Laznicka wrote:

On 08/02/2016 01:08 PM, Stanislav Laznicka wrote:

On 07/28/2016 10:57 AM, Martin Basti wrote:

Hello,

suprisingly, patch needs rebase :)

1)
Is the script error the right Exception?
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.


2)
Can you use rather raise Exception(), instead of raise Exception


Sure, please see the modified attached patch.

3)
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')
Did that, only kept those prints printing directly to stderr. Not 
sure if those should be changed as well.



Bumping for review/opinions.

Also a self-NACK, there were some sys imports that should not have 
been there anymore (thanks, mbasti). Attaching a fixed version.




You use RuntimeError in bindinstance.py, in other files you use 
ScriptError, is this RuntimeError on purpose there?


Martin^2

--
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

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

[Stanislav Laznicka]

On 08/16/2016 08:36 AM, Stanislav Laznicka wrote:

On 08/02/2016 01:08 PM, Stanislav Laznicka wrote:

On 07/28/2016 10:57 AM, Martin Basti wrote:

Hello,

suprisingly, patch needs rebase :)

1)
Is the script error the right Exception?
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.


2)
Can you use rather raise Exception(), instead of raise Exception


Sure, please see the modified attached patch.

3)
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')
Did that, only kept those prints printing directly to stderr. Not 
sure if those should be changed as well.



Bumping for review/opinions.

Also a self-NACK, there were some sys imports that should not have been 
there anymore (thanks, mbasti). Attaching a fixed version.


From 4617dbff9f8c358f4e528e1a8961ab7eb63cc2bd Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Fri, 17 Jun 2016 13:14:49 +0200
Subject: [PATCH] Remove sys.exit from install modules and scripts

sys.exit() calls sometimes make it hard to find bugs and mask code that
does not always work properly.

https://fedorahosted.org/freeipa/ticket/5750
---
 ipaserver/install/bindinstance.py  |   2 +-
 ipaserver/install/ca.py|  42 +-
 ipaserver/install/cainstance.py|   5 +-
 ipaserver/install/dns.py   |   5 +-
 ipaserver/install/dsinstance.py|   4 +-
 ipaserver/install/installutils.py  |  20 ++---
 ipaserver/install/ipa_ldap_updater.py  |   4 +-
 ipaserver/install/krainstance.py   |   4 +-
 ipaserver/install/replication.py   |  10 ++-
 ipaserver/install/server/install.py|  75 -
 ipaserver/install/server/replicainstall.py | 129 ++---
 11 files changed, 148 insertions(+), 152 deletions(-)

diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 7538e145cbe37dfc21963d97dea0e835e3bd5072..c0a4647d0544a8aab1fa6701722d4b0ffe6163d5 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -473,7 +473,7 @@ def check_reverse_zones(ip_addresses, reverse_zones, options, unattended,
 except ValueError as e:
 msg = "Reverse zone %s will not be used: %s" % (rz, e)
 if unattended:
-sys.exit(msg)
+raise RuntimeError(msg)
 else:
 root_logger.warning(msg)
 continue
diff --git a/ipaserver/install/ca.py b/ipaserver/install/ca.py
index bce804ac1c4e3eaf8dd08bed894ad45ea2d73ae1..00e0b038ca03320fd7b8268fb3eb96c5bc50a3ac 100644
--- a/ipaserver/install/ca.py
+++ b/ipaserver/install/ca.py
@@ -4,10 +4,9 @@
 
 from __future__ import print_function
 
-import sys
-
 from ipaserver.install import cainstance, dsinstance, bindinstance
 from ipapython import ipautil, certdb
+from ipapython.admintool import ScriptError
 from ipaplatform import services
 from ipaplatform.paths import paths
 from ipaserver.install import installutils, certs
@@ -30,12 +29,11 @@ def install_check(standalone, replica_config, options):
 
 if replica_config is not None:
 if standalone and api.env.ra_plugin == 'selfsign':
-sys.exit('A selfsign CA can not be added')
+raise ScriptError('A selfsign CA can not be added')
 
 if ((not options.promote
  and not ipautil.file_exists(replica_config.dir + "/cacert.p12"))):
-print('CA cannot be installed in CA-less setup.')
-sys.exit(1)
+raise ScriptError('CA cannot be installed in CA-less setup.')
 
 if standalone and not options.skip_conncheck:
 principal = options.principal
@@ -53,7 +51,7 @@ def install_check(standalone, replica_config, options):
 
 if standalone:
 if api.Command.ca_is_enabled()['result']:
-sys.exit(
+raise ScriptError(
 "One or more CA masters are already present in IPA realm "
 "'%s'.\nIf you wish to replicate CA to this host, please "
 "re-run 'ipa-ca-install'\nwith a replica file generated on "
@@ -64,28 +62,28 @@ def install_check(standalone, replica_config, options):
 if not cainstance.is_step_one_done():
 # This can happen if someone passes external_ca_file without
 # already having done the first stage of the CA install.
-print("CA is not installed yet. To install with an external CA "
+raise ScriptError(
+  "CA is not installed yet. To install with an external CA "
   "is a two-stage process.\nFirst run the installer with "
 

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

[Stanislav Laznicka]

On 08/02/2016 01:08 PM, Stanislav Laznicka wrote:

On 07/28/2016 10:57 AM, Martin Basti wrote:

Hello,

suprisingly, patch needs rebase :)

1)
Is the script error the right Exception?
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.


2)
Can you use rather raise Exception(), instead of raise Exception


Sure, please see the modified attached patch.

3)
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')
Did that, only kept those prints printing directly to stderr. Not sure 
if those should be changed as well.



Bumping for review/opinions.

--
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

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

[Stanislav Laznicka]

On 07/28/2016 10:57 AM, Martin Basti wrote:

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:

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.






Hello,

suprisingly, patch needs rebase :)

1)
Is the script error the right Exception?
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.


2)
Can you use rather raise Exception(), instead of raise Exception


Sure, please see the modified attached patch.

3)
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')
Did that, only kept those prints printing directly to stderr. Not sure 
if those should be changed as well.
From d4e820b4e59da1f2ecc6810cbb3353d6f9d53053 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Fri, 17 Jun 2016 13:14:49 +0200
Subject: [PATCH] Remove sys.exit from install modules and scripts

sys.exit() calls sometimes make it hard to find bugs and mask code that
does not always work properly.

https://fedorahosted.org/freeipa/ticket/5750
---
 ipaserver/install/bindinstance.py  |   2 +-
 ipaserver/install/ca.py|  40 -
 ipaserver/install/cainstance.py|   5 +-
 ipaserver/install/dns.py   |   5 +-
 ipaserver/install/dsinstance.py|   3 +-
 ipaserver/install/installutils.py  |  20 ++---
 ipaserver/install/ipa_ldap_updater.py  |   3 +-
 

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

[Martin Basti]

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:

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.






Hello,

suprisingly, patch needs rebase :)

1)
Is the script error the right Exception?

2)
Can you use rather raise Exception(), instead of raise Exception

3)
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')


Martin

-- 
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

Re: [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.


From 08648ce78aef1e0868d6fbaffd23ed643fd49486 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Fri, 17 Jun 2016 13:14:49 +0200
Subject: [PATCH] Remove sys.exit from install modules and scripts

sys.exit() calls sometimes make it hard to find bugs and mask code that
does not always work properly.

https://fedorahosted.org/freeipa/ticket/5750
---
 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 --
 11 files changed, 118 insertions(+), 103 deletions(-)

diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 78e75359266bbefe7954242b98922272fb0c9194..c9e528856414371ce2e5e54df3642907c89d2856 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -466,7 +466,7 @@ def check_reverse_zones(ip_addresses, reverse_zones, options, unattended,
 except ValueError as e:
 msg = "Reverse zone %s will not be used: %s" % (rz, e)
 if unattended:
-sys.exit(msg)
+   

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

[Petr Vobornik]

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

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

[Stanislav Laznicka]

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.


I'll keep the sys.exit replaces that won't make it here on the side, we 
may want to do them later.


--
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

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

[Petr Spacek]

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.

In that case I'm perfectly fine with leaving some exits are they are.

-- 
Petr^2 Spacek

-- 
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

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

[Petr Vobornik]

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.

-- 
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

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

[Stanislav Laznicka]

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.


--
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

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

[Petr Spacek]

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.

-- 
Petr^2 Spacek

-- 
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

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

[Stanislav Laznicka]

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?


--
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

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

[Petr Spacek]

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...

-- 
Petr^2 Spacek

-- 
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

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

[Stanislav Laznicka]

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.


From 7968f068141e53f7bf111221b38c40cac432 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Thu, 16 Jun 2016 17:12:24 +0200
Subject: [PATCH] Remove sys.exit from install modules and scripts

sys.exit() calls sometimes make it hard to find bugs and mask code that
does not always work properly.

https://fedorahosted.org/freeipa/ticket/5750
---
 install/tools/ipa-adtrust-install  | 17 +++---
 install/tools/ipa-ca-install   | 23 
 install/tools/ipa-compat-manage| 11 ++--
 install/tools/ipa-dns-install  |  3 +-
 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 --
 15 files changed, 147 insertions(+), 128 deletions(-)

diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install
index 36caa5c2d429c6cf107df03e82aa80e15d8efe01..da635cb02c3c8affb234515dd64f2cb06e9ea872 100755
--- a/install/tools/ipa-adtrust-install
+++ b/install/tools/ipa-adtrust-install
@@ -37,6 +37,7 @@ from ipaserver.install.installutils import (
 from ipaserver.install import service
 from ipapython import version
 from ipapython import ipautil, sysrestore, ipaldap
+from ipapython.admintool import ScriptError
 from ipalib import api, errors, krb_utils
 from ipapython.config import IPAOptionParser
 from ipaplatform.paths import paths
@@ -192,7 +193,7 @@ def set_and_check_netbios_name(netbios_name, unattended):
 if not adtrustinstance.check_netbios_name(netbios_name):
 if unattended and not gen_netbios_name:
 netbios_name_error(netbios_name)
-sys.exit("Aborting installation.")
+raise ScriptError("Aborting installation.")
 else:
 if netbios_name:
 netbios_name_error(netbios_name)
@@ -227,7 +228,7 @@ def main():
 safe_options, options = parse_options()
 
 if os.getegid() != 0:
-sys.exit("Must be root to setup AD trusts on server")
+raise ScriptError("Must be root to setup AD trusts on server")
 
 standard_logging_setup(log_file_name, debug=options.debug, filemode='a')
 print("\nThe log file for this installation can be found in %s" % log_file_name)
@@ -255,7 +256,7 @@ def main():
 
 # Check if samba packages are installed
 if not adtrustinstance.check_inst():
-sys.exit("Aborting installation.")
+raise ScriptError("Aborting installation.")
 
 # Initialize the ipalib api
 cfg = dict(
@@ -318,14 +319,14 @@ def main():
 try:
 principal = krb_utils.get_principal()
 except errors.CCacheError as e:
-sys.exit("Must have Kerberos credentials to setup AD trusts on server: %s" % e.message)
+raise ScriptError("Must have Kerberos credentials to setup AD trusts on server: %s" % e.message)
 
 try:
 api.Backend.ldap2.connect()
 except errors.ACIError as e:
-sys.exit("Outdated Kerberos credentials. Use kdestroy and kinit to update your ticket")
+raise ScriptError("Outdated Kerberos credentials. Use kdestroy and kinit to update your ticket")
 except errors.DatabaseError as e:
-sys.exit("Cannot connect to the LDAP database. Please check if IPA is running")
+raise ScriptError("Cannot connect to the LDAP database. Please check if IPA is running")
 
 try:
 user = api.Command.user_show(principal.partition('@')[0].partition('/')[0])['result']
@@ -334,9 +335,9 @@ def main():
 group['cn'][0] in user['memberof_group']):
 raise errors.RequirementError(name='admins group membership')
 except errors.RequirementError as e:
-sys.exit("Must have administrative privileges to setup AD trusts on server")
+raise ScriptError("Must have administrative privileges to setup AD trusts on server")
 except Exception as e:
-sys.exit("Unrecognized error during check of admin rights: %s" % (str(e)))
+raise ScriptError("Unrecognized error during check of admin rights: %s" % (str(e)))
 
 (netbios_name, reset_netbios_name) = \
 set_and_check_netbios_name(options.netbios_name,
diff --git a/install/tools/ipa-ca-install b/install/tools/ipa-ca-install
index