borgmatic-collective/borgmatic
borgmatic-collective
/
borgmatic
14
58
Fork 42
Code Issues 59 Pull Requests 3 Actions Packages Releases 122 Activity

By default only brief logging output at verbosity 1 #283

Merged
witten merged 5 commits from palto42/borgmatic:list-files into master 2020-01-22 20:36:59 +00:00
Conversation 3 Commits 5 Files Changed 8 +9 -24
4 changed files with 9 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 75b5e7254e - Show all commits

9
borgmatic/borg/create.py
View File

@ -178,16 +178,13 @@ def create_archive(
+ (('--lock-wait', str(lock_wait)) if lock_wait else ())
+ (
('--list', '--filter', 'AME-')
if logger.isEnabledFor(logging.INFO)
and not json
and not progress
and (files or logger.isEnabledFor(logging.DEBUG))
if (files or logger.isEnabledFor(logging.DEBUG)) and not json and not progress
else ()
)
+ (('--info',) if logger.getEffectiveLevel() == logging.INFO and not json else ())
witten commented 2020-01-21 17:42:15 +00:00
Review
Copy Link

Potential simplification: You can probably remove the if logger.isEnabledFor(logging.INFO) check, given that Borg's --list --filter works at verbosity 0, and it's now opt-in. Example revision:

if (files or logger.isEnabledFor(logging.DEBUG))
and not json
and not progress
Potential simplification: You can probably remove the `if logger.isEnabledFor(logging.INFO)` check, given that Borg's `--list --filter` works at verbosity 0, and it's now opt-in. Example revision: ``` if (files or logger.isEnabledFor(logging.DEBUG)) and not json and not progress ```
palto42 commented 2020-01-22 18:16:40 +00:00
Review
Copy Link

Agree, will change as suggested.

Agree, will change as suggested.
+ (
('--stats',)
if not dry_run and (logger.isEnabledFor(logging.DEBUG) or stats) and not json
if (stats or logger.isEnabledFor(logging.DEBUG)) and not json and not dry_run
else ()
)
+ (('--debug', '--show-rc') if logger.isEnabledFor(logging.DEBUG) and not json else ())
@ -211,8 +208,6 @@ def create_archive(
if json:
output_log_level = None
elif stats and logger.getEffectiveLevel() == logging.WARNING:
output_log_level = logging.WARNING
else:
output_log_level = logging.INFO

18
borgmatic/borg/prune.py
View File

@ -58,23 +58,13 @@ def prune_archives(
+ (('--remote-path', remote_path) if remote_path else ())
+ (('--umask', str(umask)) if umask else ())
+ (('--lock-wait', str(lock_wait)) if lock_wait else ())
+ (
('--stats',)
if not dry_run and logger.getEffectiveLevel() == logging.DEBUG or stats
else ()
)
+ (('--stats',) if (stats or logger.isEnabledFor(logging.DEBUG)) and not dry_run else ())
+ (('--info',) if logger.getEffectiveLevel() == logging.INFO else ())
+ (('--list',) if logger.getEffectiveLevel() == logging.INFO and files else ())
+ (('--debug', '--list', '--show-rc') if logger.isEnabledFor(logging.DEBUG) else ())
+ (('--list',) if files or logger.isEnabledFor(logging.DEBUG) else ())
witten commented 2020-01-21 18:02:42 +00:00
Review
Copy Link

Would be good to parenthesize the logger.getEffectiveLevel() == logging.DEBUG or stats part for clarity.

Would be good to parenthesize the `logger.getEffectiveLevel() == logging.DEBUG or stats` part for clarity.
+ (('--debug', '--show-rc') if logger.isEnabledFor(logging.DEBUG) else ())
+ (('--dry-run',) if dry_run else ())
+ (tuple(extra_borg_options.split(' ')) if extra_borg_options else ())
+ (repository,)
witten commented 2020-01-21 18:06:10 +00:00
Review
Copy Link

Shouldn't this be an or rather than an and, so --files works at multiple log levels? And for prune, is the idea that the file listing should still be enabled at INFO, rather than demoting it to DEBUG like with create?

Shouldn't this be an `or` rather than an `and`, so `--files` works at multiple log levels? And for `prune`, is the idea that the file listing should still be enabled at `INFO`, rather than demoting it to `DEBUG` like with `create`?
palto42 commented 2020-01-22 18:56:10 +00:00
Outdated
Review
Copy Link

My intention was to only show the file listing at INFO and not at WARNING level. This is related to the old influence of the --stats option to change the logging level of the borg output. Since now --statshas no influence on the borg output level anymore, I could simplify the condition to if files.

To make the code a bit more consistent I moved the --listout of extra DEBUG line similar as in create.py:

    + (('--list',) if files or logger.isEnabledFor(logging.DEBUG) else ())
    + (('--debug', '--show-rc') if logger.isEnabledFor(logging.DEBUG) else ())
My intention was to only show the file listing at INFO and not at WARNING level. This is related to the old influence of the `--stats` option to change the logging level of the borg output. Since now `--stats`has no influence on the borg output level anymore, I could simplify the condition to `if files`. To make the code a bit more consistent I moved the `--list`out of extra DEBUG line similar as in `create.py`: ``` + (('--list',) if files or logger.isEnabledFor(logging.DEBUG) else ()) + (('--debug', '--show-rc') if logger.isEnabledFor(logging.DEBUG) else ()) ```
witten commented 2020-01-22 20:29:31 +00:00
Review
Copy Link

Looks good, thanks!

Looks good, thanks!
)
execute_command(
full_command,
output_log_level=logging.WARNING
if (stats and logger.getEffectiveLevel() == logging.WARNING)
else logging.INFO,
error_on_warnings=False,
)
execute_command(full_command, output_log_level=logging.INFO, error_on_warnings=False)

2
tests/unit/borg/test_create.py
View File

@ -849,7 +849,7 @@ def test_create_archive_with_stats_calls_borg_with_stats_parameter():
flexmock(module).should_receive('_make_exclude_flags').and_return(())
flexmock(module).should_receive('execute_command').with_args(
('borg', 'create', '--stats') + ARCHIVE_WITH_PATHS,
output_log_level=logging.WARNING,
output_log_level=logging.INFO,
error_on_warnings=False,
)

4
tests/unit/borg/test_prune.py
View File

@ -89,7 +89,7 @@ def test_prune_archives_with_log_debug_calls_borg_with_debug_parameter():
BASE_PRUNE_FLAGS
)
insert_execute_command_mock(
PRUNE_COMMAND + ('--stats', '--debug', '--list', '--show-rc', 'repo'), logging.INFO
PRUNE_COMMAND + ('--stats', '--list', '--debug', '--show-rc', 'repo'), logging.INFO
)
insert_logging_mock(logging.DEBUG)
@ -147,7 +147,7 @@ def test_prune_archives_with_stats_calls_borg_with_stats_parameter():
flexmock(module).should_receive('_make_prune_flags').with_args(retention_config).and_return(
BASE_PRUNE_FLAGS
)
insert_execute_command_mock(PRUNE_COMMAND + ('--stats', 'repo'), logging.WARNING)
insert_execute_command_mock(PRUNE_COMMAND + ('--stats', 'repo'), logging.INFO)
module.prune_archives(
dry_run=False,