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³

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to