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

[Martin Basti]

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

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0330-Rename-option-dirsrv-config-mods-to-dirsrv-config-fi.patch
Type: text/x-patch
Size: 6572 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151019/404d041a/attachment.bin>