From 4de879d86e1041a434d24b0c4be87dc33a8c0e2f Mon Sep 17 00:00:00 2001 From: Florian Apolloner Date: Wed, 17 Sep 2025 08:46:02 +0200 Subject: [PATCH] address some review comments --- borgmatic/actions/restore.py | 4 ++-- borgmatic/hooks/data_source/mongodb.py | 18 ++++++++++------ borgmatic/hooks/data_source/mysql.py | 22 ++++++++++++------- borgmatic/hooks/data_source/postgresql.py | 18 ++++++++++------ tests/unit/actions/test_restore.py | 26 +++++++++++------------ 5 files changed, 52 insertions(+), 36 deletions(-) diff --git a/borgmatic/actions/restore.py b/borgmatic/actions/restore.py index 4f8b271a..70206021 100644 --- a/borgmatic/actions/restore.py +++ b/borgmatic/actions/restore.py @@ -188,7 +188,7 @@ def restore_single_dump( data_source['name'], data_source.get('hostname'), data_source.get('port'), - data_source.get('label') or data_source.get('container') or UNSPECIFIED, + data_source.get('label') or UNSPECIFIED, ), ) @@ -388,7 +388,7 @@ def collect_dumps_from_archive( except (ValueError, TypeError): port = None - dumps_from_archive.add(Dump(hook_name, data_source_name, hostname, port, host_and_port)) + dumps_from_archive.add(Dump(hook_name, data_source_name, hostname, port)) # We've successfully parsed the dump path, so need to probe any further. break diff --git a/borgmatic/hooks/data_source/mongodb.py b/borgmatic/hooks/data_source/mongodb.py index 426c37a2..b41d8c5b 100644 --- a/borgmatic/hooks/data_source/mongodb.py +++ b/borgmatic/hooks/data_source/mongodb.py @@ -6,7 +6,7 @@ import borgmatic.borg.pattern import borgmatic.config.paths import borgmatic.hooks.credential.parse from borgmatic.execute import execute_command, execute_command_with_processes -from borgmatic.hooks.data_source import config as ds_config +from borgmatic.hooks.data_source import config as database_config from borgmatic.hooks.data_source import dump logger = logging.getLogger(__name__) @@ -143,7 +143,7 @@ def build_dump_command(database, config, dump_filename, dump_format): dump_command = tuple( shlex.quote(part) for part in shlex.split(database.get('mongodump_command') or 'mongodump') ) - hostname = ds_config.resolve_database_option('hostname', database) + hostname = database_config.resolve_database_option('hostname', database) return ( dump_command + (('--out', shlex.quote(dump_filename)) if dump_format == 'directory' else ()) @@ -277,16 +277,22 @@ def build_restore_command(extract_process, database, config, dump_filename, conn ''' Return the custom mongorestore_command from a single database configuration. ''' - hostname = ds_config.resolve_database_option( + hostname = database_config.resolve_database_option( 'hostname', database, connection_params, restore=True ) - port = ds_config.resolve_database_option('port', database, connection_params, restore=True) + port = database_config.resolve_database_option( + 'port', database, connection_params, restore=True + ) username = borgmatic.hooks.credential.parse.resolve_credential( - ds_config.resolve_database_option('username', database, connection_params, restore=True), + database_config.resolve_database_option( + 'username', database, connection_params, restore=True + ), config, ) password = borgmatic.hooks.credential.parse.resolve_credential( - ds_config.resolve_database_option('password', database, connection_params, restore=True), + database_config.resolve_database_option( + 'password', database, connection_params, restore=True + ), config, ) diff --git a/borgmatic/hooks/data_source/mysql.py b/borgmatic/hooks/data_source/mysql.py index 802fa783..51710b6e 100644 --- a/borgmatic/hooks/data_source/mysql.py +++ b/borgmatic/hooks/data_source/mysql.py @@ -12,7 +12,7 @@ from borgmatic.execute import ( execute_command_and_capture_output, execute_command_with_processes, ) -from borgmatic.hooks.data_source import config as ds_config +from borgmatic.hooks.data_source import config as database_config from borgmatic.hooks.data_source import dump logger = logging.getLogger(__name__) @@ -48,7 +48,7 @@ def database_names_to_dump(database, config, username, password, environment, dr borgmatic.hooks.data_source.mariadb.parse_extra_options(database.get('list_options')) ) password_transport = database.get('password_transport', 'pipe') - hostname = ds_config.resolve_database_option('hostname', database) + hostname = database_config.resolve_database_option('hostname', database) show_command = ( mysql_show_command + ( @@ -123,7 +123,7 @@ def execute_dump_command( borgmatic.hooks.data_source.mariadb.parse_extra_options(database.get('options')) ) password_transport = database.get('password_transport', 'pipe') - hostname = ds_config.resolve_database_option('hostname', database) + hostname = database_config.resolve_database_option('hostname', database) dump_command = ( mysql_dump_command + ( @@ -354,17 +354,23 @@ def restore_data_source_dump( subprocess.Popen) to produce output to consume. ''' dry_run_label = ' (dry run; not actually restoring anything)' if dry_run else '' - hostname = ds_config.resolve_database_option( + hostname = database_config.resolve_database_option( 'hostname', data_source, connection_params, restore=True ) - port = ds_config.resolve_database_option('port', data_source, connection_params, restore=True) - tls = ds_config.resolve_database_option('tls', data_source, restore=True) + port = database_config.resolve_database_option( + 'port', data_source, connection_params, restore=True + ) + tls = database_config.resolve_database_option('tls', data_source, restore=True) username = borgmatic.hooks.credential.parse.resolve_credential( - ds_config.resolve_database_option('username', data_source, connection_params, restore=True), + database_config.resolve_database_option( + 'username', data_source, connection_params, restore=True + ), config, ) password = borgmatic.hooks.credential.parse.resolve_credential( - ds_config.resolve_database_option('password', data_source, connection_params, restore=True), + database_config.resolve_database_option( + 'password', data_source, connection_params, restore=True + ), config, ) diff --git a/borgmatic/hooks/data_source/postgresql.py b/borgmatic/hooks/data_source/postgresql.py index 7244407a..84c7982b 100644 --- a/borgmatic/hooks/data_source/postgresql.py +++ b/borgmatic/hooks/data_source/postgresql.py @@ -13,7 +13,7 @@ from borgmatic.execute import ( execute_command_and_capture_output, execute_command_with_processes, ) -from borgmatic.hooks.data_source import config as ds_config +from borgmatic.hooks.data_source import config as database_config from borgmatic.hooks.data_source import dump logger = logging.getLogger(__name__) @@ -33,7 +33,7 @@ def make_environment(database, config, restore_connection_params=None): ''' environment = dict(os.environ) - password = ds_config.resolve_database_option( + password = database_config.resolve_database_option( 'password', database, restore_connection_params, restore=restore_connection_params ) @@ -85,7 +85,7 @@ def database_names_to_dump(database, config, environment, dry_run): psql_command = tuple( shlex.quote(part) for part in shlex.split(database.get('psql_command') or 'psql') ) - hostname = ds_config.resolve_database_option('hostname', database) + hostname = database_config.resolve_database_option('hostname', database) list_command = ( psql_command + ('--list', '--no-password', '--no-psqlrc', '--csv', '--tuples-only') @@ -193,7 +193,7 @@ def dump_data_sources( ) continue - hostname = ds_config.resolve_database_option('hostname', database) + hostname = database_config.resolve_database_option('hostname', database) command = ( dump_command + ( @@ -339,12 +339,16 @@ def restore_data_source_dump( hostname, port, username, and password. ''' dry_run_label = ' (dry run; not actually restoring anything)' if dry_run else '' - hostname = ds_config.resolve_database_option( + hostname = database_config.resolve_database_option( 'hostname', data_source, connection_params, restore=True ) - port = ds_config.resolve_database_option('port', data_source, connection_params, restore=True) + port = database_config.resolve_database_option( + 'port', data_source, connection_params, restore=True + ) username = borgmatic.hooks.credential.parse.resolve_credential( - ds_config.resolve_database_option('username', data_source, connection_params, restore=True), + database_config.resolve_database_option( + 'username', data_source, connection_params, restore=True + ), config, ) diff --git a/tests/unit/actions/test_restore.py b/tests/unit/actions/test_restore.py index 32f2a6c5..997cdd26 100644 --- a/tests/unit/actions/test_restore.py +++ b/tests/unit/actions/test_restore.py @@ -564,9 +564,9 @@ def test_collect_dumps_from_archive_with_empty_dumps_metadata_path_falls_back_to ) assert archive_dumps == { - module.Dump('postgresql_databases', 'foo', label='localhost'), - module.Dump('postgresql_databases', 'bar', 'host', 1234, label='host:1234'), - module.Dump('mysql_databases', 'quux', label='localhost'), + module.Dump('postgresql_databases', 'foo'), + module.Dump('postgresql_databases', 'bar', 'host', 1234), + module.Dump('mysql_databases', 'quux'), } @@ -606,9 +606,9 @@ def test_collect_dumps_from_archive_without_dumps_metadata_falls_back_to_parsing ) assert archive_dumps == { - module.Dump('postgresql_databases', 'foo', label='localhost'), - module.Dump('postgresql_databases', 'bar', 'host', 1234, 'host:1234'), - module.Dump('mysql_databases', 'quux', label='localhost'), + module.Dump('postgresql_databases', 'foo'), + module.Dump('postgresql_databases', 'bar', 'host', 1234), + module.Dump('mysql_databases', 'quux'), } @@ -649,10 +649,10 @@ def test_collect_dumps_from_archive_parses_archive_paths_with_different_base_dir ) assert archive_dumps == { - module.Dump('postgresql_databases', 'foo', label='localhost'), - module.Dump('postgresql_databases', 'bar', label='localhost'), - module.Dump('postgresql_databases', 'baz', label='localhost'), - module.Dump('mysql_databases', 'quux', label='localhost'), + module.Dump('postgresql_databases', 'foo'), + module.Dump('postgresql_databases', 'bar'), + module.Dump('postgresql_databases', 'baz'), + module.Dump('mysql_databases', 'quux'), } @@ -691,7 +691,7 @@ def test_collect_dumps_from_archive_parses_directory_format_archive_paths(): ) assert archive_dumps == { - module.Dump('postgresql_databases', 'foo', label='localhost'), + module.Dump('postgresql_databases', 'foo'), } @@ -733,8 +733,8 @@ def test_collect_dumps_from_archive_skips_bad_archive_paths_or_bad_path_componen ) assert archive_dumps == { - module.Dump('postgresql_databases', 'foo', label='localhost'), - module.Dump('postgresql_databases', 'bar', label='localhost:abcd'), + module.Dump('postgresql_databases', 'foo'), + module.Dump('postgresql_databases', 'bar'), }