On 09/06/2016 04:51 PM, Fraser Tweedale wrote:
On Tue, Aug 30, 2016 at 10:54:32AM +0200, Martin Babinsky wrote:
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
it.
These patches don't technically depend on patch 0101 which adds the
ca-enable and ca-disable commands, but 0101 may help for testing :)
Thanks,
Fraser
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
Thanks for your review, Martin. Updated patches attached; they
address all mentioned issues.
Cheers,
Fraser
Thanks, ACK.
Pushed to:
ipa-4-4: b8491490c2dbb3b2db3ce64cd154b499142bc250
master: 520ad7d865ff147d3ff8819d3e384d7cbd69bfb7
--
Martin^3 Babinsky
--
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
