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

[Martin Babinsky]

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.

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