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

[Martin Babinsky]

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


-- 
Martin^3 Babinsky