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

[Martin Basti]

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

-- 
Martin Basti