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

@@ -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',

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

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',
+    ]