On 08/26/2016 04:19 AM, Fraser Tweedale wrote:
The attached patches add better handling of cert-request failure due
to target CA being disabled (#6260).  To do this, rather than go and
do extra work in Dogtag that we would depend on, instead I bite the
bullet and refactor ra.request_certificate to use the Dogtag REST
API, which correctly responds with status 409 in this case.

Switching RA to Dogtag REST API is an old ticket (#3437) so these
patches address it in part, and show the way forward for the rest of

These patches don't technically depend on patch 0101 which adds the
ca-enable and ca-disable commands, but 0101 may help for testing :)


Hi Fraser,

PATCH 102:

LGTM, but please use the standard ":param " annotations in the docstring for `_ssldo` method. It will make out life easier if we decide to use Sphinx or similar tool to auto-generate documentation from sources.

You can also add ":raises:" section describing that RemoteRetrieveError is raised when use_session is True but the session cookie wasn't acquired. It is kind of obvious but it may trip the uninitiated.

PATCH 103:

Due to magical behavior of our public errors, the exception body should look like this:

--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -1413,10 +1413,7 @@ class HTTPRequestError(RemoteRetrieveError):

     errno = 4035
-    def __init__(self, status=None, **kw):
-        assert status is not None
-        super(HTTPRequestError, self).__init__(status=status, **kw)
+    format = _('Request failed with status %(status)s: %(reason)')

The format string will be then automatically be supplied with status and reason if you pass them to the constructor ass you already do. The errors will be also handled magically (such as status which is None etc.)

PATCH 104:

1.) please don't use bare except here:

+        try:
+            resp_obj = json.loads(http_body)
+        except:
+ raise errors.RemoteRetrieveError(reason=_("Response from CA was not valid JSON"))

use 'except Exception' at least.

PATCH 105:

+            if e.status == 409:  # pylint: disable=E1101
+                raise errors.CertificateOperationError(
+                    error=_("CA '%s' is disabled") % ca)
+            else:
+                raise e

please use named errors instead of error codes in pylint annotations:
# pylint: disable=no-member

Martin^3 Babinsky

Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to