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.

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.

John Dennis <jden...@redhat.com>

Looking to carve out IT costs?

Freeipa-devel mailing list

Reply via email to