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

[Petr Viktorin]

On 06/10/2014 10:13 AM, Tomas Babej wrote:
> Thank you for the detailed review. Responses to all the issues found are
> inline:

I'm calling it a day now, but here are initial comments from reading the 
patches.

> On 06/06/2014 01:04 PM, Petr Viktorin wrote:
>> 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
>>>>
[...]
>>>
>>> 0202: OK

>>> 0203:
>>>
>>> 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.
>>>
>
> Added.

Nice! Just one more thing.
I don't think it's good practice to pass additional *args to 
superclasses, unless it's really some sequence of items.
In this case you should use always name the arguments to prevent 
surprises, so **kwargs is enough.


>>> 0204:
>>> When commenting what a function does, use a docstring.
>>>
> Will be documented in a later patch.

You mean an upcoming patchset, right?

>>> 0205: OK
>>> 0206: OK
>>> 0207:  OK

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

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

See Martin's mail. This patchset is already too long for a general 
solution here.
You could file a ticket for a better fix so it's not forgotten.

>>> base.tasks: These need docstrings. Will those included in the
>>> documentation that you want to provide later?

0210: OK
0211: OK
0212: OK
0213: OK
0214: OK

0215:

You could rename the commit now

0216:
[...]
> 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

OK


I looked over the paths again, and saw this:

     SLAPD_INSTANCE_CONFIG = "/etc/dirsrv/slapd-"
     ETC_DIRSRV_SLAPD_INSTANCE = "/etc/dirsrv/slapd-%s"

SLAPD_INSTANCE_CONFIG should be removed.

     ETC_PKI_TOMCAT_ALIAS = "/etc/pki/pki-tomcat/alias"
     PKI_TOMCAT_ALIAS_DIR = "/etc/pki/pki-tomcat/alias/"

We only need one of these.

>> ipatests/test_xmlrpc/test_automount_plugin.py: this change is unnecessary

I was talking about this one:
-    keyname = u'/home'
+    keyname = u'/home/'
It's not only unnecessary, it also breaks a test.

0217: OK
0218: OK
0219: Ok
0220: OK
0221: OK
0222: OK

-- 
Petr³