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

[Martin Basti]

On 19.10.2015 14:12, Martin Babinsky wrote:
> 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:
>>>>>>>>>>>>>>>>> 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
>>>
>>> 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.
>> Martin^2
>>
> ACK
Pushed to master: f4c8c93e7092b341c3ed2e04553dd5afbcc44dc5
>>>
>>>>>
>>>>>>> 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
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>