[Freeipa-devel] [PATCHES 202-222] Ipaplatform refactoring

[Petr Viktorin]

On 06/05/2014 03:14 PM, Petr Viktorin wrote:
> On 06/04/2014 11:42 AM, Tomas Babej wrote:
>> Hi,
>>
>> the following set of patches implements the ticket:
>>
>> https://fedorahosted.org/freeipa/ticket/4052
>>
>> 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.
>>
>
> 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".
>
> 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.
>
> In SystemdService.restart, I think the comment is obsolete by now. But
> If we want to keep it we should add a link to
> https://bugs.freedesktop.org/show_bug.cgi?id=39824
>
> 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
> http://bugs.python.org/msg193909.)
>
> In PlatformService.start, use `with` for open files. Also flush() is
> unnecessary before closing a file.
>
> In SystemdService.service_instance,
>      len(string) == 0
> translates to:
>      not string
>
> 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)
>
> 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:
>
> SystemdService.is_installed, the flag is not needed, just return directly.
>
> 0204:
> When commenting what a function does, use a docstring.
>
> 0205: OK
> 0206: OK
>
> 0207:
> In system_units: again, a generator expression is better than
> map(lambda, ...)
>
> In FedoraService.__init__,
>      len(string.split(c)) == 1
> translates to:
>      c not in string
>
> In FedoraDirectoryService, the comment applies just to restart(), could
> you move it there?
>
> In FedoraDirectoryService.restart, there's `len(instance_name) > 0` again
>
> In FedoraCAService, can __wait_until_running have just one leading
> underscore? I don't think we need to hide it.
>
> 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.)
>
> 0208:
> Again some hardcoded paths, would be nice to pull them from the paths
> object
>
> I'm surprised you left the parentheses around `if` expressions, since
> you're so meticulous about whitespace and line length...
>
> 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.
>
> In configure_sshd_config, why did you remove `authorized_keys_changes =
> candidate`?
>
> 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.
> 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.
>
> 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):
>
> 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?
>
> 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).
>
> There's another hardcoded path, "/usr/sbin/authconfig".
>
> 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.
>
> 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)
>
>
> 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.)
>
> In backup_and_replace_hostname, the `readlines` in `for line in
> f.readlines():` is unnecessary.
>
> 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)
>
> ipalib/__init__.py: I think your regex got too hungry here...
>
> I'd add a _TEMPLATE suffix to names of "paths" containing %s, to make it
> clear what they are.
>

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.

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.


>
> 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.


Functionally, I didn't find any regressions.

-- 
Petr³