[Freeipa-devel] [PATCHES 202-222] Ipaplatform refactoring
Tomas Babej
tbabej at redhat.com
Tue Jun 10 08:13:16 UTC 2014
Thank you for the detailed review. Responses to all the issues found are
inline:
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
>>>
>>> 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".
Fixed.
>>
>> 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.
>> 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
>>
Removed.
>> 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.)
>>
I do not go to such length as to run pep8 tool voluntarily to check all
the errors.
IDE of my choice (NINJA-IDE - check it out!) highlights a subset of PEP8
errors in the
editor, so it makes it super easy to spot these errors and fix them very
quickly.
But I agree and you have point with the converting the code to idiomatic
python code.
>> In PlatformService.start, use `with` for open files. Also flush() is
>> unnecessary before closing a file.
Fixed. Also fixed the same issues in the stop method.
>>
>> In SystemdService.service_instance,
>> len(string) == 0
>> translates to:
>> not string
>>
Fixed.
>> 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)
>>
Fixed.
>> 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:
>>
Fixed.
>> SystemdService.is_installed, the flag is not needed, just return
>> directly.
>>
Fixed.
>> 0204:
>> When commenting what a function does, use a docstring.
>>
Will be documented in a later patch.
>> 0205: OK
>> 0206: OK
>>
>> 0207:
>> In system_units: again, a generator expression is better than
>> map(lambda, ...)
>>
Fixed.
>> In FedoraService.__init__,
>> len(string.split(c)) == 1
>> translates to:
>> c not in string
>>
Fixed.
>> In FedoraDirectoryService, the comment applies just to restart(), could
>> you move it there?
>>
Done.
>> In FedoraDirectoryService.restart, there's `len(instance_name) > 0`
>> again
>>
Fixed.
>> In FedoraCAService, can __wait_until_running have just one leading
>> underscore? I don't think we need to hide it.
>>
Fixed.
>> 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.)
>>
Addresed in a later patch.
>> 0208:
>> Again some hardcoded paths, would be nice to pull them from the paths
>> object
>>
Addresed in a later patch.
>> I'm surprised you left the parentheses around `if` expressions, since
>> you're so meticulous about whitespace and line length...
>>
Fixed (in both cases).
>> 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.
>>
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?
>> In configure_sshd_config, why did you remove `authorized_keys_changes =
>> candidate`?
>>
Thanks for spotting that, fixed.
>> 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.
Fixed.
>> 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.
>>
Yes, that was a tradeoff for the portability.
>> 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):
>>
Fixed.
>> 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?
>>
Fixed.
>> 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).
>>
Fixed.
>> There's another hardcoded path, "/usr/sbin/authconfig".
>>
Fixed.
>> 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.
>>
Will be dealth with later.
>> 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)
>>
>>
Fixed.
>> 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.)
>>
Fixed.
>> In backup_and_replace_hostname, the `readlines` in `for line in
>> f.readlines():` is unnecessary.
>>
Fixed.
>> 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)
>>
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
>> ipalib/__init__.py: I think your regex got too hungry here...
>>
Thanks for spotting that.
>> I'd add a _TEMPLATE suffix to names of "paths" containing %s, to make it
>> clear what they are.
>>
>
Fixed.
> 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.
>
Fixed.
> 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.
>
Fixed.
>
>>
>> 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.
>
Not trying to steal the credit here :) I fixed the Authors and copyright
years.
>
> Functionally, I didn't find any regressions.
>
Updated patchset attached. Please note the new patch 223.
I also added new symlinks to platform modules to .gitignore.
--
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/20140610/7d67dae2/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0202-2-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/20140610/7d67dae2/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0203-2-ipaplatform-Move-service-base-platfrom-related-funct.patch
Type: text/x-patch
Size: 33013 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/7d67dae2/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0204-2-ipaplatform-Move-default-implementations-of-tasks-fr.patch
Type: text/x-patch
Size: 3644 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/7d67dae2/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0205-2-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/20140610/7d67dae2/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0206-2-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/20140610/7d67dae2/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0207-2-ipaplatform-Moved-Fedora-16-service-implementations-.patch
Type: text/x-patch
Size: 20325 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/7d67dae2/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0208-2-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/20140610/7d67dae2/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0209-2-ipaplatform-Do-not-require-custom-Authconfig-impleme.patch
Type: text/x-patch
Size: 19909 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/7d67dae2/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0210-2-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/20140610/7d67dae2/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0211-2-ipaplatform-Move-Fedora-specific-implementations-of-.patch
Type: text/x-patch
Size: 11812 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/7d67dae2/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0212-2-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/20140610/7d67dae2/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0213-2-ipaplatform-Change-service-code-in-freeipa-to-use-ip.patch
Type: text/x-patch
Size: 45297 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/7d67dae2/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0214-2-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/20140610/7d67dae2/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0215-2-ipaplatform-Remove-redundant-imports-of-ipaservices.patch
Type: text/x-patch
Size: 16436 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/7d67dae2/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0216-2-ipaplatform-Move-all-filesystem-paths-to-ipaplatform.patch
Type: text/x-patch
Size: 158632 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/7d67dae2/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0217-2-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/20140610/7d67dae2/attachment-0015.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0218-2-ipaplatform-Change-makefiles-to-accomodate-for-new-p.patch
Type: text/x-patch
Size: 9068 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/7d67dae2/attachment-0016.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0219-2-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/20140610/7d67dae2/attachment-0017.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0220-2-ipaplatform-Link-to-platform-module-during-build-tim.patch
Type: text/x-patch
Size: 1972 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/7d67dae2/attachment-0018.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0221-2-ipaplatform-Pylint-fixes.patch
Type: text/x-patch
Size: 2957 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/7d67dae2/attachment-0019.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0222-2-ipaplatform-Contain-all-the-tasks-in-the-TaskNamespa.patch
Type: text/x-patch
Size: 21565 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/7d67dae2/attachment-0020.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0223-ipaplatform-Move-hardcoded-paths-from-Fedora-platfor.patch
Type: text/x-patch
Size: 16018 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140610/7d67dae2/attachment-0021.bin>
More information about the Freeipa-devel
mailing list