[Freeipa-devel] [PATCH 0118] Allow to disable service (in LDAP)

[Martin Basti]

On 19/09/14 14:47, Jan Cholasta wrote:
> Dne 19.9.2014 v 13:33 Martin Basti napsal(a):
>> On 02/09/14 11:59, Martin Basti wrote:
>>> On 02/09/14 09:10, Jan Cholasta wrote:
>>>> Hi,
>>>>
>>>> Dne 1.9.2014 v 16:57 Martin Basti napsal(a):
>>>>> This patch allows to disable service in LDAP to prevents service 
>>>>> to be
>>>>> started by "ipactl restart"
>>>>>
>>>>> Required by DNSSEC
>>>>>
>>>>> Patch attached
>>>>
>>>> I don't think the extra argument in ldap_enable is necessary. It
>>>> should enable the service no matter if the entry existed before or 
>>>> not.
>>>>
>>>> Similarly, in ldap_disable you should not raise an error when the
>>>> entry is not found, because that already makes the service disabled.
>>>>
>>>> Honza
>>>>
>>> Updated patch attached
>>>
>>>
>>>
>>> _______________________________________________
>>> Freeipa-devel mailing list
>>> Freeipa-devel at redhat.com
>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>> Please review, this should be in 4.1
>>
>> -- 
>> Martin Basti
>
> 1) ipaConfigString is case-insensitive, so you need to look for the 
> string "enabledService" in a case-insensitive way.
>
>
> 2) Don't say "failed to enable ..." when the service is already 
> enabled. It is not a failure. Same for disabled.
>
>
> 3) Missing ipaConfigString is nothing to warn about.
>
>
> 4) You should log success in both ldap_enable/ldap_disable.
>
>
> 5) The except below update_entry should say "except errors.EmptyModlist".
>
>
Thank you,

updated patch attached

-- 
Martin Basti

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0118.3-LDAP-disable-service.patch
Type: text/x-patch
Size: 3505 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140922/a5d10d34/attachment.bin>