Re: [Freeipa-devel] [PATCH 0048] Remove sys.exit() from installer modules
On 16.08.2016 13:30, Stanislav Laznicka wrote: 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. ACK Pushed to master: 5776f1e9ccfc24689c99951864248ed01045 -- 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
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( "One o
Re: [Freeipa-devel] [PATCH 0048] Remove sys.exit() from installer modules
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
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 " "--external-ca.
Re: [Freeipa-devel] [PATCH 0048] Remove sys.exit() from installer modules
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
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 +- ipaserver/install/krainstance.py
Re: [Freeipa-devel] [PATCH 0048] Remove sys.exit() from installer modules
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
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) +raise RuntimeError(m
Re: [Freeipa-devel] [PATCH 0048] Remove sys.exit() from installer modules
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
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
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
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
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
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
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
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
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 1bc5def03bf687a1e4f9fb38a54363b5429c8fc4..3a863