On 8.4.2016 17:13, Martin Babinsky wrote: > On 04/01/2016 05:37 PM, Martin Babinsky wrote: >> On 03/24/2016 04:14 PM, Martin Babinsky wrote: >>> On 03/22/2016 04:28 PM, Rob Crittenden wrote: >>>> Martin Babinsky wrote: >>>>> On 03/21/2016 12:25 PM, Jan Cholasta wrote: >>>>>> On 21.3.2016 10:17, Petr Spacek wrote: >>>>>>> On 18.3.2016 13:49, Rob Crittenden wrote: >>>>>>>> Martin Babinsky wrote: >>>>>>>>> These patches implement behavior agreed upon during discussion of >>>>>>>>> https://fedorahosted.org/freeipa/ticket/5677 >>>>>>>>> >>>>>>>>> However I'm not sure if we want to push them into 4-3 branch (the >>>>>>>>> ticket >>>>>>>>> is triaged into 4.3.2 milestone) since they modify the framework >>>>>>>>> behavior quite a bit. >>>>>>>>> >>>>>>>>> If there is no need to have it there (CC'ing Milan since he is the >>>>>>>>> reporter), I would retriage it into 4.4 milestone. >>>>>>>> >>>>>>>> >>>>>>>> + desc="while getting entries (search base: '{}'," >>>>>>>> + "filter: {})".format(base_dn, filter)) >>>>>>>> >>>>>>>> This is going to expose parts of the DIT in an error message to >>>>>>>> users. We have >>>>>>>> tried in the past to hide the implementation. I'd propose logging >>>>>>>> the >>>>>>>> error >>>>>>>> and making the exception less verbose. >>>>>> >>>>>> I agree with Rob here, we shouldn't expose internal stuff in error >>>>>> messages for users. >>>>>> >>>>>> In this particular case, even if we included internal stuff in the >>>>>> error >>>>>> message, it should be the error message returned by the server rather >>>>>> than this ad-hoc message. >>>>>> >>>>>>> >>>>>>> IMHO it actually helps to print the DN. At very least the user can >>>>>>> see >>>>>>> if the >>>>>>> error is happening always with the same DN or if it keeps changing. >>>>>>> >>>>>>> In other words, for user it is not that important to understand >>>>>>> meaning of the >>>>>>> DN but it might be important to see if it is the same or not. >>>>>> >>>>>> I can't imagine a situation where it would actually be useful for the >>>>>> user (as opposed to the admin, who has access to logs) to know the >>>>>> base >>>>>> DN of some arbitrary LDAP search operation. Could you give an example? >>>>>> >>>>> Right, attaching updated patches. >>>> >>>> I may have suggested debug logging the detailed error. I was wrong. This >>>> should log at the error level so it always appears in the logs. This may >>>> be a spurious error and having the user turn on debug logging to capture >>>> the reasons would be asking a lot. >>>> >>>> rob >>> That's right, the user would then have to enable debug mode and re-run >>> the command. I have changed the log level to error as you suggested. >>> >>> >>> >> Bump for review. >> > Moar bumps.
ACK, sorry for the delay! -- Petr^2 Spacek -- 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
