Conf template #673

tjaalton · 2017-03-29T15:30:23Z

Move conf templates to a common location, make ipa.conf and named.conf portable.

tiran

I like it 👍

Please use ${id} syntax and use ipaplatform.NAME to detect Debian.

tiran · 2017-03-29T21:57:15Z

ipaserver/install/bindinstance.py

@@ -775,6 +775,8 @@ def __setup_sub_dict(self):
            NAMED_PID=paths.NAMED_PID,
            NAMED_VAR_DIR=paths.NAMED_VAR_DIR,
            BIND_LDAP_SO=paths.BIND_LDAP_SO,
+            DATA_DIR='' if ipautil.file_exists(paths.ETC_DEBIAN_VERSION) else 'data/',


You can use ipaplatform.NAME instead. It would also fix the pylint isues

import ipaplatform ... DATA_DIR='' if ipaplatform.NAME == 'debian' else 'data/'

tiran · 2017-03-29T21:57:48Z

install/share/ipa.conf.template

-Alias /ipa/ui/fonts/open-sans "/usr/share/fonts/open-sans"
-Alias /ipa/ui/fonts/fontawesome "/usr/share/fonts/fontawesome"
-<Directory "/usr/share/fonts">
+Alias /ipa/ui/fonts/open-sans "$FONTS_DIR/open-sans"


${FONTS_DIR}

tiran · 2017-03-29T21:58:07Z

install/share/bind.named.conf.template

-	type hint;
-	file "named.ca";
-};
+$DISABLE_DEBIANzone "." IN {


${DISABLE_DEBIAN}

tiran · 2017-03-29T21:59:31Z

install/share/bind.named.conf.template

-	dump-file		"data/cache_dump.db";
-	statistics-file		"data/named_stats.txt";
-	memstatistics-file	"data/named_mem_stats.txt";
+	dump-file		"$DATA_DIRcache_dump.db";


It's really hard to read and probably does not work as intended. https://docs.python.org/3/library/string.html#template-strings supports ${id} syntax, so ${DATA_DIR}cache_dump.db

tiran · 2017-03-29T22:00:12Z

install/share/ipa.conf.template

@@ -146,7 +146,7 @@ Alias /ipa/session/cookie "/usr/share/ipa/gssapi.login"
 # Custodia stuff is redirected to the custodia daemon
 # after authentication
 <Location "/ipa/keys/">
-    ProxyPass "unix:/run/httpd/ipa-custodia.sock|http://localhost/keys/"
+    ProxyPass "unix:$IPA_CUSTODIA_SOCKET|http://localhost/keys/"


${IPA_CUSTODIA_SOCKET}

tjaalton · 2017-03-30T08:56:24Z

https://pagure.io/freeipa/issue/6837

tiran

Thanks! LGTM

MartinBasti · 2017-03-30T15:25:23Z

install/share/ipa.conf.template

@@ -47,7 +47,7 @@ FileETag None

 # FIXME: WSGISocketPrefix is a server-scope directive.  The mod_wsgi package
 # should really be fixed by adding this its /etc/httpd/conf.d/wsgi.conf:
-WSGISocketPrefix /run/httpd/wsgi
+WSGISocketPrefix $WSGI_PREFIX_DIR


shouldn't be this ${WSGI_PREFIX_DIR} (on multiple places in this file)

Maybe, but I also left the original variables be like they were.

$WSGI_PREFIX_DIR is fine. The syntax ${WSGI_PREFIX_DIR} is only required when the variable is surrounded by alphanumeric chars.

MartinBasti · 2017-03-30T15:29:09Z

ipaserver/install/bindinstance.py

@@ -775,6 +776,8 @@ def __setup_sub_dict(self):
            NAMED_PID=paths.NAMED_PID,
            NAMED_VAR_DIR=paths.NAMED_VAR_DIR,
            BIND_LDAP_SO=paths.BIND_LDAP_SO,
+            DATA_DIR='' if ipaplatform.NAME == 'debian' else 'data/',
+            DISABLE_DEBIAN='//' if ipaplatform.NAME == 'debian' else '',
            )


I don't like platform dependent code outside of ipaplatform module, I prefer to move this logic into tasks, or even better to have template files per platform if needed.

I'm fine with having a separate conffile for bind, just need to first create one that fits Debian better

I like Martin's suggestion to move the variables to tasks.

I'd rename DISABLE_DEBIAN to DISABLE_ROOTS or something more generic. Maintaining two files could be prone to error especially since the diffs at the moment seem to be relatively small.

pvoborni · 2017-10-04T08:15:32Z

We are lowering the number of opened PRs. @tjaalton could you rebase the PR? So that it applies and CI is run.

tjaalton · 2017-12-20T09:46:31Z

I've dropped the named.conf changes for Debian, that needs more work but the others can be merged now

tiran

Please address PEP 8 violations and rebase your branch. You can use make fastcheck to perform pylint and pep8 checks locally.

PEP-8 errors:
./ipaserver/install/dogtaginstance.py:235:80: E501 line too long (81 > 79 characters)
./ipaserver/install/httpinstance.py:224:80: E501 line too long (86 > 79 characters)
./ipaserver/install/server/upgrade.py:1693:80: E501 line too long (80 > 79 characters)
./ipaserver/install/server/upgrade.py:1695:80: E501 line too long (88 > 79 characters)
./ipaserver/install/server/upgrade.py:1700:80: E501 line too long (85 > 79 characters)

tiran · 2018-02-08T10:34:32Z

pycodestyle complains about a couple of PEP 8 violations. IPA only runs pycodestyle on git diff. If you change a line, then you have to fix PEP 8 violations in the surrounding lines, too.

PEP-8 errors:
./ipaserver/install/dogtaginstance.py:235:80: E501 line too long (81 > 79 characters)
./ipaserver/install/httpinstance.py:224:80: E501 line too long (86 > 79 characters)
./ipaserver/install/server/upgrade.py:1693:80: E501 line too long (80 > 79 characters)
./ipaserver/install/server/upgrade.py:1695:80: E501 line too long (88 > 79 characters)
./ipaserver/install/server/upgrade.py:1700:80: E501 line too long (85 > 79 characters)

tiran · 2018-02-08T13:57:20Z

@stlaz @rcritten could you please review the patch, too? It looks fine to me.

stlaz · 2018-02-08T15:07:11Z

LGTM, need to test it at least on Fedora.

stlaz · 2018-02-08T16:11:34Z

Everything seems to be working, will wait for @rcritten to do the final ACK.

stlaz · 2018-02-09T08:12:21Z

May this be ACKed. Thank you, @tjaalton :)

tiran · 2018-02-09T08:14:54Z

master:

1adb3ed Move config templates from install/conf to install/share
e6c707b ipaplatform, ipa.conf: Use paths variables in ipa.conf.template

tiran requested changes Mar 29, 2017

View reviewed changes

tjaalton force-pushed the conf-template branch 2 times, most recently from 8ce3ac3 to b484cc5 Compare March 30, 2017 07:38

tiran approved these changes Mar 30, 2017

View reviewed changes

MartinBasti suggested changes Mar 30, 2017

View reviewed changes

MartinBasti self-assigned this Mar 30, 2017

MartinBasti removed their assignment May 30, 2017

tjaalton force-pushed the conf-template branch 2 times, most recently from f52b17d to e210048 Compare December 20, 2017 09:42

felipevolpone added the re-run Trigger a new run of PR-CI label Dec 20, 2017

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Dec 20, 2017

tiran requested changes Jan 5, 2018

View reviewed changes

tjaalton force-pushed the conf-template branch from e210048 to d9687c9 Compare February 8, 2018 08:38

tjaalton added 2 commits February 8, 2018 14:00

Move config templates from install/conf to install/share

0237a0c

ipaplatform, ipa.conf: Use paths variables in ipa.conf.template

e8af75d

tjaalton force-pushed the conf-template branch from d9687c9 to e8af75d Compare February 8, 2018 12:01

tiran approved these changes Feb 8, 2018

View reviewed changes

tiran added the re-run Trigger a new run of PR-CI label Feb 8, 2018

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Feb 8, 2018

tiran added the re-run Trigger a new run of PR-CI label Feb 8, 2018

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Feb 8, 2018

stlaz added the ack Pull Request approved, can be merged label Feb 9, 2018

tiran added the pushed Pull Request has already been pushed label Feb 9, 2018

tiran closed this Feb 9, 2018

tjaalton mentioned this pull request May 21, 2018

Changes for Debian #1949

Closed

Conf template #673

Conf template #673

tjaalton commented Mar 29, 2017 •

edited by tiran

tiran left a comment

tiran Mar 29, 2017

tiran Mar 29, 2017

tiran Mar 29, 2017

tiran Mar 29, 2017

tiran Mar 29, 2017

tjaalton commented Mar 30, 2017

tiran left a comment

MartinBasti Mar 30, 2017

tjaalton Mar 30, 2017

tiran Apr 3, 2017

MartinBasti Mar 30, 2017

tjaalton Mar 30, 2017

rcritten Sep 14, 2017

pvoborni commented Oct 4, 2017

tjaalton commented Dec 20, 2017

tiran left a comment

tiran commented Feb 8, 2018

tiran commented Feb 8, 2018

stlaz commented Feb 8, 2018

stlaz commented Feb 8, 2018

stlaz commented Feb 9, 2018

tiran commented Feb 9, 2018

Conf template #673

Conf template #673

Conversation

tjaalton commented Mar 29, 2017 • edited by tiran

tiran 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjaalton commented Mar 30, 2017

tiran 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvoborni commented Oct 4, 2017

tjaalton commented Dec 20, 2017

tiran left a comment

Choose a reason for hiding this comment

tiran commented Feb 8, 2018

tiran commented Feb 8, 2018

stlaz commented Feb 8, 2018

stlaz commented Feb 8, 2018

stlaz commented Feb 9, 2018

tiran commented Feb 9, 2018

tjaalton commented Mar 29, 2017 •

edited by tiran