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

Thu Jun 5 13:14:43 UTC 2014

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:
     <platform>/paths_module.py:
         class PathsNamespace(object):
             ...
     ipaplatform/paths_module.py → <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.

(note to self: I'm at line 1500 of the patch)

---

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.

-- 
Petr³