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

Mon Jun 16 12:53:10 UTC 2014

On 06/10/2014 05:07 PM, Petr Viktorin wrote:
> 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.

Done!

>
>
>>>> 0204:
>>>> When commenting what a function does, use a docstring.
>>>>
>> Will be documented in a later patch.
>
> You mean an upcoming patchset, right?

Yep, documentation patch is attached.

>
>>>> 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.
Fixed and I filed https://fedorahosted.org/freeipa/ticket/4377 .

>
>>>> 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.
I replaced both, it required some addtional refactoring though.

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

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

-- 
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0226-ipaplatform-Document-the-platform-tasks-API.patch
Type: text/x-patch
Size: 4284 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0202-3-ipaplatform-Create-separate-module-for-platform-file.patch
Type: text/x-patch
Size: 6513 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0203-3-ipaplatform-Move-service-base-platfrom-related-funct.patch
Type: text/x-patch
Size: 32999 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0204-3-ipaplatform-Move-default-implementations-of-tasks-fr.patch
Type: text/x-patch
Size: 3636 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0205-3-ipaplatform-Create-default-implementations-for-tasks.patch
Type: text/x-patch
Size: 870 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0206-3-ipaplatform-Add-base-fedora-platform-module.patch
Type: text/x-patch
Size: 5314 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0207-3-ipaplatform-Moved-Fedora-16-service-implementations-.patch
Type: text/x-patch
Size: 20311 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0208-3-ipaplatform-Move-restore_context-and-check_selinux_s.patch
Type: text/x-patch
Size: 5549 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0209-3-ipaplatform-Do-not-require-custom-Authconfig-impleme.patch
Type: text/x-patch
Size: 19704 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0210-3-ipaplatform-Remove-legacy-redhat-platform-module.patch
Type: text/x-patch
Size: 12402 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0211-3-ipaplatform-Move-Fedora-specific-implementations-of-.patch
Type: text/x-patch
Size: 11775 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0212-3-ipaplatform-Change-platform-dependant-code-in-freeip.patch
Type: text/x-patch
Size: 18012 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0213-3-ipaplatform-Change-service-code-in-freeipa-to-use-ip.patch
Type: text/x-patch
Size: 45332 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0214-3-ipaplatform-Change-paths-dependant-on-ipaservices-to.patch
Type: text/x-patch
Size: 3705 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0215-3-ipaplatform-Remove-redundant-imports-of-ipaservices.patch
Type: text/x-patch
Size: 16471 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0216-3-ipaplatform-Move-all-filesystem-paths-to-ipaplatform.patch
Type: text/x-patch
Size: 158799 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0015.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0217-3-ipaplatform-Remove-remnants-of-the-ipapython-platfor.patch
Type: text/x-patch
Size: 16172 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0016.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0218-3-ipaplatform-Change-makefiles-to-accomodate-for-new-p.patch
Type: text/x-patch
Size: 9090 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0017.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0219-3-ipaplatform-Let-fedora-path-module-use-PathNamespace.patch
Type: text/x-patch
Size: 882 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0018.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0220-3-ipaplatform-Link-to-platform-module-during-build-tim.patch
Type: text/x-patch
Size: 1994 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0019.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0221-3-ipaplatform-Pylint-fixes.patch
Type: text/x-patch
Size: 2957 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0020.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0222-3-ipaplatform-Contain-all-the-tasks-in-the-TaskNamespa.patch
Type: text/x-patch
Size: 20846 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0021.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0223-2-ipaplatform-Move-hardcoded-paths-from-Fedora-platfor.patch
Type: text/x-patch
Size: 15736 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140616/c27d234b/attachment-0022.bin>