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

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky