[Freeipa-devel] [PATCH 0381] admintool: Remove the option to override the log file

[Petr Spacek]

On 25.11.2015 14:15, Jan Cholasta wrote:
> On 25.11.2015 13:49, Tomas Babej wrote:
>>
>>
>> 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.

This is such a minor thing that I would throw it away completely. It is not
worth the bytes :-)

User will get a message that the option does not exist which seems
functionally equivalent to the 'say that this option is deprecated and bail out'.

Petr^2 Spacek

>>
>> 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.
> 
> Add log_file_option=False argument to add_options(), if it's True, add the
> option to the parser. Set it to True in commands which currently have the option.
> 
> In _setup_logging(), if the option value is not None, add a handler for the
> file to the log manager (untested):
> 
>     user_file_handler = dict(
>         name='user_file',
>         filename=self.options.log_file,
>         filemode=log_file_mode,
>         permission=0600,
>         level='debug',
>         format=ipa_log_manager.LOGGING_FORMAT_STANDARD_FILE,
>     )
>     ipa_log_manager.log_mgr.create_log_handlers([user_file_handler], None, None)
> 
> This way it will log into both the standard location and the user-specified file.