Re: [Freeipa-devel] [PATCH 0033] Remove trivial path constants
On 11/03/2014 01:54 PM, Petr Spacek wrote: On 4.10.2014 01:58, Gabe Alford wrote: Thanks Petr. Updated patch attached. Petr^3, is it okay now? Oh my, this patch fell through the cracks during the release frenzy. Apologies! ACK, pushed to master: 7eca640ffa3e661140843d91dc4a846d3355a242 Petr^2 Spacek On Tue, Sep 30, 2014 at 10:59 AM, Petr Viktorin pvikt...@redhat.com wrote: On 09/30/2014 05:13 AM, Gabe Alford wrote: Updated patch to fix merge conflicts from recent changes. On Wed, Sep 24, 2014 at 8:34 PM, Gabe Alford redhatri...@gmail.com mailto:redhatri...@gmail.com wrote: Hello, Patch for https://fedorahosted.org/freeipa/ticket/4399. Let me know if I missed any. Thanks, Gabe -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0033] Remove trivial path constants
On 4.10.2014 01:58, Gabe Alford wrote: Thanks Petr. Updated patch attached. Petr^3, is it okay now? Petr^2 Spacek On Tue, Sep 30, 2014 at 10:59 AM, Petr Viktorin pvikt...@redhat.com wrote: On 09/30/2014 05:13 AM, Gabe Alford wrote: Updated patch to fix merge conflicts from recent changes. On Wed, Sep 24, 2014 at 8:34 PM, Gabe Alford redhatri...@gmail.com mailto:redhatri...@gmail.com wrote: Hello, Patch for https://fedorahosted.org/freeipa/ticket/4399. Let me know if I missed any. Thanks, Gabe Thanks for the patch, and sorry for the delay! ipaserver/tools/ipa-upgradeconfig: The `filename` and `ca_file` aren't module-level constants; I think in this case they improve readability. The ticket calls for removing module-level lines like: NSSWITCH_CONF = paths.NSSWITCH_CONF which are just silly, but assigning a name locally to a global constant is a valid thing to do -- even if the local name just says the file we're working on now. -ca_file = paths.ALIAS_CACERT_ASC -if os.path.exists(ca_file): +if os.path.exists(paths.SYSCONFIG_HTTPD): Whoops! install/wsgi/plugins.py: -PLUGINS_DIR = paths.IPA_JS_PLUGINS_DIR - [...] -if not os.path.isdir(PLUGINS_DIR): +if not os.path.isdir(paths.IPA_CA_CSR): Whoops too! ipaplatform/fedora/tasks.py: ipa-client/ipa-install/ipa-client-install: ipaserver/install/dsinstance.py: ipaserver/install/httpinstance.py: Again, I'd not change the target_fname, filepath. ipapython/sysrestore.py: Again, `SYSRESTORE_PATH` describes better what we do with `paths.TMP`, so I'd prefer keeping it. ipaserver/install/adtrustinstance.py: I don't think we want to convert the self.* to constants. ipaserver/install/certs.py: I'd leave NSS_DIR as it is, rather than lose the comment. ipapython/ipautil.py: ipaserver/install/ldapupdate.py: ipalib/session.py: ipaserver/install/bindinstance.py: SHARE_DIR, UPDATES_DIR, and krbccache_dir, NAMED_CONF (respectively) need to stay, unless you also replace them in everything that uses them. Be sure to run make-lint after doing these changes. I've rebased, and I made some of the changes as I went along the review. You can base another revision on the attached patch. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0033] Remove trivial path constants
Thanks Petr. Updated patch attached. On Tue, Sep 30, 2014 at 10:59 AM, Petr Viktorin pvikt...@redhat.com wrote: On 09/30/2014 05:13 AM, Gabe Alford wrote: Updated patch to fix merge conflicts from recent changes. On Wed, Sep 24, 2014 at 8:34 PM, Gabe Alford redhatri...@gmail.com mailto:redhatri...@gmail.com wrote: Hello, Patch for https://fedorahosted.org/freeipa/ticket/4399. Let me know if I missed any. Thanks, Gabe Thanks for the patch, and sorry for the delay! ipaserver/tools/ipa-upgradeconfig: The `filename` and `ca_file` aren't module-level constants; I think in this case they improve readability. The ticket calls for removing module-level lines like: NSSWITCH_CONF = paths.NSSWITCH_CONF which are just silly, but assigning a name locally to a global constant is a valid thing to do -- even if the local name just says the file we're working on now. -ca_file = paths.ALIAS_CACERT_ASC -if os.path.exists(ca_file): +if os.path.exists(paths.SYSCONFIG_HTTPD): Whoops! install/wsgi/plugins.py: -PLUGINS_DIR = paths.IPA_JS_PLUGINS_DIR - [...] -if not os.path.isdir(PLUGINS_DIR): +if not os.path.isdir(paths.IPA_CA_CSR): Whoops too! ipaplatform/fedora/tasks.py: ipa-client/ipa-install/ipa-client-install: ipaserver/install/dsinstance.py: ipaserver/install/httpinstance.py: Again, I'd not change the target_fname, filepath. ipapython/sysrestore.py: Again, `SYSRESTORE_PATH` describes better what we do with `paths.TMP`, so I'd prefer keeping it. ipaserver/install/adtrustinstance.py: I don't think we want to convert the self.* to constants. ipaserver/install/certs.py: I'd leave NSS_DIR as it is, rather than lose the comment. ipapython/ipautil.py: ipaserver/install/ldapupdate.py: ipalib/session.py: ipaserver/install/bindinstance.py: SHARE_DIR, UPDATES_DIR, and krbccache_dir, NAMED_CONF (respectively) need to stay, unless you also replace them in everything that uses them. Be sure to run make-lint after doing these changes. I've rebased, and I made some of the changes as I went along the review. You can base another revision on the attached patch. -- Petr³ From 71cd9bc46f21506226906783ce46665e4c520237 Mon Sep 17 00:00:00 2001 From: Gabe redhatri...@gmail.com Date: Fri, 3 Oct 2014 17:27:57 -0600 Subject: [PATCH] Remove trivial path constants from modules https://fedorahosted.org/freeipa/ticket/4399 --- install/tools/ipa-server-install| 22 +-- install/wsgi/plugins.py | 6 +-- ipa-client/ipa-install/ipa-client-automount | 61 + ipa-client/ipa-install/ipa-client-install | 18 - ipapython/certmonger.py | 3 -- ipaserver/install/dsinstance.py | 9 ++--- ipaserver/install/ipa_backup.py | 5 +-- ipaserver/install/ipa_restore.py| 3 +- ipaserver/install/sysupgrade.py | 5 +-- ipaserver/install/upgradeinstance.py| 5 +-- 10 files changed, 57 insertions(+), 80 deletions(-) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index 89d73304fbf7797c73f4f6251ff96c17a761d8af..38377c82da09fa51f9b413a0e26dae235143b6a8 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -401,31 +401,29 @@ def signal_handler(signum, frame): dsinstance.erase_ds_instance_data (ds.serverid) sys.exit(1) -ANSWER_CACHE = paths.ROOT_IPA_CACHE - def read_cache(dm_password): Returns a dict of cached answers or empty dict if no cache file exists. -if not ipautil.file_exists(ANSWER_CACHE): +if not ipautil.file_exists(paths.ROOT_IPA_CACHE): return {} top_dir = tempfile.mkdtemp(ipa) fname = %s/cache % top_dir try: -decrypt_file(ANSWER_CACHE, fname, dm_password, top_dir) +decrypt_file(paths.ROOT_IPA_CACHE, fname, dm_password, top_dir) except Exception, e: shutil.rmtree(top_dir) -raise Exception(Decryption of answer cache in %s failed, please check your password. % ANSWER_CACHE) +raise Exception(Decryption of answer cache in %s failed, please check your password. % paths.ROOT_IPA_CACHE) try: with open(fname, 'rb') as f: try: optdict = pickle.load(f) except Exception, e: -raise Exception(Parse error in %s: %s % (ANSWER_CACHE, str(e))) +raise Exception(Parse error in %s: %s % (paths.ROOT_IPA_CACHE, str(e))) except IOError, e: -raise Exception(Read error in %s: %s % (ANSWER_CACHE, str(e))) +raise Exception(Read error in %s: %s % (paths.ROOT_IPA_CACHE, str(e))) finally: shutil.rmtree(top_dir) @@ -446,7 +444,7 @@ def write_cache(options): try: with open(fname, 'wb') as f: pickle.dump(options, f) -ipautil.encrypt_file(fname,
Re: [Freeipa-devel] [PATCH 0033] Remove trivial path constants
Hello Gabe, Thanks for the patch! And thank you for being patient, most people are focusing on wrapping up FreeIPA 4.1 release, so the review forces are limited. Martin On 09/30/2014 05:13 AM, Gabe Alford wrote: Updated patch to fix merge conflicts from recent changes. On Wed, Sep 24, 2014 at 8:34 PM, Gabe Alford redhatri...@gmail.com wrote: Hello, Patch for https://fedorahosted.org/freeipa/ticket/4399. Let me know if I missed any. Thanks, Gabe ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0033] Remove trivial path constants
On 09/30/2014 05:13 AM, Gabe Alford wrote: Updated patch to fix merge conflicts from recent changes. On Wed, Sep 24, 2014 at 8:34 PM, Gabe Alford redhatri...@gmail.com mailto:redhatri...@gmail.com wrote: Hello, Patch for https://fedorahosted.org/freeipa/ticket/4399. Let me know if I missed any. Thanks, Gabe Thanks for the patch, and sorry for the delay! ipaserver/tools/ipa-upgradeconfig: The `filename` and `ca_file` aren't module-level constants; I think in this case they improve readability. The ticket calls for removing module-level lines like: NSSWITCH_CONF = paths.NSSWITCH_CONF which are just silly, but assigning a name locally to a global constant is a valid thing to do -- even if the local name just says the file we're working on now. -ca_file = paths.ALIAS_CACERT_ASC -if os.path.exists(ca_file): +if os.path.exists(paths.SYSCONFIG_HTTPD): Whoops! install/wsgi/plugins.py: -PLUGINS_DIR = paths.IPA_JS_PLUGINS_DIR - [...] -if not os.path.isdir(PLUGINS_DIR): +if not os.path.isdir(paths.IPA_CA_CSR): Whoops too! ipaplatform/fedora/tasks.py: ipa-client/ipa-install/ipa-client-install: ipaserver/install/dsinstance.py: ipaserver/install/httpinstance.py: Again, I'd not change the target_fname, filepath. ipapython/sysrestore.py: Again, `SYSRESTORE_PATH` describes better what we do with `paths.TMP`, so I'd prefer keeping it. ipaserver/install/adtrustinstance.py: I don't think we want to convert the self.* to constants. ipaserver/install/certs.py: I'd leave NSS_DIR as it is, rather than lose the comment. ipapython/ipautil.py: ipaserver/install/ldapupdate.py: ipalib/session.py: ipaserver/install/bindinstance.py: SHARE_DIR, UPDATES_DIR, and krbccache_dir, NAMED_CONF (respectively) need to stay, unless you also replace them in everything that uses them. Be sure to run make-lint after doing these changes. I've rebased, and I made some of the changes as I went along the review. You can base another revision on the attached patch. -- Petr³ From a04e19cfd767fa5a994030a6ecf0b3f74fcaa903 Mon Sep 17 00:00:00 2001 From: Gabe redhatri...@gmail.com Date: Mon, 29 Sep 2014 17:23:25 -0600 Subject: [PATCH] Remove trivial path constants from modules https://fedorahosted.org/freeipa/ticket/4399 --- .../certmonger/dogtag-ipa-ca-renew-agent-submit| 8 ++- install/tools/ipa-adtrust-install | 8 ++- install/tools/ipa-ca-install | 5 +- install/tools/ipa-dns-install | 8 ++- install/tools/ipa-replica-conncheck| 9 ++-- install/tools/ipa-replica-install | 6 +-- install/tools/ipa-server-install | 39 ++ install/tools/ipa-upgradeconfig| 30 +-- install/wsgi/plugins.py| 6 +-- ipa-client/ipa-install/ipa-client-automount| 62 ++ ipa-client/ipa-install/ipa-client-install | 37 ++--- ipa-client/ipaclient/ntpconf.py| 28 +- ipalib/session.py | 4 +- ipaplatform/fedora/tasks.py| 6 +-- ipapython/certmonger.py| 8 +-- ipapython/ipautil.py | 1 - ipaserver/dcerpc.py| 3 +- ipaserver/install/adtrustinstance.py | 3 +- ipaserver/install/bindinstance.py | 23 ipaserver/install/dsinstance.py| 9 ++-- ipaserver/install/ipa_backup.py| 7 +-- ipaserver/install/ipa_replica_prepare.py | 15 +++--- ipaserver/install/ipa_restore.py | 3 +- ipaserver/install/sysupgrade.py| 5 +- ipaserver/install/upgradeinstance.py | 5 +- ipaserver/rpcserver.py | 5 +- 26 files changed, 140 insertions(+), 203 deletions(-) diff --git a/install/certmonger/dogtag-ipa-ca-renew-agent-submit b/install/certmonger/dogtag-ipa-ca-renew-agent-submit index 4f0b78accac6840471f8b2e9f17288b3b4e82105..942ffec65d7b041fc6f9d3b2c19d3596fae79d31 100755 --- a/install/certmonger/dogtag-ipa-ca-renew-agent-submit +++ b/install/certmonger/dogtag-ipa-ca-renew-agent-submit @@ -71,8 +71,7 @@ def request_cert(): syslog.syslog(syslog.LOG_NOTICE, Forwarding request to dogtag-ipa-renew-agent) -path = paths.DOGTAG_IPA_RENEW_AGENT_SUBMIT -args = [path] + sys.argv[1:] +args = [paths.DOGTAG_IPA_RENEW_AGENT_SUBMIT] + sys.argv[1:] stdout, stderr, rc = ipautil.run(args, raiseonerr=False, env=os.environ) sys.stderr.write(stderr) sys.stderr.flush() @@ -282,12 +281,11 @@ def export_csr(): if not cert: return (REJECTED, New certificate requests not supported) -csr_file = paths.IPA_CA_CSR try: -with open(csr_file, 'wb') as f: +with
Re: [Freeipa-devel] [PATCH 0033] Remove trivial path constants
Updated patch to fix merge conflicts from recent changes. On Wed, Sep 24, 2014 at 8:34 PM, Gabe Alford redhatri...@gmail.com wrote: Hello, Patch for https://fedorahosted.org/freeipa/ticket/4399. Let me know if I missed any. Thanks, Gabe From 51ea42d4ecb16a425ea4ff6871ed9e0d5ce67327 Mon Sep 17 00:00:00 2001 From: Gabe redhatri...@gmail.com Date: Mon, 29 Sep 2014 17:23:25 -0600 Subject: [PATCH] Remove trivial path constants from modules https://fedorahosted.org/freeipa/ticket/4399 --- .../certmonger/dogtag-ipa-ca-renew-agent-submit| 8 +-- install/tools/ipa-adtrust-install | 8 +-- install/tools/ipa-ca-install | 5 +- install/tools/ipa-dns-install | 8 +-- install/tools/ipa-replica-conncheck| 9 ++- install/tools/ipa-replica-install | 6 +- install/tools/ipa-server-install | 39 +--- install/tools/ipa-upgradeconfig| 30 - install/wsgi/plugins.py| 6 +- ipa-client/ipa-install/ipa-client-automount| 62 +-- ipa-client/ipa-install/ipa-client-install | 72 ++ ipa-client/ipaclient/ntpconf.py| 28 - ipalib/session.py | 5 +- ipaplatform/fedora/tasks.py| 44 ++--- ipapython/certmonger.py| 8 +-- ipapython/ipautil.py | 3 - ipapython/sysrestore.py| 5 +- ipaserver/dcerpc.py| 3 +- ipaserver/install/adtrustinstance.py | 13 ++-- ipaserver/install/bindinstance.py | 23 +++ ipaserver/install/certs.py | 6 +- ipaserver/install/dsinstance.py| 35 +-- ipaserver/install/httpinstance.py | 32 -- ipaserver/install/ipa_backup.py| 7 +-- ipaserver/install/ipa_replica_prepare.py | 15 ++--- ipaserver/install/ldapupdate.py| 3 - ipaserver/install/sysupgrade.py| 5 +- ipaserver/install/upgradeinstance.py | 5 +- ipaserver/rpcserver.py | 5 +- 29 files changed, 203 insertions(+), 295 deletions(-) diff --git a/install/certmonger/dogtag-ipa-ca-renew-agent-submit b/install/certmonger/dogtag-ipa-ca-renew-agent-submit index 4f0b78accac6840471f8b2e9f17288b3b4e82105..942ffec65d7b041fc6f9d3b2c19d3596fae79d31 100755 --- a/install/certmonger/dogtag-ipa-ca-renew-agent-submit +++ b/install/certmonger/dogtag-ipa-ca-renew-agent-submit @@ -71,8 +71,7 @@ def request_cert(): syslog.syslog(syslog.LOG_NOTICE, Forwarding request to dogtag-ipa-renew-agent) -path = paths.DOGTAG_IPA_RENEW_AGENT_SUBMIT -args = [path] + sys.argv[1:] +args = [paths.DOGTAG_IPA_RENEW_AGENT_SUBMIT] + sys.argv[1:] stdout, stderr, rc = ipautil.run(args, raiseonerr=False, env=os.environ) sys.stderr.write(stderr) sys.stderr.flush() @@ -282,12 +281,11 @@ def export_csr(): if not cert: return (REJECTED, New certificate requests not supported) -csr_file = paths.IPA_CA_CSR try: -with open(csr_file, 'wb') as f: +with open(paths.IPA_CA_CSR, 'wb') as f: f.write(csr) except Exception, e: -return (UNREACHABLE, Failed to write %s: %s % (csr_file, e)) +return (UNREACHABLE, Failed to write %s: %s % (paths.IPA_CA_CSR, e)) return (ISSUED, cert) diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install index 6e55bbe3e57f1c609398dc571e90cb8677d91a33..aabd695b8c63856875a5daa2a3b2aef70980973d 100755 --- a/install/tools/ipa-adtrust-install +++ b/install/tools/ipa-adtrust-install @@ -33,8 +33,6 @@ from ipaplatform.paths import paths from ipapython.ipa_log_manager import * from ipapython.dn import DN -log_file_name = paths.IPASERVER_INSTALL_LOG - def parse_options(): parser = IPAOptionParser(version=version.VERSION) parser.add_option(-d, --debug, dest=debug, action=store_true, @@ -213,8 +211,8 @@ def main(): if os.getegid() != 0: sys.exit(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 +standard_logging_setup(paths.IPASERVER_INSTALL_LOG, debug=options.debug, filemode='a') +print \nThe log file for this installation can be found in %s % paths.IPASERVER_INSTALL_LOG root_logger.debug('%s was invoked with options: %s' % (sys.argv[0], safe_options)) root_logger.debug(missing options might be asked for interactively later\n) @@ -452,5 +450,5 @@ information return 0 if __name__ == '__main__': -run_script(main, log_file_name=log_file_name, +