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

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0029.4-suppress-errors-arising-from-deleting-non-existent-f.patch
Type: text/x-patch
Size: 3392 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150420/07c99632/attachment.bin>