borgmatic-collective/borgmatic
borgmatic-collective
/
borgmatic
15
57
Fork 40
Code Issues 54 Pull Requests 1 Actions Packages Releases 121 Activity

WIP: Add locking of borgmatic config file #254

Closed
drewkett wants to merge 9 commits from (deleted):configlock into main
pull from: (deleted):configlock
merge into: borgmatic-collective:main
Conversation 5 Commits 9 Files Changed 2 +29 -30
2 changed files with 29 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 398c129411 - Show all commits

57
borgmatic/commands/borgmatic.py
View File

@ -65,35 +65,34 @@ def run_configuration(config_filename, config, arguments):
'{}: Failed to acquire lock'.format(config_filename), error
)
if not encountered_error:
if 'create' in arguments:
try:
dispatch.call_hooks(
'ping_monitor',
hooks,
config_filename,
monitor.MONITOR_HOOK_NAMES,
monitor.State.START,
global_arguments.dry_run,
)
command.execute_hook(
hooks.get('before_backup'),
hooks.get('umask'),
config_filename,
'pre-backup',
global_arguments.dry_run,
)
dispatch.call_hooks(
'dump_databases',
hooks,
config_filename,
dump.DATABASE_HOOK_NAMES,
global_arguments.dry_run,
)
except (OSError, CalledProcessError) as error:
encountered_error = error
yield from make_error_log_records(
'{}: Error running pre-backup hook'.format(config_filename), error
if not encountered_error and 'create' in arguments:
try:
witten commented 2019-12-09 18:00:00 +00:00
Outdated
Review
Copy Link

These can be combined into a single if statement.

These can be combined into a single `if` statement.
dispatch.call_hooks(
'ping_monitor',
hooks,
config_filename,
monitor.MONITOR_HOOK_NAMES,
monitor.State.START,
global_arguments.dry_run,
)
command.execute_hook(
hooks.get('before_backup'),
hooks.get('umask'),
config_filename,
'pre-backup',
global_arguments.dry_run,
)
dispatch.call_hooks(
'dump_databases',
hooks,
config_filename,
dump.DATABASE_HOOK_NAMES,
global_arguments.dry_run,
)
except (OSError, CalledProcessError) as error:
encountered_error = error
yield from make_error_log_records(
'{}: Error running pre-backup hook'.format(config_filename), error
)
if not encountered_error:

2
borgmatic/config/schema.yaml
View File

@ -34,7 +34,7 @@ map:
example: true
lock_client:
type: bool
desc: Lock config when running borgmatic to prevent multiple instances from running simultaneously
desc: Lock config when running borgmatic to prevent multiple instances from running simultaneously. Defaults to false.
witten commented 2019-12-09 18:04:46 +00:00
Outdated
Review
Copy Link

You could add: ". Defaults to false." here. See other descriptions for an example.

You could add: ". Defaults to false." here. See other descriptions for an example.
example: true
witten commented 2019-12-03 01:01:31 +00:00
Review
Copy Link

The fact that it's locking the configuration file might just be an implementation detail as far as the user is concerned. What about calling this something like lock_client, lock_borgmatic, etc? I don't feel strongly about this in any case.

The fact that it's locking the configuration file might just be an implementation detail as far as the user is concerned. What about calling this something like `lock_client`, `lock_borgmatic`, etc? I don't feel strongly about this in any case.
drewkett commented 2019-12-03 06:52:39 +00:00
Review
Copy Link

Those sound better. I’ll fix it when I get the chance

Those sound better. I’ll fix it when I get the chance
drewkett commented 2019-12-03 19:28:14 +00:00
Review
Copy Link

I renamed it to lock_client and left it as a bool for now. I didn't see immediately the best way to do it with the config (and validation), but if you want to keep options open switching this to something like lock: "config" so that then you could add a lock: "everything option later would be an option.

I renamed it to `lock_client` and left it as a bool for now. I didn't see immediately the best way to do it with the config (and validation), but if you want to keep options open switching this to something like `lock: "config"` so that then you could add a `lock: "everything` option later would be an option.
drewkett commented 2019-12-03 19:29:27 +00:00
Review
Copy Link

I also wasn't sure if the example line is supposed to be the default value or not.

I also wasn't sure if the example line is supposed to be the default value or not.
witten commented 2019-12-03 21:19:54 +00:00
Review
Copy Link

A bool is fine for now, as far as I'm concerned.

example: is not usually the default value. In fact it's often the opposite of the default value in that it's demonstrating how you would override the default if you are so inclined. So example: true sounds right to me here, given that I'm assuming the default is false (don't lock).

A bool is fine for now, as far as I'm concerned. `example:` is not usually the default value. In fact it's often the *opposite* of the default value in that it's demonstrating how you would override the default if you are so inclined. So `example: true` sounds right to me here, given that I'm assuming the default is `false` (don't lock).
numeric_owner:
type: bool