From 903507bd03aed7942fd8434b7ba8f01005c7e97a Mon Sep 17 00:00:00 2001 From: Divyansh Singh Date: Sat, 4 Mar 2023 01:27:07 +0530 Subject: [PATCH] code review --- borgmatic/config/schema.yaml | 18 +++++------ borgmatic/hooks/sqlite.py | 16 +++++----- docs/how-to/backup-your-databases.md | 9 +++--- tests/unit/hooks/test_sqlite.py | 46 ++++++++++++++++++---------- 4 files changed, 50 insertions(+), 39 deletions(-) diff --git a/borgmatic/config/schema.yaml b/borgmatic/config/schema.yaml index 6f399ed2..ac8cd28b 100644 --- a/borgmatic/config/schema.yaml +++ b/borgmatic/config/schema.yaml @@ -938,24 +938,24 @@ properties: required: ['path','name'] additionalProperties: false properties: + name: + type: string + description: | + This is used to tag the database dump file + with a name. It is not the path to the database + file itself. The name "all" has no special + meaning for SQLite databases. + example: users path: type: string description: | Path to the SQLite database file to dump. If relative, it is relative to the current working - directory. If absolute, it is relative to the - root of the filesystem. Note that using this + directory. Note that using this database hook implicitly enables both read_special and one_file_system (see above) to support dump and restore streaming. example: /var/lib/sqlite/users.db - name: - type: string - description: | - This is used to tag the database dump file with - a name. It is not used to identify the database - file itself. - example: users mongodb_databases: type: array items: diff --git a/borgmatic/hooks/sqlite.py b/borgmatic/hooks/sqlite.py index f9b83aea..d31e331f 100644 --- a/borgmatic/hooks/sqlite.py +++ b/borgmatic/hooks/sqlite.py @@ -17,7 +17,7 @@ def make_dump_path(location_config): # pragma: no cover ) -def dump_databases(databases, log_prefix, location_config, dry_run): # pragma: no cover +def dump_databases(databases, log_prefix, location_config, dry_run): ''' Dump the given SQLite3 databases to a file. The databases are supplied as a sequence of configuration dicts, as per the configuration schema. Use the given log prefix in any log @@ -29,11 +29,13 @@ def dump_databases(databases, log_prefix, location_config, dry_run): # pragma: logger.info('{}: Dumping SQLite databases{}'.format(log_prefix, dry_run_label)) + if databases[0]['name'] == 'all': + logger.warning('The "all" database name has no meaning for SQLite3 databases') + for database in databases: database_path = database['path'] - database_filename = database['name'] dump_path = make_dump_path(location_config) - dump_filename = dump.make_database_dump_filename(dump_path, database_filename) + dump_filename = dump.make_database_dump_filename(dump_path, database['name']) if os.path.exists(dump_filename): logger.warning( f'{log_prefix}: Skipping duplicate dump of SQLite database at {database_path} to {dump_filename}' @@ -98,13 +100,9 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run, if dry_run: return - remove_command = ( - 'rm', - database_path, - ) try: - execute_command(remove_command, shell=True) - except CalledProcessError: + execute_command(('rm', database_path), shell=True) + except CalledProcessError: # pragma: no cover logger.info(f'{log_prefix}: Database does not exist at {database_path}, skipping removal') restore_command = ( diff --git a/docs/how-to/backup-your-databases.md b/docs/how-to/backup-your-databases.md index 5bf2002a..3e1cf2a0 100644 --- a/docs/how-to/backup-your-databases.md +++ b/docs/how-to/backup-your-databases.md @@ -28,8 +28,8 @@ hooks: mongodb_databases: - name: messages sqlite_databases: - - path: /var/lib/sqlite3/mydb.sqlite - name: mydb + path: /var/lib/sqlite3/mydb.sqlite ``` As part of each backup, borgmatic streams a database dump for each configured @@ -78,8 +78,8 @@ hooks: authentication_database: mongousers options: "--ssl" sqlite_databases: - - path: /var/lib/sqlite3/mydb.sqlite - name: mydb + path: /var/lib/sqlite3/mydb.sqlite ``` See your [borgmatic configuration @@ -103,8 +103,7 @@ hooks: ``` Note that you may need to use a `username` of the `postgres` superuser for -this to work with PostgreSQL. Also, the `all` database name is not supported -for SQLite databases. +this to work with PostgreSQL. New in version 1.7.6 With PostgreSQL and MySQL, you can optionally dump "all" databases to separate @@ -161,7 +160,7 @@ bring back any missing configuration files in order to restore a database. ## Supported databases -As of now, borgmatic supports PostgreSQL, MySQL/MariaDB, MongoDB databases and SQLite databases +As of now, borgmatic supports PostgreSQL, MySQL/MariaDB, MongoDB and SQLite databases directly. But see below about general-purpose preparation and cleanup hooks as a work-around with other database systems. Also, please [file a ticket](https://torsion.org/borgmatic/#issues) for additional database systems diff --git a/tests/unit/hooks/test_sqlite.py b/tests/unit/hooks/test_sqlite.py index d679cad0..f9e09e66 100644 --- a/tests/unit/hooks/test_sqlite.py +++ b/tests/unit/hooks/test_sqlite.py @@ -10,12 +10,10 @@ def test_dump_databases_logs_and_skips_if_dump_already_exists(): databases = [{'path': '/path/to/database', 'name': 'database'}] flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') - flexmock(module).should_receive('dump.make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( '/path/to/dump/database' ) flexmock(module.os.path).should_receive('exists').and_return(True) - flexmock(logging).should_receive('info') - flexmock(logging).should_receive('warning') assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == [] @@ -25,34 +23,49 @@ def test_dump_databases_dumps_each_database(): {'path': '/path/to/database1', 'name': 'database1'}, {'path': '/path/to/database2', 'name': 'database2'}, ] + processes = [flexmock(), flexmock()] flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') - flexmock(module).should_receive('dump.make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( '/path/to/dump/database' ) flexmock(module.os.path).should_receive('exists').and_return(False) - flexmock(logging).should_receive('info') - flexmock(logging).should_receive('warning') - flexmock(module).should_receive('dump.create_parent_directory_for_dump') - flexmock(module).should_receive('execute_command').and_return('process') + flexmock(module.dump).should_receive('create_parent_directory_for_dump') + flexmock(module).should_receive('execute_command').and_return(processes[0]).and_return( + processes[1] + ) - assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == [ - 'process', - 'process', + assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == processes + + +def test_dumping_database_with_name_all_warns_and_dumps_all_databases(): + databases = [ + {'path': '/path/to/database1', 'name': 'all'}, ] + processes = [flexmock()] + + flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') + flexmock(logging).should_receive('warning') + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( + '/path/to/dump/database' + ) + flexmock(module.os.path).should_receive('exists').and_return(False) + flexmock(module.dump).should_receive('create_parent_directory_for_dump') + flexmock(module).should_receive('execute_command').and_return(processes[0]) + + assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == processes def test_dump_databases_does_not_dump_if_dry_run(): databases = [{'path': '/path/to/database', 'name': 'database'}] flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') - flexmock(module).should_receive('dump.make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( '/path/to/dump/database' ) flexmock(module.os.path).should_receive('exists').and_return(False) - flexmock(logging).should_receive('info') - flexmock(logging).should_receive('warning') - flexmock(module).should_receive('dump.create_parent_directory_for_dump') + flexmock(module.dump).should_receive('create_parent_directory_for_dump').never() + flexmock(module).should_receive('execute_command').never() assert module.dump_databases(databases, 'test.yaml', {}, dry_run=True) == [] @@ -61,7 +74,8 @@ def test_restore_database_dump_restores_database(): database_config = [{'path': '/path/to/database', 'name': 'database'}] extract_process = flexmock(stdout=flexmock()) - flexmock(module).should_receive('execute_command_with_processes').and_return('process') + flexmock(module).should_receive('execute_command_with_processes').once() + flexmock(module).should_receive('execute_command').once() module.restore_database_dump( database_config, 'test.yaml', {}, dry_run=False, extract_process=extract_process