Hi,

On 30.8.2016 09:56, Martin Babinsky wrote:
On 08/25/2016 10:25 AM, Fraser Tweedale wrote:
Hi team,

The attached patch fixes
https://fedorahosted.org/freeipa/ticket/6257.

The behaviour of cert-request when the CA is disabled is not very
nice (it reports a server error from Dogtag).  The Dogtag REST
interface gives much better errors so I plan to move to it in a
later change (which will also address
https://fedorahosted.org/freeipa/ticket/3473, in part).

Thanks,
Fraser




HI Fraser,

I have a couple of comments below:

1.)
@@ -25,6 +33,10 @@ EXAMPLES:
     ipa ca-add puppet --desc "Puppet" \\
         --subject "CN=Puppet CA,O=EXAMPLE.COM"

+  Disable a CA.
+
+    ipa ca-disable puppet
+
 """)

You missed an example of `ca-enable` command in the doc string.

2.)

Regarding implementation of ca_enable/disable, I think you can reduce
the amount of code duplication by employing a base class which will look
up the required sub-CA and call the RA backend method required by the
subclass. See the attached untested diff (passes lint) for details.

NACK, please don't use getattr() this way. Since you are subclassing here, you should use polymorphism:

    class CAQuery(LDAPQuery):
        def execute(...):
            ...
            self.perform_action(ca_api, ca_id)
            ...

        def perform_action(self, ca_api, ca_id):
            raise NotImplementedError()

    class ca_enable(CAQuery):
        def perform_action(self, ca_api, ca_id):
            ca_api.enable_ca(ca_id)

Honza

--
Jan Cholasta

--
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

Reply via email to