Re: [Freeipa-devel] [PATCH 0033] Remove trivial path constants

2014-11-04 Thread Petr Viktorin

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

2014-11-03 Thread Petr Spacek

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

2014-10-03 Thread Gabe Alford
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

2014-09-30 Thread Martin Kosek
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

2014-09-30 Thread Petr Viktorin

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

2014-09-29 Thread Gabe Alford
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,
+