On 12.10.2015 12:30, Martin Babinsky wrote:
On 10/08/2015 05:58 PM, Martin Basti wrote:
The attached patches fix following tickets:
     https://fedorahosted.org/freeipa/ticket/4949
     https://fedorahosted.org/freeipa/ticket/4048
     https://fedorahosted.org/freeipa/ticket/1930

With these patches, an administrator can specify LDIF file that contains
modifications to be applied to dse.ldif right after creation of DS
instance.


Hi,

Functionally the paches work as expected. However I have following nitpicks:

in patch 318:

1.) there is a typo in ModifyLDIF class docstring:

+    Operations keep the order in whihc were specified per DN.

in patch 320:

1.) you should use 'os.path.join' to construct FS paths:


-        dse_filename = '%s/%s' % (
+        dse_filename = os.path.join(
             paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE % self.serverid,
-            'dse.ldif',
+            'dse.ldif'
         )

2.) IIUC the 'config_ldif_file' knob in BaseServer holds the path to LDIF containing the mod operations to dse.ldif. However, the knob name sounds like the option accepts the path of dse.ldif itself. I propose to rename the knob to something more in-line with the supposed function, like 'dse_mods_file'.

Users might not know what dse means, I would suggest to name option as 'dirsrv_config_mods' then.

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to