[Freeipa-devel] [PATCH 0029] suppress errors arising from deleting non-existent files during client uninstall

[Martin Basti]

On 17/04/15 14:11, Martin Babinsky wrote:
> On 04/17/2015 12:41 PM, Martin Babinsky wrote:
>> On 04/17/2015 12:36 PM, Martin Basti wrote:
>>> On 17/04/15 12:33, Martin Babinsky wrote:
>>>> On 04/17/2015 12:04 PM, Martin Basti wrote:
>>>>> On 15/04/15 15:53, Martin Babinsky wrote:
>>>>>> On 04/14/2015 04:24 PM, Martin Basti wrote:
>>>>>>> On 14/04/15 16:12, Martin Basti wrote:
>>>>>>>> On 14/04/15 14:25, Martin Babinsky wrote:
>>>>>>>>> This patch addresses https://fedorahosted.org/freeipa/ticket/4966
>>>>>>>>>
>>>>>>>>> The noise during rollback/uninstall is caused mainly by
>>>>>>>>> unsuccessful
>>>>>>>>> attempts to remove files that do not exist anymore. These errors
>>>>>>>>> are
>>>>>>>>> now logged at debug level and do not pop-up to stdout/stderr.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>> Hello, thank you for the patch.
>>>>>>>>
>>>>>>>> 1)
>>>>>>>> The option add_warning is quite unclear to me. It does not show
>>>>>>>> warning but error. I suggest something like, show_hint,
>>>>>>>> show_user_action, or something show_additional_..., or
>>>>>>>> promt_manual_removal
>>>>>>>>
>>>>>>>> Martin^2
>>>>>>>>
>>>>>>>>
>>>>>>> Continue...
>>>>>>>
>>>>>>> 2)
>>>>>>>
>>>>>>>              if file_exists(preferences_fname):
>>>>>>>                  try:
>>>>>>>                      os.remove(preferences_fname)
>>>>>>>                  except OSError as e:
>>>>>>>                      log_file_removal_error(e, preferences_fname,
>>>>>>> True)
>>>>>>>
>>>>>>> In this case file not found error should never happen.
>>>>>>>
>>>>>>> Could you remove the 'if file_exists' part and handle just 
>>>>>>> exception?
>>>>>>>
>>>>>> I just reverted this bit to original form in order to not fix
>>>>>> something that isn't broken. Is that ok?
>>>>>>> 3)
>>>>>>> this is inconsistent with change above, choose one style please:
>>>>>>>
>>>>>>>              if os.path.exists(ca_file):
>>>>>>>                  try:
>>>>>>>                      os.unlink(ca_file)
>>>>>>>                  except OSError, e:
>>>>>>>                      root_logger.error(
>>>>>>>                          "Failed to remove '%s': %s", ca_file, e)
>>>>>>>
>>>>>>> -- 
>>>>>>> Martin Basti
>>>>>>>
>>>>>>
>>>>>> Attaching updated patch.
>>>>>>
>>>>> thanks,
>>>>>
>>>>> just one nitpick, can you move the new function into installutils, it
>>>>> can be used in different scripts not just in ipaclient.
>>>>>
>>>>
>>>> I'm not sure if it is a good idea as installutils is a part for
>>>> freeipa-server package.
>>>>
>>>> Placing it there would create an unnecessary dependency of
>>>> freeipa-client on freeipa-server because of a single function.
>>>>
>>> you are right, I do not why I thought that ipa-client-install uses
>>> installutils.
>>>
>>> ACK
>>>
>> self-NACK, I will try to rewrite the patch in a slightly less dumb way.
>>
>> Sorry for the confusion.
>>
>
> Attaching updated patch which does the same but using a wrapper around 
> os.remove().
>
> Jan suggested to keep the new function in 'ipa-client-install' and 
> move it around when we do installer re#$%@^ing.
>
> Is that ok?
>
It looks better, ACK.

-- 
Martin Basti