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

[Martin Basti]

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.

-- 
Martin Basti