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.

--
Martin Basti

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to