[Freeipa-devel] [PATCH] 0101 Add ca-disable and ca-enable commands
Jan Cholasta
jcholast at redhat.com
Tue Aug 30 08:09:56 UTC 2016
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
More information about the Freeipa-devel
mailing list