Re: [Freeipa-devel] [PATCH 75] log dogtag errors
On 10/18/2012 07:20 PM, John Dennis wrote: On 10/18/2012 05:06 AM, Petr Viktorin wrote: This looks much better. I found one more issue, though. +if detail is not None: +err_msg += ' (%s)' % detail Here I get TypeError: unsupported operand type(s) for +=: 'Gettext' and 'unicode'. Until our Gettext class supports addition (part of #3188), please use `err_msg = u'%s (%s)' % (err_msg, detail)` instead. Good catch, fixed. New patch attached. It works fine now, thanks. ACK -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 75] log dogtag errors
On 10/19/2012 09:45 AM, Petr Viktorin wrote: On 10/18/2012 07:20 PM, John Dennis wrote: On 10/18/2012 05:06 AM, Petr Viktorin wrote: This looks much better. I found one more issue, though. +if detail is not None: +err_msg += ' (%s)' % detail Here I get TypeError: unsupported operand type(s) for +=: 'Gettext' and 'unicode'. Until our Gettext class supports addition (part of #3188), please use `err_msg = u'%s (%s)' % (err_msg, detail)` instead. Good catch, fixed. New patch attached. It works fine now, thanks. ACK Pushed to master, ipa-3-0. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 75] log dogtag errors
On 10/17/2012 08:23 PM, John Dennis wrote:
On 10/12/2012 04:35 AM, Petr Viktorin wrote:
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.
Added a utility function as suggested. Revised patch attached.
This looks much better. I found one more issue, though.
diff --git a/ipaserver/plugins/dogtag.py b/ipaserver/plugins/dogtag.py
index baa41ad..66fef57 100644
--- a/ipaserver/plugins/dogtag.py
+++ b/ipaserver/plugins/dogtag.py
@@ -1233,6 +1233,27 @@ class ra(rabase.rabase):
self.password = ''
super(ra, self).__init__()
+def raise_certificate_operation_error(self, func_name, err_msg=None,
detail=None):
+
+:param func_name: function name where error occurred
+
+:param err_msg: diagnostic error message, if not supplied it will be
+ 'Unable to communicate with CMS'
+:param detail:extra information that will be appended to err_msg
+ inside a parenthesis
+
+Raise a CertificateOperationError and log the error message.
+
+
+if err_msg is None:
+err_msg = _('Unable to communicate with CMS')
+
+if detail is not None:
+err_msg += ' (%s)' % detail
Here I get TypeError: unsupported operand type(s) for +=: 'Gettext' and
'unicode'.
Until our Gettext class supports addition (part of #3188), please use
`err_msg = u'%s (%s)' % (err_msg, detail)` instead.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 75] log dogtag errors
On 10/18/2012 05:06 AM, Petr Viktorin wrote:
This looks much better. I found one more issue, though.
+if detail is not None:
+err_msg += ' (%s)' % detail
Here I get TypeError: unsupported operand type(s) for +=: 'Gettext' and
'unicode'.
Until our Gettext class supports addition (part of #3188), please use
`err_msg = u'%s (%s)' % (err_msg, detail)` instead.
Good catch, fixed. New patch attached.
--
John Dennis jden...@redhat.com
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
From 503e1e5939bb4fd9af8ecfab2e5a07accf03c3fa Mon Sep 17 00:00:00 2001
From: John Dennis jden...@redhat.com
Date: Wed, 17 Oct 2012 13:29:13 -0400
Subject: [PATCH 75-2] log dogtag errors
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
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. This is done in the utility
method raise_certificate_operation_error().
https://fedorahosted.org/freeipa/ticket/2622
---
ipaserver/plugins/dogtag.py | 68 -
1 file changed, 48 insertions(+), 20 deletions(-)
diff --git a/ipaserver/plugins/dogtag.py b/ipaserver/plugins/dogtag.py
index baa41ad..d52bb7e 100644
--- a/ipaserver/plugins/dogtag.py
+++ b/ipaserver/plugins/dogtag.py
@@ -1233,6 +1233,27 @@ class ra(rabase.rabase):
self.password = ''
super(ra, self).__init__()
+def raise_certificate_operation_error(self, func_name, err_msg=None, detail=None):
+
+:param func_name: function name where error occurred
+
+:param err_msg: diagnostic error message, if not supplied it will be
+ 'Unable to communicate with CMS'
+:param detail:extra information that will be appended to err_msg
+ inside a parenthesis
+
+Raise a CertificateOperationError and log the error message.
+
+
+if err_msg is None:
+err_msg = _('Unable to communicate with CMS')
+
+if detail is not None:
+err_msg = u'%s (%s)' % (err_msg, detail)
+
+self.error('%s.%s(): %s', self.fullname, func_name, err_msg)
+raise CertificateOperationError(error=err_msg)
+
def _host_has_service(self, host, service='CA'):
:param host: A host which might be a master for a service.
@@ -1376,14 +1397,15 @@ class ra(rabase.rabase):
# Parse and handle errors
if (http_status != 200):
-raise CertificateOperationError(error=_('Unable to communicate with CMS (%s)') % \
- http_reason_phrase)
+self.raise_certificate_operation_error('check_request_status',
+ detail=http_reason_phrase)
parse_result = self.get_parse_result_xml(http_body, parse_check_request_result_xml)
request_status = parse_result['request_status']
if request_status != CMS_STATUS_SUCCESS:
-raise CertificateOperationError(error='%s (%s)' % \
- (cms_request_status_to_string(request_status), parse_result.get('error_string')))
+self.raise_certificate_operation_error('check_request_status',
+ cms_request_status_to_string(request_status),
+ parse_result.get('error_string'))
# Return command result
cmd_result = {}
@@ -1461,14 +1483,15 @@ class ra(rabase.rabase):
# Parse and handle errors
if (http_status != 200):
-raise CertificateOperationError(error=_('Unable to communicate with CMS (%s)') % \
- http_reason_phrase)
+self.raise_certificate_operation_error('get_certificate',
+ detail=http_reason_phrase)
parse_result = self.get_parse_result_xml(http_body, parse_display_cert_xml)
request_status = parse_result['request_status']
if request_status != CMS_STATUS_SUCCESS:
-raise CertificateOperationError(error='%s (%s)' % \
- (cms_request_status_to_string(request_status), parse_result.get('error_string')))
+self.raise_certificate_operation_error('get_certificate',
+ cms_request_status_to_string(request_status),
+ parse_result.get('error_string'))
# Return command result
cmd_result = {}
@@ -1527,15 +1550,17 @@ class ra(rabase.rabase):
xml='true')
# Parse and handle errors
if (http_status != 200):
-
Re: [Freeipa-devel] [PATCH 75] log dogtag errors
On 10/12/2012 04:35 AM, Petr Viktorin wrote:
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.
Added a utility function as suggested. Revised patch attached.
--
John Dennis jden...@redhat.com
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
From 48f1560745cfa0281a387aacf0737841bb78b065 Mon Sep 17 00:00:00 2001
From: John Dennis jden...@redhat.com
Date: Wed, 17 Oct 2012 13:29:13 -0400
Subject: [PATCH 75-1] log dogtag errors
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
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. This is done in the utility
method raise_certificate_operation_error().
https://fedorahosted.org/freeipa/ticket/2622
---
ipaserver/plugins/dogtag.py | 68 -
1 file changed, 48 insertions(+), 20 deletions(-)
diff --git a/ipaserver/plugins/dogtag.py b/ipaserver/plugins/dogtag.py
index baa41ad..66fef57 100644
--- a/ipaserver/plugins/dogtag.py
+++ b/ipaserver/plugins/dogtag.py
@@ -1233,6 +1233,27 @@ class ra(rabase.rabase):
self.password = ''
super(ra, self).__init__()
+def raise_certificate_operation_error(self, func_name, err_msg=None, detail=None):
+
+:param func_name: function name where error occurred
+
+:param err_msg: diagnostic error message, if not supplied it will be
+ 'Unable to communicate with CMS'
+:param detail:extra information that will be appended to err_msg
+ inside a parenthesis
+
+Raise a CertificateOperationError and log the error message.
+
+
+if err_msg is None:
+err_msg = _('Unable to communicate with CMS')
+
+if detail is not None:
+err_msg += ' (%s)' % detail
+
+self.error('%s.%s(): %s', self.fullname, func_name, err_msg)
+raise CertificateOperationError(error=err_msg)
+
def _host_has_service(self, host, service='CA'):
:param host: A host which might be a master for a service.
@@ -1376,14 +1397,15 @@ class
Re: [Freeipa-devel] [PATCH 75] log dogtag errors
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³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 75] log dogtag errors
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? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 75] log dogtag errors
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? -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
