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

[Martin Babinsky]

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?

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