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

[Jan Cholasta]

Dne 25.9.2014 v 16:15 Martin Basti napsal(a):
> On 22/09/14 19:30, Martin Basti wrote:
>> 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
>>
> Updated patch attached. I modified ldap_disable to use search function.
>

ACK.

-- 
Jan Cholasta