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³

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

Reply via email to