[Freeipa-devel] [PATCH 75] log dogtag errors

[Petr Viktorin]

On 10/11/2012 06:53 PM, John Dennis wrote:
> On 04/28/2012 09:50 AM, John Dennis wrote:
>> On 04/27/2012 04:45 AM, Petr Viktorin wrote:
>>> On 04/20/2012 08:07 PM, John Dennis wrote:
>>>> Ticket #2622
>>>>
>>>> If we get an error from dogtag we always did raise a
>>>> CertificateOperationError exception with a message describing the
>>>> problem. Unfortuanately that error message did not go into the log,
>>>> just sent back to the caller. The fix is to format the error message
>>>> and send the same message to both the log and use it to initialize the
>>>> CertificateOperationError exception.
>>>>
>>>
>>> The patch contains five hunks with almost exactly the same code,
>>> applying the same changes in each case.
>>> Wouldn't it make sense to move the _sslget call, parsing, and error
>>> handling to a common method?
>>>
>>
>> Yes it would and ordinarily I would have taken that approach. However on
>> IRC (or phone?) with Rob we decided not to perturb the code too much for
>> this particular issue because we intend to refactor the code later. This
>> was one of the last patches destined for 2.2 which is why we took the
>> more conservative approach.
>>
>
> I went back and looked at this. It's not practical to collapse
> everything into a common subroutine unless you paramaterize the heck out
> of a common subroutine. That's because all the patched locations have
> subtly different things going on, different parameters to sslget
> followed by different result parsing and handling. In retrospect I think
> it's clearer to keep things separate rather than one subroutine that
> needs a lot of parameters and/or convoluted logic to handle each unique
> case.

I don't agree that it's clearer, but I guess that's debatable.

> Part of the problem is the dogtag interface. Every command has the
> potential to behave differently making it difficult to work with. I
> wrote this code originally and got it reduced to as many common parts as
> I could. At some point soon we'll be switching to a new dogtag REST
> interface which hopefully will allow for greater commonality due to
> interface consistency.
>
> In summary: I still stand by the original patch.
>

However, I see no reason to not use a method such as:

def raise_certop_error(self, method_name, error=None, detail=None):
     """Log and raise a CertificateOperationError

     :param method_name: Name of the method in which the error occured
     :param error: Error string. If None, "Unable to communicate with
         CMS" is used.
     :param detail: Detailed or low-level information. Will be put in
         parentheses.
     """

to at least get rid of the repetition this patch is adding - almost the 
same format+log+raise sequence is used twice in each of the five hunks.

-- 
Petr³