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

[Jan Cholasta]

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.

>>
>>>> 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