[Freeipa-devel] [PATCH] 0101 Add ca-disable and ca-enable commands

[Martin Babinsky]

On 08/30/2016 10:09 AM, Jan Cholasta wrote:
> 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
>

Looks like I forgot how to OOP while on PTO :) Honza is right, of 
course, see the example code in the attached diff (again not tested, 
just a quick example).

-- 
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: better_example.patch
Type: text/x-patch
Size: 2245 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160830/06d35747/attachment.bin>