On 10/19/2015 12:47 PM, Martin Basti wrote:

On 16.10.2015 12:36, Jan Cholasta wrote:
On 16.10.2015 10:31, Martin Basti wrote:

On 16.10.2015 10:05, Martin Babinsky wrote:
On 10/16/2015 08:47 AM, Jan Cholasta wrote:
On 16.10.2015 08:42, Martin Kosek wrote:
On 10/16/2015 06:00 AM, Jan Cholasta wrote:
On 15.10.2015 19:47, Martin Basti wrote:

On 15.10.2015 18:47, Martin Basti wrote:

On 15.10.2015 18:36, Martin Babinsky wrote:
On 10/15/2015 04:50 PM, Martin Basti wrote:

On 14.10.2015 16:10, Martin Basti wrote:

On 14.10.2015 15:51, Martin Babinsky wrote:
On 10/13/2015 06:38 PM, Martin Basti wrote:

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:

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


Functionally the paches work as expected. However I have

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(
-            '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
to rename the knob to something more in-line with the
function, like 'dse_mods_file'.

Updated patches + CI test attached

Patches work as expected and CI tests are OK.

However it seems that warning level messages are not logged to
installer output but only to log file (*waves hand* magic).

Maybe it would be a good idea to raise the message level to
so that it is immediately obvious to the user that his DSE
contain an error and cannot be parsed.

Also you have a typo in the commit message of patch 320

Updated patches attached.

Rebased patches for master branch.

Pushed to master: 5233165ce7062bb7aa649bf95a029103c375207b
Pushed to ipa-4-2: 94412b81c1c09b56ee2900942a1b804f21c264c5

These tickets are not for ipa-4-2,
reverted  a17936a1aad139236f18f0e5ad0b066f7747ca60

Can we use a better option name? --dirsrv-config-mods sounds weird,
as you need
to specify a file, not mods...

+1. maybe --dirsrv-config-ldif?

--dirsrv-config-file is most consistent with other options which
files (--external-cert-file, --ca-cert-file, --kasp-db-file, etc.)

This, however, may be confusing to user since '--dirsrv-config-file'
may evoke a feeling that it consumes *whole new* dse.ldif while in
reality it is only a few custom mods to directory server configuration.
I agree, it expects only file containing modifications in LDIF format,
'config-file' suffix may be confusing

Sorry, but this does not make any sense. Why would anyone think they
are supposed to specify a complete dse.ldif? Is it written somewhere
that DS config file == dse.ldif? I don't think so. And, if you use
--help, you will see exactly what the option does right away.

What is actually confusing is inventing a "smart" name instead of
making it consistent with everything else.

Patch attached.


Speaking about the option, I saw some unescaped
"-"'s in the man page updates:

+The path to LDIF file that will be used to modify configuration of
during installation of the directory server instance


Martin^3 Babinsky

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

Reply via email to