Borgmatic without arguments/parameters should show usage help instead of starting a backup #262 #1046
witten
Reference in New Issue
Block a user
Delete Branch "gautamaggarwal2810/borgmatic:issue262"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This is first draft for PR for issue #262.
I have tried to implement what I understood in discussion on this issue on thread #262 .
Could you please kindly review this and suggest changes??Also should I add unit tests for this??
@@ -946,0 +951,4 @@"""if len(sys.argv) == 1: # No arguments providedshow_help_on_no_args = any(config.get('show_help_on_no_args', False)I might suggest calling this option something a little more succinct like
default_actionsor similar, which would require flipping the logic and the default (but still leaving it as a boolean). The idea is that ifdefault_actionsistrue, then borgmatic applies default actions when none are supplied. But ifdefault_actionsisfalse, then it doesn't supply any defaults and just exits with help displayed.It's certainly less specific than your option name, but IMO that's probably okay for this use case.
@@ -946,0 +955,4 @@for config in load_configurations(tuple(collect.collect_config_filenames(None)))[0].values())if show_help_on_no_args:print(parse_arguments('--help'))It's too bad you don't have easy access to the
argparseparser at this point, or you could just doparser.print_help()!In any case, I don't think you need the
print()here...?parse_arguments()should handle printing the help.@@ -972,2 +988,4 @@sys.exit(0)# Use the helper function to check and show help on no argumentscheck_and_show_help_on_no_args()If you move this down a few lines, then you could pass in the already-loaded
configsand you wouldn't have to load them again.Thanks for taking time to give your feedback @witten!!
I have made all changes suggested by you.
Please review and suggest changes if any.
Thanks 😄
This looks good to me! To answer your earlier question, yes, I think this should have automated tests. I don't think you need to worry about
main()—it's not currently under test at all—but IMO the logic incheck_and_show_help_on_no_args()should get exercised by a test. Whether that makes the most sense as a unit test or an integration test is up to; just please make sure it goes in the right directory. And thank you!@@ -2668,0 +2668,4 @@default_actions:type: booleandescription: |Whether to apply default actions (e.g., backup) when no argumentsIt might be nice to briefly list those actions here. They're mentioned here in the docs: https://torsion.org/borgmatic/docs/how-to/set-up-backups/#default-actions
Speaking of, I think your new option should probably be mentioned there in the docs! Do you want to take a stab at that or would you rather I added it? I'm fine either way.
I can definitely add the new option to the docs. I’ll make the changes and submit it in a bit. If you have any specific suggestions for how it should be worded,let me know.
Which page of docs should be updated with this info? Should it be on run-arbitary-borg-commands.md or set-up-backups.md??
No specific wording suggestions at this point, but I think the idea is just to briefly say: If you want to disable this default actions feature entirely, do this...
I'd put it right here, maybe after the "skipping action" section: https://torsion.org/borgmatic/docs/how-to/set-up-backups/#default-actions
WIP: Borgmatic without arguments/parameters should show usage help instead of starting a backup #262to Borgmatic without arguments/parameters should show usage help instead of starting a backup #262Added unit tests and made changes. All tests ran successfully !!
@@ -2082,0 +2098,4 @@flexmock(module).should_receive('parse_arguments').never()flexmock(module.sys).should_receive('exit').never()module.check_and_show_help_on_no_args({'test.yaml': {'default_actions': False}})Tests look great! Do you think it's also worth testing the case of multiple (conflicting) configs?
I have added test cases for the case of multiple conflicting configs and also updated the docs. All tests ran successfully !!
Minor feedback below!
@@ -2668,0 +2672,4 @@are supplied to the borgmatic command. If set to true, borgmaticwill trigger the default actions,which include :-Create backups based on your configured schedules and paths.- Prune old backups based on your retention policies.I think you're missing "compact" here.
Also, IMO you can just list the action names like you do in parentheses in the docs; you don't have to spell out what each action does in this schema unless you want that level of detail.
@@ -296,6 +296,16 @@ skip_actions:- compact```## Disabling default actionsMinor nit: I'd do
###here instead of##so this heading is nested under the## Default actionsheading above.@@ -298,1 +298,4 @@## Disabling default actionsBy default, running `borgmatic` without any arguments will perform the default backup actions (create, prune, and compact). If you want to disable this behavior and require explicit actions to be specified, add the following to your configuration:It's actually: create, prune, compact, and check!
Also, minor nit: Wrap docs lines to 80 characters, please.
@@ -299,0 +304,4 @@default_actions: false```With this setting, running `borgmatic` without arguments will show the help message instead of performing any actions.These docs look good! Nice and to the point. Thank you for adding them!
Looks great! Thank you!!