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

[Jan Cholasta]

Dne 28.4.2015 v 15:03 Martin Basti napsal(a):
> On 27/04/15 10:33, Martin Babinsky wrote:
>> On 04/24/2015 03:19 PM, Martin Basti wrote:
>>> On 20/04/15 10:58, Martin Babinsky wrote:
>>>> On 04/20/2015 10:32 AM, Martin Basti wrote:
>>>>> 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.
>>>>>
>>>> Jan NACKed your ACK.
>>>>
>>>> Attaching updated patch.
>>>>
>>> Sorry, NACK
>>>
>>> ************* Module ipa-client-install
>>> ipa-client/ipa-install/ipa-client-install:791:
>>> [E1121(too-many-function-args), uninstall] Too many positional arguments
>>> for function call)
>>> ipa-client/ipa-install/ipa-client-install:797:
>>> [E1121(too-many-function-args), uninstall] Too many positional arguments
>>> for function call)
>>>
>>> consult with Honza if option which show prompt user to delete file
>>> manually, should be there or not.
>>>
>>
>> Updated patch attached.
>>
> ACK
>

Pushed to:
master: 98376589de9b33d7007c8d43366d26f3e3307662
ipa-4-1: b04435a0f5c63e18ec36f2d3e0849a6dee384589

-- 
Jan Cholasta