On 11/25/2015 01:29 PM, Jan Cholasta wrote:
> On 25.11.2015 13:24, Tomas Babej wrote:
>> On 11/10/2015 02:22 PM, Tomas Babej wrote:
>>> Hi,
>>>
>>> This has been rarely used, and can be replaced by proper shell output
>>> redirection.
>>>
>>> https://fedorahosted.org/freeipa/ticket/5408
>>>
>>
>> Here's an updated version of the patch that gets rid of one missed
>> occurrence of log_file usage.
> 
> The ticket seems unrelated to the change.
> 
> Shouldn't the option be kept in the respective commands for backward
> compatibility? See how the debug option is handled in AdminTool.
> 

Yeah, the correct ticket is: https://fedorahosted.org/freeipa/ticket/5385

I'm curious, in what manner do you envision the backward compatibility?
The debug option is being replaced with verbose, but here we're removing
existing functionality since it does not work properly and is of little
use anyway.

So there are two possiblities I see:

1.) We remove the functionality and keep the option, just to be able to
say that this option is deprecated and bail out.

2.) We keep the functionality, and keep the option, just issue a warning
when it's being used.

>From my point of view: I did not do (1), but imho we can add it, albeit
it's a marginal usability improvement. As far as (2) goes, it does not
solve the underlying problem.

Tomas
From 3a1c4a54c0be5a8bb9802df3bd023d5afb82c9f1 Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Tue, 10 Nov 2015 14:20:45 +0100
Subject: [PATCH] admintool: Remove the option to override the log file

This has been rarely used, and can be replaced by proper shell output
redirection.

https://fedorahosted.org/freeipa/ticket/5385
---
 install/tools/man/ipa-kra-install.1    |  3 ---
 install/tools/man/ipa-server-upgrade.1 |  3 ---
 ipapython/admintool.py                 | 17 ++++++-----------
 ipapython/install/cli.py               |  7 +------
 4 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/install/tools/man/ipa-kra-install.1 b/install/tools/man/ipa-kra-install.1
index e3133eee188af0b613fca76b51d2f5b4f8d1ba7d..5bda4fe4b80947588ad7afde2c9f8fd81f320614 100644
--- a/install/tools/man/ipa-kra-install.1
+++ b/install/tools/man/ipa-kra-install.1
@@ -47,9 +47,6 @@ Enable debug output when more verbose output is needed
 .TP
 \fB\-q\fR, \fB\-\-quiet\fR
 Output only errors
-.TP
-\fB\-v\fR, \fB\-\-log-file\fR=\fFILE\fR
-Log to the given file
 .SH "EXIT STATUS"
 0 if the command was successful
 
diff --git a/install/tools/man/ipa-server-upgrade.1 b/install/tools/man/ipa-server-upgrade.1
index cbbdc590171bff0a88b67bcf1de961fd783ac35c..7f06e09fc4d181fa9a89772e7285d4a6567bc361 100644
--- a/install/tools/man/ipa-server-upgrade.1
+++ b/install/tools/man/ipa-server-upgrade.1
@@ -36,9 +36,6 @@ Print debugging information
 \fB\-q\fR, \fB\-\-quiet\fR
 Output only errors
 .TP
-\fB-\-log-file=FILE\fR
-Log to given file
-.TP
 
 .SH "EXIT STATUS"
 0 if the command was successful
diff --git a/ipapython/admintool.py b/ipapython/admintool.py
index e40f300b1318b7325eb2b39ec83970753d39c406..0c8a5d54676fac0704202cf183990115978cebed 100644
--- a/ipapython/admintool.py
+++ b/ipapython/admintool.py
@@ -113,8 +113,6 @@ class AdminTool(object):
                 action="store_true", help="alias for --verbose (deprecated)")
         group.add_option("-q", "--quiet", dest="quiet", default=False,
             action="store_true", help="output only errors")
-        group.add_option("--log-file", dest="log_file", default=None,
-            metavar="FILE", help="log to the given file")
         parser.add_option_group(group)
 
     @classmethod
@@ -208,9 +206,8 @@ class AdminTool(object):
         :param _to_file: Setting this to false will disable logging to file.
             For internal use.
 
-        If the --log-file option was given or if a filename is in
-        self.log_file_name, the tool will log to that file. In this case,
-        all messages are logged.
+        If self.log_file_name is set, the tool will log to that file.
+        In this case, all messages are logged.
 
         What is logged to the console depends on command-line options:
         the default is INFO; --quiet sets ERROR; --verbose sets DEBUG.
@@ -232,12 +229,8 @@ class AdminTool(object):
         self._setup_logging(log_file_mode=log_file_mode)
 
     def _setup_logging(self, log_file_mode='w', no_file=False):
-        if no_file:
-            log_file_name = None
-        elif self.options.log_file:
-            log_file_name = self.options.log_file
-        else:
-            log_file_name = self.log_file_name
+        log_file_name = None if no_file else self.log_file_name
+
         if self.options.verbose:
             console_format = '%(name)s: %(levelname)s: %(message)s'
             verbose = True
@@ -249,10 +242,12 @@ class AdminTool(object):
                 verbose = False
             else:
                 verbose = True
+
         ipa_log_manager.standard_logging_setup(
             log_file_name, console_format=console_format,
             filemode=log_file_mode, debug=debug, verbose=verbose)
         self.log = ipa_log_manager.log_mgr.get_logger(self)
+
         if log_file_name:
             self.log.debug('Logging to %s' % log_file_name)
         elif not no_file:
diff --git a/ipapython/install/cli.py b/ipapython/install/cli.py
index aed0bc9fe12e0c56987a4e2f78d73f476dcfc2c8..37ede148b0cbde2f4c4ef46bddf39d13cbda6ed9 100644
--- a/ipapython/install/cli.py
+++ b/ipapython/install/cli.py
@@ -265,12 +265,7 @@ class ConfigureTool(admintool.AdminTool):
                 index += 1
 
     def _setup_logging(self, log_file_mode='w', no_file=False):
-        if no_file:
-            log_file_name = None
-        elif self.options.log_file:
-            log_file_name = self.options.log_file
-        else:
-            log_file_name = self.log_file_name
+        log_file_name = None if no_file else self.log_file_name
         ipa_log_manager.standard_logging_setup(log_file_name,
                                                debug=self.options.verbose)
         self.log = ipa_log_manager.log_mgr.get_logger(self)
-- 
2.5.0

-- 
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