code review

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
+                                directory. Note that using this
-                                root of the filesystem. 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:

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)
+        execute_command(('rm', database_path), shell=True)
-    except CalledProcessError:
+    except CalledProcessError:  # pragma: no cover
         logger.info(f'{log_prefix}: Database does not exist at {database_path}, skipping removal')
 
     restore_command = (

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
+this to work with PostgreSQL.
-for SQLite databases.
 
 <span class="minilink minilink-addedin">New in version 1.7.6</span> 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

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(module.dump).should_receive('create_parent_directory_for_dump')
-    flexmock(logging).should_receive('warning')
+    flexmock(module).should_receive('execute_command').and_return(processes[0]).and_return(
-    flexmock(module).should_receive('dump.create_parent_directory_for_dump')
+        processes[1]
-    flexmock(module).should_receive('execute_command').and_return('process')
+    )
 
-    assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == [
+    assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == processes
-        'process',
+
-        'process',
+
+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(module.dump).should_receive('create_parent_directory_for_dump').never()
-    flexmock(logging).should_receive('warning')
+    flexmock(module).should_receive('execute_command').never()
-    flexmock(module).should_receive('dump.create_parent_directory_for_dump')
 
     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