Add feature that allows users to define the log file name. #284
Conversation
…s allows for easier use of log rotation tools such as logrotate
|
@cgravatt Can you please open an issue that describes the desired functionality? It's hard to evaluate your pull request without a spec for what it's supposed to do. |
|
@chesterl86 @cgravatt I have written up #292 as my understanding of what's desired here. Please take a look and let me know if that's what you mean. Assuming it is, I'll review the pull request. I'd like to get this into the 2.2.2 release if possible. |
| # If a relative path is specified, it is interpreted relative to this configuration | ||
| # file, not relative to the User Sync process working directory. | ||
| file_log_directory: logs | ||
|
|
||
| # (optional) file_log_name (default value "default") |
There was a problem hiding this comment.
The name of this parameter should be file_log_name_format.
The value of this parameter should be a Python format string that can refer to a single positional argument which is a datetime.datetime object produced by calling datetime.now() at the start of the run.
The default value of this string should be '{%Y-%m-%d}.log', which produces the current default value.
| @@ -121,6 +121,7 @@ def init_log(logging_config): | |||
| builder.set_bool_value('log_to_file', False) | |||
| builder.set_string_value('file_log_directory', 'logs') | |||
| builder.set_string_value('file_log_level', 'info') | |||
| builder.set_string_value('file_log_name', 'default') | |||
There was a problem hiding this comment.
same change as above: parameter is file_log_name_format and default value is '{%Y-%m-%d}.log'
| @@ -148,7 +149,13 @@ def init_log(logging_config): | |||
| if not os.path.exists(file_log_directory): | |||
| os.makedirs(file_log_directory) | |||
|
|
|||
| file_path = os.path.join(file_log_directory, datetime.date.today().isoformat() + ".log") | |||
| file_log_name = "{datestamp}.log".format(datestamp=datetime.date.today().isoformat()) | |||
There was a problem hiding this comment.
This line is not necessary. since you set the file_log_name_format in the builder, it will always be present.
| file_path = os.path.join(file_log_directory, datetime.date.today().isoformat() + ".log") | ||
| file_log_name = "{datestamp}.log".format(datestamp=datetime.date.today().isoformat()) | ||
|
|
||
| if options['file_log_name']: |
There was a problem hiding this comment.
This line is not necessary. since you set the file_log_name_format in the builder, it will always be present.
| file_log_name = "{datestamp}.log".format(datestamp=datetime.date.today().isoformat()) | ||
|
|
||
| if options['file_log_name']: | ||
| if options['file_log_name'] != 'default': |
There was a problem hiding this comment.
It's now a format, so we don't have to test for any given value.
|
|
||
| if options['file_log_name']: | ||
| if options['file_log_name'] != 'default': | ||
| file_log_name = options['file_log_name'] |
There was a problem hiding this comment.
This should unconditionally be
file_log_name = option['file_log_name_format'].format(datetime.now())| @@ -340,6 +347,7 @@ def main(): | |||
| try: | |||
| try: | |||
| args = process_args() | |||
|
|
|||
There was a problem hiding this comment.
PEP8: no blank lines in a try/except sequence. Also it's very bad practice in a PR to make changes to code that has nothing to do with the code you mean to fix, because it produces unnecessary merge errors.
|
This feature was added in 94e1090, so there's no need for this PR anymore. |
I want to use log rotate to handle log rotation for log files of the usersync tool, but log rotate is not very compatible with log files where the log filename is changed daily. I've added an option to allow you to specify a log name or maintain current adobe implementation.
Please accept or deny my proposed changes as deemed appropriate.