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Ā³

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

Reply via email to