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, ANSWER_CACHE, options['dm_password'], top_dir) + ipautil.encrypt_file(fname, paths.ROOT_IPA_CACHE, options['dm_password'], top_dir) except IOError, e: raise Exception("Unable to cache command-line options %s" % str(e)) finally: @@ -583,7 +581,7 @@ def uninstall(): tasks.restore_network_configuration(fstore, sstore) fstore.restore_all_files() try: - os.remove(ANSWER_CACHE) + os.remove(paths.ROOT_IPA_CACHE) except Exception: pass try: @@ -782,7 +780,7 @@ def main(): sys.exit(1) # This will override any settings passed in on the cmdline - if ipautil.file_exists(ANSWER_CACHE): + if ipautil.file_exists(paths.ROOT_IPA_CACHE): if options.dm_password is not None: dm_password = options.dm_password else: @@ -1343,8 +1341,8 @@ def main(): print "In order for Firefox autoconfiguration to work you will need to" print "use a SSL signing certificate. See the IPA documentation for more details." - if ipautil.file_exists(ANSWER_CACHE): - os.remove(ANSWER_CACHE) + if ipautil.file_exists(paths.ROOT_IPA_CACHE): + os.remove(paths.ROOT_IPA_CACHE) return 0 if __name__ == '__main__': diff --git a/install/wsgi/plugins.py b/install/wsgi/plugins.py index 82b35eb438f7915e0672cbc116fa8344a2704bf4..64df08765d6ef9f2915282f6594902e0cbe05ea1 100644 --- a/install/wsgi/plugins.py +++ b/install/wsgi/plugins.py @@ -25,14 +25,12 @@ import os from ipaplatform.paths import paths from ipapython.ipa_log_manager import root_logger -PLUGINS_DIR = paths.IPA_JS_PLUGINS_DIR - def get_plugin_index(): - if not os.path.isdir(PLUGINS_DIR): + if not os.path.isdir(paths.IPA_JS_PLUGINS_DIR): raise Exception("Supplied plugin directory path is not a directory") - dirs = os.listdir(PLUGINS_DIR) + dirs = os.listdir(paths.IPA_JS_PLUGINS_DIR) index = 'define([],function(){return[' index += ','.join("'"+x+"'" for x in dirs) index += '];});' diff --git a/ipa-client/ipa-install/ipa-client-automount b/ipa-client/ipa-install/ipa-client-automount index 110e0ba13287e8c3061864b2e6c7b27d0ca83a6c..7b9e701dead5f50a033a455eb62e30df78cc0249 100755 --- a/ipa-client/ipa-install/ipa-client-automount +++ b/ipa-client/ipa-install/ipa-client-automount @@ -41,11 +41,6 @@ from ipaplatform.tasks import tasks from ipaplatform import services from ipaplatform.paths import paths -AUTOFS_CONF = paths.SYSCONFIG_AUTOFS -NSSWITCH_CONF = paths.NSSWITCH_CONF -AUTOFS_LDAP_AUTH = paths.AUTOFS_LDAP_AUTH_CONF -NFS_CONF = paths.SYSCONFIG_NFS -IDMAPD_CONF = paths.IDMAPD_CONF def parse_options(): usage = "%prog [options]\n" @@ -96,10 +91,10 @@ def wait_for_sssd(): def configure_xml(fstore): from lxml import etree - fstore.backup_file(AUTOFS_LDAP_AUTH) + fstore.backup_file(paths.AUTOFS_LDAP_AUTH_CONF) try: - f = open(AUTOFS_LDAP_AUTH, 'r') + f = open(paths.AUTOFS_LDAP_AUTH_CONF, 'r') lines = f.read() f.close() @@ -113,7 +108,7 @@ def configure_xml(fstore): root = element[0].getroottree() if len(element) != 1: - raise RuntimeError('Unable to parse %s' % AUTOFS_LDAP_AUTH) + raise RuntimeError('Unable to parse %s' % paths.AUTOFS_LDAP_AUTH_CONF) element[0].set('usetls', 'no') element[0].set('tlsrequired', 'no') @@ -121,20 +116,20 @@ def configure_xml(fstore): element[0].set('authtype', 'GSSAPI') element[0].set('clientprinc', 'host/%s@%s' % (api.env.host, api.env.realm)) - newconf = open(AUTOFS_LDAP_AUTH, 'w') + newconf = open(paths.AUTOFS_LDAP_AUTH_CONF, 'w') try: root.write(newconf, pretty_print=True, xml_declaration=True, encoding='UTF-8') newconf.close() except IOError, e: - print "Unable to write %s: %s" % (AUTOFS_LDAP_AUTH, e) - print "Configured %s" % AUTOFS_LDAP_AUTH + print "Unable to write %s: %s" % (paths.AUTOFS_LDAP_AUTH_CONF, e) + print "Configured %s" % paths.AUTOFS_LDAP_AUTH_CONF def configure_nsswitch(fstore, options): """ Point automount to ldap in nsswitch.conf. This function is for non-SSSD setups only """ - fstore.backup_file(NSSWITCH_CONF) + fstore.backup_file(paths.NSSWITCH_CONF) conf = ipachangeconf.IPAChangeConf("IPA Installer") conf.setOptionAssignment(':') @@ -144,9 +139,9 @@ def configure_nsswitch(fstore, options): opts = [{'name':'automount', 'type':'option', 'action':'set', 'value':nss_value}, {'name':'empty', 'type':'empty'}] - conf.changeConf(NSSWITCH_CONF, opts) + conf.changeConf(paths.NSSWITCH_CONF, opts) - print "Configured %s" % NSSWITCH_CONF + print "Configured %s" % paths.NSSWITCH_CONF def configure_autofs_sssd(fstore, statestore, autodiscover, options): try: @@ -221,11 +216,11 @@ def configure_autofs(fstore, statestore, autodiscover, server, options): } ipautil.backup_config_and_replace_variables(fstore, - AUTOFS_CONF, replacevars=replacevars) - tasks.restore_context(AUTOFS_CONF) + paths.SYSCONFIG_AUTOFS, replacevars=replacevars) + tasks.restore_context(paths.SYSCONFIG_AUTOFS) statestore.backup_state('autofs', 'sssd', False) - print "Configured %s" % AUTOFS_CONF + print "Configured %s" % paths.SYSCONFIG_AUTOFS def configure_autofs_common(fstore, statestore, options): autofs = services.knownservices.autofs @@ -244,16 +239,16 @@ def configure_autofs_common(fstore, statestore, options): def uninstall(fstore, statestore): print "Restoring configuration" - if fstore.has_file(AUTOFS_CONF): - fstore.restore_file(AUTOFS_CONF) - if fstore.has_file(NSSWITCH_CONF): - fstore.restore_file(NSSWITCH_CONF) - if fstore.has_file(AUTOFS_LDAP_AUTH): - fstore.restore_file(AUTOFS_LDAP_AUTH) - if fstore.has_file(NFS_CONF): - fstore.restore_file(NFS_CONF) - if fstore.has_file(IDMAPD_CONF): - fstore.restore_file(IDMAPD_CONF) + if fstore.has_file(paths.SYSCONFIG_AUTOFS): + fstore.restore_file(paths.SYSCONFIG_AUTOFS) + if fstore.has_file(paths.NSSWITCH_CONF): + fstore.restore_file(paths.NSSWITCH_CONF) + if fstore.has_file(paths.AUTOFS_LDAP_AUTH_CONF): + fstore.restore_file(paths.AUTOFS_LDAP_AUTH_CONF) + if fstore.has_file(paths.SYSCONFIG_NFS): + fstore.restore_file(paths.SYSCONFIG_NFS) + if fstore.has_file(paths.IDMAPD_CONF): + fstore.restore_file(paths.IDMAPD_CONF) if statestore.has_state('autofs'): enabled = statestore.restore_state('autofs', 'enabled') running = statestore.restore_state('autofs', 'running') @@ -314,19 +309,19 @@ def configure_nfs(fstore, statestore): 'SECURE_NFS': 'yes', } ipautil.backup_config_and_replace_variables(fstore, - NFS_CONF, replacevars=replacevars) - tasks.restore_context(NFS_CONF) + paths.SYSCONFIG_NFS, replacevars=replacevars) + tasks.restore_context(paths.SYSCONFIG_NFS) - print "Configured %s" % NFS_CONF + print "Configured %s" % paths.SYSCONFIG_NFS replacevars = { 'Domain': api.env.domain, } ipautil.backup_config_and_replace_variables(fstore, - IDMAPD_CONF, replacevars=replacevars) - tasks.restore_context(IDMAPD_CONF) + paths.IDMAPD_CONF, replacevars=replacevars) + tasks.restore_context(paths.IDMAPD_CONF) - print "Configured %s" % IDMAPD_CONF + print "Configured %s" % paths.IDMAPD_CONF rpcidmapd = services.knownservices.rpcidmapd statestore.backup_state('rpcidmapd', 'enabled', rpcidmapd.is_enabled()) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index 2e59df99551dfb457e8ca19a31d51b103f59825a..dd6edda2d9c70f8d9b1666fd8bfe53d09549803a 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -70,10 +70,6 @@ CLIENT_NOT_CONFIGURED = 2 CLIENT_ALREADY_CONFIGURED = 3 CLIENT_UNINSTALL_ERROR = 4 # error after restoring files/state -SSH_AUTHORIZEDKEYSCOMMAND = paths.SSS_SSH_AUTHORIZEDKEYS -SSH_PROXYCOMMAND = paths.SSS_SSH_KNOWNHOSTSPROXY -SSH_KNOWNHOSTSFILE = paths.SSSD_PUBCONF_KNOWN_HOSTS - client_nss_nickname_format = 'IPA Machine Certificate - %s' def parse_options(): @@ -1276,9 +1272,9 @@ def configure_ssh_config(fstore, options): 'PubkeyAuthentication': 'yes', } - if options.sssd and file_exists(SSH_PROXYCOMMAND): - changes['ProxyCommand'] = '%s -p %%p %%h' % SSH_PROXYCOMMAND - changes['GlobalKnownHostsFile'] = SSH_KNOWNHOSTSFILE + if options.sssd and file_exists(paths.SSS_SSH_KNOWNHOSTSPROXY): + changes['ProxyCommand'] = '%s -p %%p %%h' % paths.SSS_SSH_KNOWNHOSTSPROXY + changes['GlobalKnownHostsFile'] = paths.SSSD_PUBCONF_KNOWN_HOSTS if options.trust_sshfp: changes['VerifyHostKeyDNS'] = 'yes' changes['HostKeyAlgorithms'] = 'ssh-rsa,ssh-dss' @@ -1304,20 +1300,20 @@ def configure_sshd_config(fstore, options): 'UsePAM': 'yes', } - if options.sssd and file_exists(SSH_AUTHORIZEDKEYSCOMMAND): + if options.sssd and file_exists(paths.SSS_SSH_AUTHORIZEDKEYS): authorized_keys_changes = None candidates = ( { - 'AuthorizedKeysCommand': SSH_AUTHORIZEDKEYSCOMMAND, + 'AuthorizedKeysCommand': paths.SSS_SSH_AUTHORIZEDKEYS, 'AuthorizedKeysCommandUser': 'nobody', }, { - 'AuthorizedKeysCommand': SSH_AUTHORIZEDKEYSCOMMAND, + 'AuthorizedKeysCommand': paths.SSS_SSH_AUTHORIZEDKEYS, 'AuthorizedKeysCommandRunAs': 'nobody', }, { - 'PubKeyAgent': '%s %%u' % SSH_AUTHORIZEDKEYSCOMMAND, + 'PubKeyAgent': '%s %%u' % paths.SSS_SSH_AUTHORIZEDKEYS, 'PubKeyAgentRunAs': 'nobody', }, ) diff --git a/ipapython/certmonger.py b/ipapython/certmonger.py index bcfafdaf450903a1e67a0bcfa23d26d0ce311428..c56b0d631d2d7d1ebd66b50c6acda67f78cadae5 100644 --- a/ipapython/certmonger.py +++ b/ipapython/certmonger.py @@ -32,9 +32,6 @@ from ipapython.ipa_log_manager import * from ipaplatform.paths import paths from ipaplatform import services -REQUEST_DIR = paths.CERTMONGER_REQUESTS_DIR -CA_DIR = paths.CERTMONGER_CAS_DIR - DBUS_CM_PATH = '/org/fedorahosted/certmonger' DBUS_CM_IF = 'org.fedorahosted.certmonger' DBUS_CM_REQUEST_IF = 'org.fedorahosted.certmonger.request' diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py index c9a1c15e9495108382f6e2e8a58f6cc4e8f79c98..62eeabec90d542d4b7a5e7a86c30d3cafedc2db5 100644 --- a/ipaserver/install/dsinstance.py +++ b/ipaserver/install/dsinstance.py @@ -45,9 +45,6 @@ from ipapython.dn import DN from ipaplatform import services from ipaplatform.paths import paths -SERVER_ROOT_64 = paths.USR_LIB_DIRSRV_64 -SERVER_ROOT_32 = paths.USR_LIB_DIRSRV - DS_USER = 'dirsrv' DS_GROUP = 'dirsrv' @@ -69,10 +66,10 @@ ALL_SCHEMA_FILES = IPA_SCHEMA_FILES + ("05rfc2247.ldif", ) def find_server_root(): - if ipautil.dir_exists(SERVER_ROOT_64): - return SERVER_ROOT_64 + if ipautil.dir_exists(paths.USR_LIB_DIRSRV_64): + return paths.USR_LIB_DIRSRV_64 else: - return SERVER_ROOT_32 + return paths.USR_LIB_DIRSRV def realm_to_serverid(realm_name): return "-".join(realm_name.split(".")) diff --git a/ipaserver/install/ipa_backup.py b/ipaserver/install/ipa_backup.py index ded02171cbf62687620666d55c9802f9670b1942..94f85d3522b20820105d48bcbe684559fcbe3048 100644 --- a/ipaserver/install/ipa_backup.py +++ b/ipaserver/install/ipa_backup.py @@ -63,7 +63,6 @@ EOF --keyring /root/backup.pub --list-secret-keys """ -BACKUP_DIR = paths.IPA_BACKUP_DIR def encrypt_file(filename, keyring, remove_original=True): @@ -528,10 +527,10 @@ class Backup(admintool.AdminTool): ''' if data_only: - backup_dir = os.path.join(BACKUP_DIR, time.strftime('ipa-data-%Y-%m-%d-%H-%M-%S')) + backup_dir = os.path.join(paths.IPA_BACKUP_DIR, time.strftime('ipa-data-%Y-%m-%d-%H-%M-%S')) filename = os.path.join(backup_dir, "ipa-data.tar") else: - backup_dir = os.path.join(BACKUP_DIR, time.strftime('ipa-full-%Y-%m-%d-%H-%M-%S')) + backup_dir = os.path.join(paths.IPA_BACKUP_DIR, time.strftime('ipa-full-%Y-%m-%d-%H-%M-%S')) filename = os.path.join(backup_dir, "ipa-full.tar") os.mkdir(backup_dir, 0700) diff --git a/ipaserver/install/ipa_restore.py b/ipaserver/install/ipa_restore.py index 239de99c462639854e8e25c6b9278cb94b6fc6b8..7898de0f6f6613db95ea93bb4a91bd44a2c68951 100644 --- a/ipaserver/install/ipa_restore.py +++ b/ipaserver/install/ipa_restore.py @@ -41,7 +41,6 @@ from ipaserver.install import adtrustinstance from ipapython import ipaldap import ipapython.errors from ipaplatform.tasks import tasks -from ipaserver.install.ipa_backup import BACKUP_DIR from ipaplatform import services from ipaplatform.paths import paths @@ -144,7 +143,7 @@ class Restore(admintool.AdminTool): dirname = self.args[0] if not os.path.isabs(dirname): - self.backup_dir = os.path.join(BACKUP_DIR, dirname) + self.backup_dir = os.path.join(paths.IPA_BACKUP_DIR, dirname) else: self.backup_dir = dirname diff --git a/ipaserver/install/sysupgrade.py b/ipaserver/install/sysupgrade.py index 4ce652ca1ceeed212e918a17eb60629e38507773..19e017d904a67f1165f2054068612418029d4463 100644 --- a/ipaserver/install/sysupgrade.py +++ b/ipaserver/install/sysupgrade.py @@ -24,7 +24,6 @@ from ipapython import sysrestore from ipaplatform.paths import paths from ipapython.ipa_log_manager import * -STATEFILE_DIR = paths.STATEFILE_DIR STATEFILE_FILE = 'sysupgrade.state' _sstore = None @@ -32,7 +31,7 @@ _sstore = None def _load_sstore(): global _sstore if _sstore is None: - _sstore = sysrestore.StateFile(STATEFILE_DIR, STATEFILE_FILE) + _sstore = sysrestore.StateFile(paths.STATEFILE_DIR, STATEFILE_FILE) def get_upgrade_state(module, state): _load_sstore() @@ -51,6 +50,6 @@ def remove_upgrade_state(module, state): def remove_upgrade_file(): try: - os.remove(os.path.join(STATEFILE_DIR, STATEFILE_FILE)) + os.remove(os.path.join(paths.STATEFILE_DIR, STATEFILE_FILE)) except Exception, e: root_logger.debug('Cannot remove sysupgrade state file: %s', e) diff --git a/ipaserver/install/upgradeinstance.py b/ipaserver/install/upgradeinstance.py index b553721eba615f324f9e52331cbd0910c3e14be6..622f2b8cfca6aa7e07452868a8c9264dd3b0f88d 100644 --- a/ipaserver/install/upgradeinstance.py +++ b/ipaserver/install/upgradeinstance.py @@ -31,7 +31,6 @@ from ipaserver.install import schemaupdate from ipaserver.install import ldapupdate from ipaserver.install import service -DSBASE = paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE DSE = 'dse.ldif' class IPAUpgrade(service.Service): @@ -54,8 +53,8 @@ class IPAUpgrade(service.Service): ext += h service.Service.__init__(self, "dirsrv") serverid = dsinstance.realm_to_serverid(realm_name) - self.filename = '%s/%s' % (DSBASE % serverid, DSE) - self.savefilename = '%s/%s.ipa.%s' % (DSBASE % serverid, DSE, ext) + self.filename = '%s/%s' % (paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE % serverid, DSE) + self.savefilename = '%s/%s.ipa.%s' % (paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE % serverid, DSE, ext) self.live_run = live_run self.files = files self.modified = False -- 2.0.0
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
