ipaplatform: Add Debian platform module. #373

tjaalton · 2017-01-05T14:33:05Z

Hi, this just adds the Debian platform module. There are still other changes needed before vanilla master can be used on Debian or it's derivatives, but they need bigger changes while this is mostly standalone.

tiran · 2017-01-05T14:46:17Z

ipaplatform/debian/services.py

+
+# Mappings from service names as FreeIPA code references to these services
+# to their actual systemd service names
+debian_system_units = redhat_services.redhat_system_units


Please create a copy of the dict with redhat_services.redhat_system_units.copy()

tiran · 2017-01-05T14:48:08Z

ipaplatform/debian/services.py

+                ports = base_services.wellknownports[self.service_name]
+        if ports:
+            ipautil.wait_for_open_ports('localhost', ports, self.api.env.startup_timeout)
+    def stop(self, instance_name='', capture_output=True):


Missing an empty line before def stop

tiran · 2017-01-05T14:48:34Z

ipaplatform/debian/services.py

+    def tune_nofile_platform(self):
+        return True
+
+# For services which have no Debian counterpart


pep8 wants two empty lines between classes.

tiran · 2017-01-05T14:48:55Z

ipaplatform/debian/tasks.py

+
+BaseTask = BaseTaskNamespace()
+
+class DebianTaskNamespace(RedHatTaskNamespace):


two empty lines

stlaz

Thank you for you contribution! Please find some comments inline, some more may come.

stlaz · 2017-01-05T15:09:09Z

ipaplatform/debian/tasks.py

+"""
+
+from ipaplatform.paths import paths
+from ipaplatform.base.tasks import *


Please don't use wildcard imports, QuantifiedCode is having a bad day if you do.

stlaz · 2017-01-05T15:12:00Z

ipaplatform/debian/tasks.py

+        return True
+
+    def parse_ipa_version(self, version):
+        return BaseTask.parse_ipa_version(version)


You could perhaps make parse_ipa_version a static method so that you don't have to create a BaseTaskNamespace instance just for this one purpose.

so something like:

from pkg_resources import parse_version
...
def parse_ipa_version(self, version):
return parse_version(version)

?

What I meant was to add @staticmethod before BaseTaskNamespace parse_version definition so that it goes something like this:

class BaseTaskNamespace(object): ... @staticmethod def parse_ipa_version(version): """ :param version: textual version :return: object implementing proper __cmp__ method for version compare """ return parse_version(version)

Note the removal of self which is not needed in the method anyway so it being static is actually the preferred way. Once you do this, you can go:

class DebianTaskNamespace(RedHatTaskNamespace): ... def parse_ipa_version(self, version): return BaseTaskNamespace.parse_ipa_version(version)

Note that your parse_ipa_version can itself be static as it uses none of the instance members stored in self :)

tiran · 2017-01-11T10:40:58Z

pylint is failing:

Pylint is running, please wait ...
************* Module ipaplatform.debian.tasks
ipaplatform/debian/tasks.py:10: [E0611(no-name-in-module), ] No name 'BaseTaskNameSpace' in module 'ipaplatform.base.tasks')
ipaplatform/debian/tasks.py:49: [E0602(undefined-variable), DebianTaskNamespace.parse_ipa_version] Undefined variable 'BaseTaskNamespace')
ipaplatform/debian/tasks.py:9: [W0611(unused-import), ] Unused paths imported from ipaplatform.paths)
ipaplatform/debian/tasks.py:10: [W0611(unused-import), ] Unused BaseTaskNameSpace imported from ipaplatform.base.tasks)
************* Module ipaplatform.debian.services
ipaplatform/debian/services.py:64: [R0102(simplifiable-if-statement), DebianSysvService.stop] The if statement can be replaced with 'var = bool(test)')
ipaplatform/debian/services.py:65: [W0612(unused-variable), DebianSysvService.stop] Unused variable 'update_service_list')
ipaplatform/debian/services.py:73: [R0102(simplifiable-if-statement), DebianSysvService.start] The if statement can be replaced with 'var = bool(test)')
ipaplatform/debian/services.py:74: [W0612(unused-variable), DebianSysvService.start] Unused variable 'update_service_list')
ipaplatform/debian/services.py:9: [W0611(unused-import), ] Unused import time)
ipaplatform/debian/services.py:11: [W0611(unused-import), ] Unused tasks imported from ipaplatform.tasks)
ipaplatform/debian/services.py:15: [W0611(unused-import), ] Unused root_logger imported from ipapython.ipa_log_manager)
make: *** [pylint] Error 14

stlaz · 2017-01-11T11:11:00Z

ipaplatform/debian/services.py

+        installed = True
+        try:
+            ipautil.run([paths.SBIN_SERVICE, self.service_name, "status"])
+        except ipautil.CalledProcessError, e:


Please use the (not so) new syntax except ipautil.CalledProcessError as e, this breaks Python 3.

stlaz · 2017-01-17T07:26:31Z

ipaplatform/debian/paths.py

+    SYSTEMD_SYSTEM_HTTPD_D_DIR = "/etc/systemd/system/apache2.service.d/"
+    SYSTEMD_SYSTEM_HTTPD_IPA_CONF = "/etc/systemd/system/apache2.service.d/ipa.conf"
+    DNSSEC_TRUSTED_KEY = "/etc/bind/trusted-key.key"
+    KRA_AGENT_PEM = "/etc/apache2/nssdb/kra-agent.pem"


NOTE to myself: the location of KRA_AGENT_PEM changes in #367 to a distro-common location so we'll be able to remove this once <-- is pulled.

stlaz · 2017-01-17T07:31:15Z

ipaplatform/debian/paths.py

+    KRB5CC_HTTPD = "/var/run/apache2/ipa/krbcache/krb5ccache"
+    IPA_CUSTODIA_SOCKET = "/run/apache2/ipa-custodia.sock"
+    IPA_CUSTODIA_AUDIT_LOG = '/var/log/ipa-custodia.audit.log'
+    IPA_GETKEYTAB = '/usr/sbin/ipa-getkeytab'


This path is the same as in BasePathNamespace so it may not be needed here.

v2: - use redhat_services.redhat_system_units.copy - don't use wildcard imports - add some empty lines to make pep8 happy v3: - make parse_ipa_version static v4: - make more methods static v5: - fix pylint issues - use syntax that doesn't break with python3 v6: - remove IPA_GETKEYTAB from paths, it's the same across distros

tiran · 2017-01-19T13:51:49Z

@stlaz the patch looks fine to me now. I can't comment on the path values, though. Do you like to see additional modifications?

stlaz · 2017-01-19T14:54:08Z

@tiran I would like to test this in a Vagrant box before pushing it

edit: we're not quite there yet, will give it a final read

stlaz · 2017-01-19T15:20:55Z

The patch seems fine, I could have some nitpicks but nothing really imporant. ACK.

MartinBasti · 2017-01-19T15:46:09Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/e04b75cb9e71fb2b9faa49aea7f2244b01fddbcb

tjaalton force-pushed the deb-platform branch from 593a3e7 to 7fc38de Compare January 5, 2017 14:41

tiran reviewed Jan 5, 2017

View reviewed changes

stlaz reviewed Jan 5, 2017

View reviewed changes

tjaalton force-pushed the deb-platform branch 3 times, most recently from 394f36a to 3bce583 Compare January 10, 2017 14:05

stlaz reviewed Jan 11, 2017

View reviewed changes

tjaalton force-pushed the deb-platform branch 2 times, most recently from 1d9022b to c7b64b7 Compare January 11, 2017 23:27

stlaz self-assigned this Jan 17, 2017

stlaz reviewed Jan 17, 2017

View reviewed changes

tjaalton force-pushed the deb-platform branch from c7b64b7 to 6b490e2 Compare January 17, 2017 13:47

tiran mentioned this pull request Jan 18, 2017

Add templating to ipaplatform path [RFC] #244

Closed

stlaz added the ack Pull Request approved, can be merged label Jan 19, 2017

MartinBasti added the pushed Pull Request has already been pushed label Jan 19, 2017

MartinBasti closed this Jan 19, 2017

ipaplatform: Add Debian platform module. #373

ipaplatform: Add Debian platform module. #373

tjaalton commented Jan 5, 2017 •

edited by MartinBasti

tiran Jan 5, 2017

tiran Jan 5, 2017

tiran Jan 5, 2017

tiran Jan 5, 2017

stlaz left a comment

stlaz Jan 5, 2017

stlaz Jan 5, 2017

tjaalton Jan 5, 2017

stlaz Jan 6, 2017 •

edited

tiran commented Jan 11, 2017

stlaz Jan 11, 2017

stlaz Jan 17, 2017

stlaz Jan 17, 2017

tiran commented Jan 19, 2017

stlaz commented Jan 19, 2017 •

edited

stlaz commented Jan 19, 2017

MartinBasti commented Jan 19, 2017


		BaseTask = BaseTaskNamespace()

		class DebianTaskNamespace(RedHatTaskNamespace):

ipaplatform: Add Debian platform module. #373

ipaplatform: Add Debian platform module. #373

Conversation

tjaalton commented Jan 5, 2017 • edited by MartinBasti

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz Jan 6, 2017 • edited

Choose a reason for hiding this comment

tiran commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran commented Jan 19, 2017

stlaz commented Jan 19, 2017 • edited

stlaz commented Jan 19, 2017

MartinBasti commented Jan 19, 2017

tjaalton commented Jan 5, 2017 •

edited by MartinBasti

stlaz Jan 6, 2017 •

edited

stlaz commented Jan 19, 2017 •

edited