<div dir="ltr">Thanks Petr. Updated patch attached. <div class="gmail_extra"> <div class="gmail_quote">On Tue, Sep 30, 2014 at 10:59 AM, Petr Viktorin <<a href="mailto:pviktori@redhat.com" target="_blank">pviktori@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 09/30/2014 05:13 AM, Gabe Alford wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Updated patch to fix merge conflicts from recent changes. On Wed, Sep 24, 2014 at 8:34 PM, Gabe Alford <<a href="mailto:redhatrises@gmail.com" target="_blank">redhatrises@gmail.com</a> <mailto:<a href="mailto:redhatrises@gmail.com" target="_blank">redhatrises@gmail.com</a>>> wrote: Hello, Patch for <a href="https://fedorahosted.org/freeipa/ticket/4399" target="_blank">https://fedorahosted.org/freeipa/ticket/4399</a>. Let me know if I missed any. Thanks, Gabe </blockquote> 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". <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - ca_file = paths.ALIAS_CACERT_ASC - if os.path.exists(ca_file): + if os.path.exists(paths.SYSCONFIG_HTTPD): </blockquote> Whoops! install/wsgi/plugins.py: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> -PLUGINS_DIR = paths.IPA_JS_PLUGINS_DIR - </blockquote> [...] <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - if not os.path.isdir(PLUGINS_DIR): + if not os.path.isdir(paths.IPA_CA_CSR): </blockquote> 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³ </blockquote></div> </div></div>