<html> <head> <meta content="text/html; charset=UTF-8" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> Thank you for the detailed review. Responses to all the issues found are inline: <div class="moz-cite-prefix">On 06/06/2014 01:04 PM, Petr Viktorin wrote: </div> <blockquote cite="mid:5391A03D.1030408@redhat.com" 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> The refactoring touches both server and client bits, main features are: * easier inheritance and creation of new platform modules * all filesystem paths are defined as platform constants * platform related functionality is implemented as transparent platform tasks (as opposed to platform dependent implementations) * no need to implement your own authconfig class, since tasks encapsulate the relevant parts of the code More documentation, mainly on the part of the documenting the tasks contracts and the creation of own platform modules should follow later. </blockquote> Thanks for all the work! I didn't test yet; actually I haven't read it all yet, but sharing the first thoughts might make the review faster. If you'd rather have the whole thing, just wait :) 0202: OK 0203: Can we remove the leading underscores from `__wait_for_open_ports`? I don't think there's a good reason for that to be "private", let alone super-annoyingly "private". </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite"> 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 cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">In SystemdService.restart, I think the comment is obsolete by now. But If we want to keep it we should add a link to <a class="moz-txt-link-freetext" href="https://bugs.freedesktop.org/show_bug.cgi?id=39824">https://bugs.freedesktop.org/show_bug.cgi?id=39824</a> </blockquote> </blockquote> Removed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">Also, it's pointless to just fix whitespace issues and line length. If you're cleaning up, please convert the code to actually idiomatic Python, otherwise these are just changes for the sake of change. (Or maybe for the sake of passing automated style checks, which is pretty stupid. The real issues are those the tool doesn't report. There's a nice summary on using the `pep8` tool like this in the last paragraph of <a class="moz-txt-link-freetext" href="http://bugs.python.org/msg193909">http://bugs.python.org/msg193909</a>.) </blockquote> </blockquote> I do not go to such length as to run pep8 tool voluntarily to check all the errors. IDE of my choice (NINJA-IDE - check it out!) highlights a subset of PEP8 errors in the editor, so it makes it super easy to spot these errors and fix them very quickly. But I agree and you have point with the converting the code to idiomatic python code. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">In PlatformService.start, use `with` for open files. Also flush() is unnecessary before closing a file. </blockquote> </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> Fixed. Also fixed the same issues in the stop method. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite"> In SystemdService.service_instance, len(string) == 0 translates to: not string </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">In SystemdService.parse_variables, map(lambda x: y(x), z) translates to: [y(x) for x in z] (plus you can skip the [ ] since you don't need a list) </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">In SystemdService.stop/start if 'context' in api.env and api.env.context in X: translates to: if getattr(api.env, 'context', None) in X: </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">SystemdService.is_installed, the flag is not needed, just return directly. </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" 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 cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">0205: OK 0206: OK 0207: In system_units: again, a generator expression is better than map(lambda, ...) </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">In FedoraService.__init__, len(string.split(c)) == 1 translates to: c not in string </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">In FedoraDirectoryService, the comment applies just to restart(), could you move it there? </blockquote> </blockquote> Done. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">In FedoraDirectoryService.restart, there's `len(instance_name) > 0` again </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">In FedoraCAService, can __wait_until_running have just one leading underscore? I don't think we need to hide it. </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">In FedoraCAService.__wait_until_running, there are some hardcoded paths. Can we pull them from the paths module? It'll be easier to reuse if we ever find out the class applies to more than Fedora. (You could do it in some later patch of course.) </blockquote> </blockquote> Addresed in a later patch. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">0208: Again some hardcoded paths, would be nice to pull them from the paths object </blockquote> </blockquote> Addresed in a later patch. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">I'm surprised you left the parentheses around `if` expressions, since you're so meticulous about whitespace and line length... </blockquote> </blockquote> Fixed (in both cases). <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">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. There's no newline at the end of the file. 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 cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">In configure_sshd_config, why did you remove `authorized_keys_changes = candidate`? </blockquote> </blockquote> Thanks for spotting that, fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">base.tasks: These need docstrings. Will those included in the documentation that you want to provide later? base.{tasks,fedora}.restore_pre_ipa_client_configuration: line too long. </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">You fix this in a later patch, why not here? Each of the functions configure_sssd_for_user_info_and_auth, configure_ldap_for_user_info, configure_auto_homedir_creation now execute authconfig. </blockquote> </blockquote> Yes, that was a tradeoff for the portability. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">In the base.authconfig.Authconfig docstring, the example is wrong. In that docstring and in ipaplatform.fedora.tasks, instead of auth_config.enable("sssd").\ enable("sssdauth") please use auth_config.enable("sssd") auth_config.enable("sssdauth") In addition to PEP8 compliance (this is Python, not JQuery), it's more diff-friendly. Alternately, be pythonic and add support for: auth_config.enable("sssd", "sssdauth") auth_config.add_parameters(nisdomain=nisdomain): </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">It seems that for every AuthConfig.execute(), you need to do auth_config.add_option("update") Why not roll that into execute(), with a (default) argument? </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">AuthConfig.__build_args should lose the leading underscores. Nothing to hide here; according to the docstring it's even part of the public interface. I'd also move it (and execute()) to the base implementation, since it's quite unlikely that authconfig would use different arguments in other distros that support it (and they can override anyway). </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">There's another hardcoded path, "/usr/sbin/authconfig". </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">0210: OK It might be good to point to this patch in some docs, there are some ideas for porting to System V distros. 0211: I believe ipaplatform.paths.path_namespace doesn't exist yet. Should these changes be in a later patch? I see more hardcodced paths. </blockquote> </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> Will be dealth with later. <meta name="Description" content="Copy-Paste Buffer"> <meta name="Generator" content="Zim"> <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">For logging, use multiple arguments instead of %, and str() is unnecessary, e.g.: root_logger.info("Failed to add CA to the systemwide " "CA trust database: %s", e) </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">In backup_and_replace_hostname, please use the logger for the message, so it ends up a logfile. (Do it in addition to the print if the logger might be unconfigured.) </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">In backup_and_replace_hostname, the `readlines` in `for line in f.readlines():` is unnecessary. </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">0212: OK 0213: OK 0214: OK 0215: Looks OK (pylint should be enough to check this, haven't run it yet) 0216: I see a lot of these lines: from ipaplatform.paths import path_namespace as paths I don't think it's healthy to have an object that you *always* import as another name (actually in a single codebase we shouldn't need renaming objects on import at all, but that would be a very minor nitpick). We had some discussions about this, maybe I gave you the idea; sorry for any miscommunication. (I was mostly scared at wanting to redefine a module's __getattr__, which is some seriously arcane magic -- you should only try it at home.) You can do better by just placing the class in a convenient place: ipaplatform/<platform>/paths_module.py: class PathsNamespace(object): ... ipaplatform/paths_module.py → ipaplatform/<platform>/paths_module.py ipaplatform/__init__.py: from ipaplatform.paths_module import PathsNamespace paths = PathsNamespace() any other module: from ipaplatform import paths mkdir(paths.IMPORTANT_DIR) </blockquote> </blockquote> 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 cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">ipalib/__init__.py: I think your regex got too hungry here... </blockquote> </blockquote> Thanks for spotting that. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite">I'd add a _TEMPLATE suffix to names of "paths" containing %s, to make it clear what they are. </blockquote> </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite">Continuing with 0216: ipaserver/install/httpinstance.py: NSS_CONF is already in paths; SSL_CONF should be added there as well. selinux_warning should use paths.SETSEBOOL. </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite">Regarding the integration tests, we'll probably want to have each host have a "paths" attribute and use that in the commands, since each can potentially be on a different platform. That can be done later, though. ipatests/test_xmlrpc/test_automount_plugin.py: this change is unnecessary 0217: OK 0218: OK 0219: As I said above, I'd prefer this renamed 0220: Looks OK 0221: Looks OK 0222: Similarly to paths, we should do `from ipaplatform import tasks`; the module can have an inconvenient name. </blockquote> Fixed. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> <blockquote type="cite"> When moving code around you remove the Authors: lines from the top of the files, and replace them with yourself. I don't think that's a fair thing to do. </blockquote> </blockquote> Not trying to steal the credit here :) I fixed the Authors and copyright years. <blockquote cite="mid:5391A03D.1030408@redhat.com" type="cite"> Functionally, I didn't find any regressions. </blockquote> Updated patchset attached. Please note the new patch 223. I also added new symlinks to platform modules to .gitignore. <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>