From 53c549664795f54dbc337e45aeba84fbff843109 Mon Sep 17 00:00:00 2001
From: Armando Neto <abiagion@redhat.com>
Date: Tue, 3 Jul 2018 11:00:57 -0300
Subject: [PATCH] ipa-client-install: Update how comments are added by
 ipachangeconf

Due to how 'openldap-client' parses its configuration files this patch
changes how comments are added, moving them to the line above instead
of appending to the same line.

IPA doesn't want to break existing configuration, if a value already
exists it adds a comment to the modified setting and a note about that
on the line above.

New settings will be added without any note.

Issue: https://pagure.io/freeipa/issue/5202

Signed-off-by: Armando Neto <abiagion@redhat.com>
Reviewed-By: Rob Crittenden <rcritten@redhat.com>
---
 ipaclient/install/client.py              |  7 ++--
 ipaclient/install/ipachangeconf.py       |  8 +++-
 ipatests/test_install/test_changeconf.py | 50 ++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 4 deletions(-)
 create mode 100644 ipatests/test_install/test_changeconf.py

diff --git a/ipaclient/install/client.py b/ipaclient/install/client.py
index 4b9c9d29f..b96feb47c 100644
--- a/ipaclient/install/client.py
+++ b/ipaclient/install/client.py
@@ -560,13 +560,14 @@ def configure_openldap_conf(fstore, cli_basedn, cli_server):
         {
             'name': 'comment',
             'type': 'comment',
-            'value': '   In case any of them were set, a comment with '
-                     'trailing note'
+            'value': '   In case any of them were set, a comment has been '
+                     'inserted and'
         },
         {
             'name': 'comment',
             'type': 'comment',
-            'value': '   "# modified by IPA" note has been inserted.'
+            'value': '   "# CONF_NAME modified by IPA" added to the line '
+                     'above.'
         },
         {
             'name': 'comment',
diff --git a/ipaclient/install/ipachangeconf.py b/ipaclient/install/ipachangeconf.py
index 762661981..7e357cf6a 100644
--- a/ipaclient/install/ipachangeconf.py
+++ b/ipaclient/install/ipachangeconf.py
@@ -349,11 +349,17 @@ class IPAChangeConf(object):
                     opts.append(no)
                     continue
                 if no['action'] == 'addifnotset':
+                    opts.append({
+                        'name': 'comment',
+                        'type': 'comment',
+                        'value': self._dump_line(
+                            ' ', no['name'], ' modified by IPA'
+                        ),
+                    })
                     opts.append({'name': 'comment', 'type': 'comment',
                                 'value': self._dump_line(no['name'],
                                                          self.dassign,
                                                          no['value'],
-                                                         u' # modified by IPA'
                                                          )})
                     opts.append(o)
                     continue
diff --git a/ipatests/test_install/test_changeconf.py b/ipatests/test_install/test_changeconf.py
new file mode 100644
index 000000000..2dc2b7dc1
--- /dev/null
+++ b/ipatests/test_install/test_changeconf.py
@@ -0,0 +1,50 @@
+# Copyright (C) 2018  FreeIPA Contributors see COPYING for license
+
+from __future__ import absolute_import
+
+import pytest
+from ipaclient.install.ipachangeconf import IPAChangeConf
+
+
+@pytest.fixture(scope='function')
+def config_filename(tmpdir):
+    filename = tmpdir.mkdir('data').join('config_file.conf')
+    filename.write('SOME_CONF /some/user/defined/path\n')
+    return filename
+
+
+def test_addifnotset_action(config_filename):
+    """Test if addifnotset action adds a comment about the modified conf.
+
+    IPA doesn't want to break existing configuration, if a value already exists
+    it adds a comment to the modified setting and a note about that on the line
+    above.
+
+    New settings will be added without any note.
+    """
+    ipa_conf = IPAChangeConf('IPA Installer Test')
+    ipa_conf.setOptionAssignment(' ')
+
+    opts = [
+        {
+            'action': 'addifnotset',
+            'name': 'SOME_CONF',
+            'type': 'option',
+            'value': '/path/defined/by/ipa',
+        },
+        {
+            'action': 'addifnotset',
+            'name': 'NEW_CONF',
+            'type': 'option',
+            'value': '/path/to/somewhere',
+        },
+    ]
+
+    ipa_conf.changeConf(str(config_filename), opts)
+
+    assert config_filename.readlines() == [
+        '# SOME_CONF modified by IPA\n',
+        '#SOME_CONF /path/defined/by/ipa\n',
+        'SOME_CONF /some/user/defined/path\n',
+        'NEW_CONF /path/to/somewhere\n',
+    ]