[Freeipa-devel] [PATCHES 0318 - 0320, 0323] installer: allow to modify dse.ldif during installation

[Jan Cholasta]

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:
>>>>>>>>>>> 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'.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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 "error",
>>>>>>>> so that it is immediately obvious to the user that his DSE mods
>>>>>>>> contain an error and cannot be parsed.
>>>>>>>>
>>>>>>>> Also you have a typo in the commit message of patch 320
>>>>>>>> (s/chages/changes/).
>>>>>>>>
>>>>>>> Updated patches attached.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> Rebased patches for master branch.
>>>>> ACK
>>>>>
>>>> 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 accept 
files (--external-cert-file, --ca-cert-file, --kasp-db-file, etc.)

> Speaking about the option, I saw some unescaped
> "-"'s in the man page updates:
>
> +.TP
> +\fB\-\-dirsrv-config-mods\fR
> +The path to LDIF file that will be used to modify configuration of dse.ldif
> during installation of the directory server instance
>
> Martin
>


-- 
Jan Cholasta