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? 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 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code