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

[Martin Basti]

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