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

[Martin Basti]

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:
>>>>>>>>>>>>> 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.)
>>
> 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
>
>>> 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
>>>
>>
>>
>
>