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

[Jan Cholasta]

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

-- 
Jan Cholasta