[Freeipa-devel] [PATCH] 0084 Make sure state of services is preserved after client uninstall

[Martin Kosek]

On 01/13/2014 05:36 PM, Rob Crittenden wrote:
> Ana Krivokapic wrote:
>> On 11/15/2013 05:03 PM, Tomas Babej wrote:
>>> On 11/07/2013 05:25 PM, Ana Krivokapic wrote:
>>>> Hello,
>>>>
>>>> This patch addresses tickethttps://fedorahosted.org/freeipa/ticket/3790.
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Freeipa-devel mailing list
>>>> Freeipa-devel at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>
>>> Looking good..
>>>
>>> I have two questions:
>>>
>>>
>>> 1.) Nitpick: I'd suggest we rename the save_state(service) and
>>> restore_state(service) to more descriptive
>>> save_service_state/restore_service_state?
>>>
>>
>> Well, if the argument you are passing to these function does not have a
>> name which suggests it is a service (which it should have anyway), you
>> can do: `save_state(service=x)`. So I don't think
>> `save_service_state(x)` is more readable.
>>
>>> 2.) There are other places in ipa-client-install where we save and
>>> restore the state of the service. Having abstracted that into a
>>> function, should we use this at other places as well?
>>>
>>>
>>
>> I looked at other instances in ipa-client-install where service states
>> are saved and restored. It seems that each of these cases includes some
>> custom logic which does not make it possible to use the two functions
>> I've added.
> 
> It looks fine to me as-is. I couldn't find any other services to include in
> this which is indeed a shame as this significantly simplifies things.
> 
> ACK
> 
> rob

Thanks for the review. Pushed to master.

Martin