<html> <head> <meta content="text/html; charset=UTF-8" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> On 06/10/2014 05:07 PM, Petr Viktorin wrote: <blockquote cite="mid:53971F35.5090006@redhat.com" type="cite">On 06/10/2014 10:13 AM, Tomas Babej wrote: <blockquote type="cite">Thank you for the detailed review. Responses to all the issues found are inline: </blockquote> I'm calling it a day now, but here are initial comments from reading the patches. <blockquote type="cite">On 06/06/2014 01:04 PM, Petr Viktorin wrote: <blockquote type="cite">On 06/05/2014 03:14 PM, Petr Viktorin wrote: <blockquote type="cite">On 06/04/2014 11:42 AM, Tomas Babej wrote: <blockquote type="cite">Hi, the following set of patches implements the ticket: <a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/4052">https://fedorahosted.org/freeipa/ticket/4052</a> </blockquote> </blockquote> </blockquote> </blockquote> [...] <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite"> 0202: OK </blockquote> </blockquote> </blockquote> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite">0203: It would be nice to have the Services' __init__s take an optional `api` argument, and then use `self.api` everywhere. I'm certain we'd appreciate it in the future. </blockquote> </blockquote> Added. </blockquote> Nice! Just one more thing. I don't think it's good practice to pass additional *args to superclasses, unless it's really some sequence of items. In this case you should use always name the arguments to prevent surprises, so **kwargs is enough. </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> Done! <blockquote cite="mid:53971F35.5090006@redhat.com" type="cite"> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite">0204: When commenting what a function does, use a docstring. </blockquote> </blockquote> Will be documented in a later patch. </blockquote> You mean an upcoming patchset, right? </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> Yep, documentation patch is attached. <blockquote cite="mid:53971F35.5090006@redhat.com" type="cite"> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite">0205: OK 0206: OK 0207: OK </blockquote> </blockquote> </blockquote> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite">0208: check_selinux_status, in the RuntimeError message, assumes that it's called from an installation. This should at least be pointed out in the docstring. </blockquote> </blockquote> </blockquote> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite">0209: ipa-client-install, --noac help: "Red Hat" has two words. Also it's a company; I don't think "Red Hat based distributions" is a correct use of the trademark. In comments/class names it doesn't really matter but in user-facing text we should try to be professional. We can either go with "Fedora-based" here and sort this out in a RHEL patch if needed, or better, adjust the help text (or visibility of the option) based on if the platform uses authconfig. </blockquote> </blockquote> I'm thinking we could go as far as to provide a way in the installers to define platform dependent options. What do you think? </blockquote> See Martin's mail. This patchset is already too long for a general solution here. You could file a ticket for a better fix so it's not forgotten. </blockquote> Fixed and I filed <a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/4377">https://fedorahosted.org/freeipa/ticket/4377</a> . <blockquote cite="mid:53971F35.5090006@redhat.com" type="cite"> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite">base.tasks: These need docstrings. Will those included in the documentation that you want to provide later? </blockquote> </blockquote> </blockquote> 0210: OK 0211: OK 0212: OK 0213: OK 0214: OK 0215: You could rename the commit now 0216: [...] <blockquote type="cite">As we discussed, to avoid cyclical imports, separate modules for paths and tasks are needed. Calling the paths_namespace object by its descriptive name is rather cumbersome, so I changed that to: from ipaplatform.paths import paths from ipaplatform.tasks import tasks </blockquote> OK I looked over the paths again, and saw this: SLAPD_INSTANCE_CONFIG = "/etc/dirsrv/slapd-" ETC_DIRSRV_SLAPD_INSTANCE = "/etc/dirsrv/slapd-%s" SLAPD_INSTANCE_CONFIG should be removed. ETC_PKI_TOMCAT_ALIAS = "/etc/pki/pki-tomcat/alias" PKI_TOMCAT_ALIAS_DIR = "/etc/pki/pki-tomcat/alias/" We only need one of these. </blockquote> I replaced both, it required some addtional refactoring though. <blockquote cite="mid:53971F35.5090006@redhat.com" type="cite"> <blockquote type="cite"> <blockquote type="cite">ipatests/test_xmlrpc/test_automount_plugin.py: this change is unnecessary </blockquote> </blockquote> I was talking about this one: - keyname = u'/home' + keyname = u'/home/' It's not only unnecessary, it also breaks a test. </blockquote> Fixed. <blockquote cite="mid:53971F35.5090006@redhat.com" type="cite"> 0217: OK 0218: OK 0219: Ok 0220: OK 0221: OK 0222: OK </blockquote> <pre class="moz-signature" cols="72">-- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org </pre> </body> </html>