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
9 changed files with 69 additions and 15 deletions
Showing only changes of commit e27ba0d08a - Show all commits

3
NEWS
View File

@ -1,3 +1,6 @@
1.4.22.dev0-files
* Removed `borg --list --stats` option from `create`and `prune` actions at verbosity 1
* New option `--files`to (re-)add the `borg` `--list` and `--stats` at verbosity 1
1.4.22.dev0
* #276: Disable colored output when "--json" flag is used, so as to produce valid JSON ouput.
* In "borgmatic --help", don't expand $HOME in listing of default "--config" paths.

View File

@ -131,6 +131,7 @@ def create_archive(
progress=False,
stats=False,
json=False,
files=False,
):
'''
Given vebosity/dry-run flags, a local or remote repository path, a location config dict, and a
@ -177,13 +178,22 @@ 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
if logger.isEnabledFor(logging.INFO)
and not json
and not progress
and (files or logger.isEnabledFor(logging.DEBUG))
Review

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

Agree, will change as suggested.

Agree, will change as suggested.
else ()
)
+ (('--info',) if logger.getEffectiveLevel() == logging.INFO and not json else ())
+ (
('--stats',)
if not dry_run and (logger.isEnabledFor(logging.INFO) or stats) and not json
if not dry_run
and (
(logger.isEnabledFor(logging.INFO) and files)
or logger.isEnabledFor(logging.DEBUG)
or stats
)
and not json
else ()
)
+ (('--debug', '--show-rc') if logger.isEnabledFor(logging.DEBUG) and not json else ())

View File

@ -41,6 +41,7 @@ def prune_archives(
local_path='borg',
remote_path=None,
stats=False,
files=False,
):
'''
Given dry-run flag, a local or remote repository path, a storage config dict, and a
@ -57,11 +58,18 @@ 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.isEnabledFor(logging.INFO) else ())
+ (('--info', '--list') if logger.getEffectiveLevel() == logging.INFO else ())
+ (
('--stats',)
if not dry_run
Review

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.
and (logger.isEnabledFor(logging.INFO) and files)
or logger.getEffectiveLevel() == logging.DEBUG
or stats
else ()
Review

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

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 ()) ```
Review

Looks good, thanks!

Looks good, thanks!
)
+ (('--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 ())
+ (('--dry-run',) if dry_run else ())
+ (('--stats',) if stats else ())
+ (tuple(extra_borg_options.split(' ')) if extra_borg_options else ())
+ (repository,)
)

View File

@ -237,6 +237,13 @@ def parse_arguments(*unparsed_arguments):
action='store_true',
help='Display statistics of archive',
)
prune_group.add_argument(
'--files',
dest='files',
default=False,
action='store_true',
help='Show file details and stats at verbosity 1',
)
prune_group.add_argument('-h', '--help', action='help', help='Show this help message and exit')
create_parser = subparsers.add_parser(
@ -261,6 +268,13 @@ def parse_arguments(*unparsed_arguments):
action='store_true',
help='Display statistics of archive',
)
create_group.add_argument(
'--files',
dest='files',
default=False,
action='store_true',
help='Show file details and stats at verbosity 1',
)
create_group.add_argument(
'--json', dest='json', default=False, action='store_true', help='Output results as JSON'
)

View File

@ -212,6 +212,7 @@ def run_actions(
local_path=local_path,
remote_path=remote_path,
stats=arguments['prune'].stats,
files=arguments['prune'].files,
)
if 'create' in arguments:
logger.info('{}: Creating archive{}'.format(repository, dry_run_label))
@ -225,6 +226,7 @@ def run_actions(
progress=arguments['create'].progress,
stats=arguments['create'].stats,
json=arguments['create'].json,
files=arguments['create'].files,
)
if json_output:
yield json.loads(json_output)

View File

@ -1,6 +1,6 @@
from setuptools import find_packages, setup
VERSION = '1.4.22.dev0'
VERSION = '1.4.22.dev0-files'

If/when you merge this, it'd be good to remove the branch version tag.

If/when you merge this, it'd be good to remove the branch version tag.
setup(

View File

@ -98,12 +98,14 @@ def test_parse_arguments_with_no_actions_defaults_to_all_actions_enabled():
def test_parse_arguments_with_no_actions_passes_argument_to_relevant_actions():
flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default'])
arguments = module.parse_arguments('--stats')
arguments = module.parse_arguments('--stats', '--files')
assert 'prune' in arguments
assert arguments['prune'].stats
assert arguments['prune'].files
assert 'create' in arguments
assert arguments['create'].stats
assert arguments['create'].files
assert 'check' in arguments
@ -423,6 +425,25 @@ def test_parse_arguments_with_stats_flag_but_no_create_or_prune_flag_raises_valu
module.parse_arguments('--stats', 'list')
def test_parse_arguments_with_files_and_create_flags_does_not_raise():
flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default'])
module.parse_arguments('--files', 'create', 'list')
def test_parse_arguments_with_files_and_prune_flags_does_not_raise():
flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default'])
module.parse_arguments('--files', 'prune', 'list')
def test_parse_arguments_with_files_flag_but_no_create_or_prune_or_restore_flag_raises_value_error():
flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default'])
with pytest.raises(SystemExit):
module.parse_arguments('--files', 'list')
def test_parse_arguments_allows_json_with_list_or_info():
flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default'])

View File

@ -295,7 +295,7 @@ def test_create_archive_with_log_info_calls_borg_with_info_parameter():
flexmock(module).should_receive('_make_pattern_flags').and_return(())
flexmock(module).should_receive('_make_exclude_flags').and_return(())
flexmock(module).should_receive('execute_command').with_args(
('borg', 'create', '--list', '--filter', 'AME-', '--info', '--stats') + ARCHIVE_WITH_PATHS,
('borg', 'create', '--info') + ARCHIVE_WITH_PATHS,
output_log_level=logging.INFO,
error_on_warnings=False,
)
@ -432,8 +432,7 @@ def test_create_archive_with_dry_run_and_log_info_calls_borg_without_stats_param
flexmock(module).should_receive('_make_pattern_flags').and_return(())
flexmock(module).should_receive('_make_exclude_flags').and_return(())
flexmock(module).should_receive('execute_command').with_args(
('borg', 'create', '--list', '--filter', 'AME-', '--info', '--dry-run')
+ ARCHIVE_WITH_PATHS,
('borg', 'create', '--info', '--dry-run') + ARCHIVE_WITH_PATHS,
output_log_level=logging.INFO,
error_on_warnings=False,
)
@ -875,8 +874,7 @@ def test_create_archive_with_progress_and_log_info_calls_borg_with_progress_para
flexmock(module).should_receive('_make_pattern_flags').and_return(())
flexmock(module).should_receive('_make_exclude_flags').and_return(())
flexmock(module).should_receive('execute_command_without_capture').with_args(
('borg', 'create', '--info', '--stats', '--progress') + ARCHIVE_WITH_PATHS,
error_on_warnings=False,
('borg', 'create', '--info', '--progress') + ARCHIVE_WITH_PATHS, error_on_warnings=False
)
insert_logging_mock(logging.INFO)

View File

@ -75,9 +75,7 @@ def test_prune_archives_with_log_info_calls_borg_with_info_parameter():
flexmock(module).should_receive('_make_prune_flags').with_args(retention_config).and_return(
BASE_PRUNE_FLAGS
)
insert_execute_command_mock(
PRUNE_COMMAND + ('--stats', '--info', '--list', 'repo'), logging.INFO
)
insert_execute_command_mock(PRUNE_COMMAND + ('--info', 'repo'), logging.INFO)
insert_logging_mock(logging.INFO)
module.prune_archives(