<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 14.10.2015 16:10, Martin Basti
wrote:<br>
</div>
<blockquote cite="mid:561E624C.2080601@redhat.com" type="cite">
<br>
<br>
On 14.10.2015 15:51, Martin Babinsky wrote:
<br>
<blockquote type="cite">On 10/13/2015 06:38 PM, Martin Basti
wrote:
<br>
<blockquote type="cite">
<br>
<br>
On 12.10.2015 12:30, Martin Babinsky wrote:
<br>
<blockquote type="cite">On 10/08/2015 05:58 PM, Martin Basti
wrote:
<br>
<blockquote type="cite">The attached patches fix following
tickets:
<br>
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/4949">https://fedorahosted.org/freeipa/ticket/4949</a>
<br>
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/4048">https://fedorahosted.org/freeipa/ticket/4048</a>
<br>
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/1930">https://fedorahosted.org/freeipa/ticket/1930</a>
<br>
<br>
With these patches, an administrator can specify LDIF file
that contains
<br>
modifications to be applied to dse.ldif right after
creation of DS
<br>
instance.
<br>
<br>
<br>
</blockquote>
Hi,
<br>
<br>
Functionally the paches work as expected. However I have
following
<br>
nitpicks:
<br>
<br>
in patch 318:
<br>
<br>
1.) there is a typo in ModifyLDIF class docstring:
<br>
<br>
+ Operations keep the order in whihc were specified per
DN.
<br>
<br>
in patch 320:
<br>
<br>
1.) you should use 'os.path.join' to construct FS paths:
<br>
<br>
<br>
- dse_filename = '%s/%s' % (
<br>
+ dse_filename = os.path.join(
<br>
paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE %
self.serverid,
<br>
- 'dse.ldif',
<br>
+ 'dse.ldif'
<br>
)
<br>
<br>
2.) IIUC the 'config_ldif_file' knob in BaseServer holds the
path to
<br>
LDIF containing the mod operations to dse.ldif. However, the
knob name
<br>
sounds like the option accepts the path of dse.ldif itself.
I propose
<br>
to rename the knob to something more in-line with the
supposed
<br>
function, like 'dse_mods_file'.
<br>
<br>
</blockquote>
<br>
Updated patches + CI test attached
<br>
</blockquote>
<br>
Patches work as expected and CI tests are OK.
<br>
<br>
However it seems that warning level messages are not logged to
installer output but only to log file (*waves hand* magic).
<br>
<br>
Maybe it would be a good idea to raise the message level to
"error", so that it is immediately obvious to the user that his
DSE mods contain an error and cannot be parsed.
<br>
<br>
Also you have a typo in the commit message of patch 320
(s/chages/changes/).
<br>
<br>
</blockquote>
Updated patches attached.
<br>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
Rebased patches for master branch.<br>
</body>
</html>