[Freeipa-devel] [PATCH 0043] Stop uninstaller from failing if a service can't be started

[Stanislav Laznicka]

On 06/24/2016 04:04 PM, Martin Basti wrote:
> On 24.06.2016 15:50, Stanislav Laznicka wrote:
>> On 06/21/2016 04:39 PM, Martin Basti wrote:
>>>
>>>
>>> On 14.06.2016 17:26, Stanislav Laznicka wrote:
>>>> -            signerd_service.start()
>>>> +            try:
>>>> +                signerd_service.start()
>>>> +            except Exception as e:
>>>> +                root_logger.error("Unable to start '{svcname}': 
>>>> {err}"
>>>> + .format(svcname=signerd_service.service_name,
>>>> +                                          err=e))
>>>
>>> why is signerd so special?
>>>
>>> Martin^2
>>>
>> From ODSExporterInstance.uninstall():
>>
>>         signerd_service = services.knownservices.ods_signerd
>>
>> This means that signerd_service here is not an instance of the 
>> service.Service class or of its child class but is rather an instance 
>> of the RedHatService class, a child class of the 
>> services.SystemdService class. Thus it has to be treated with special 
>> care.
>>
>
> Well then I prefer to put this option to systemdservice, and only pass 
> it from service.restart() to systemdService.restart()
>
> You forgot to handle regular_named service, which is the same as 
> ods_signerd

The passing of the option is not as easy as it may seem. The 
service.restart() method calls self.service.restart(). However, 
self.service is not directly of SystemdService class but rather of one 
of RedHat*Service classes, some of which override the 
SystemdService.restart() method (and call it somewhere in the process of 
their overridden method).

Would you then suggest:
1) Adding the option to all the methods on the way from Service to 
SystemdService classes just for the sake of passing => exceptions still 
may occur and suppress_errors option therefore does not do what you'd 
expect it to do
2) Adding the option to all the methods and treat all the possible 
exceptions on the way with a try: .. except: .. blocks ===> veeery ugly 
copy-pasta.
3) Leaving it as is => some .restart() calls offer suppress_errors 
option, some don't.