Added TIMESPAN flags to match archive in various commands (Borg2 feature) #661
witten
Reference in New Issue
Block a user
Delete Branch ":borg2-archive-flags"
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?
Fixes #659
Also adds
--firstand--lastflags support to mount option.Subcommands support
info
transfer
prune
mount
rlist
Tasks:
Adjust tests
Rewrite help text for args if needed
Cleanup debug messages and formatting
Signed-off-by: jetchirag thechiragaggarwal@gmail.com
Nice! This approach seems like it'll work just great.
@@ -35,6 +35,7 @@ def run_mount(mount_arguments.paths,mount_arguments.foreground,mount_arguments.options,mount_arguments,I would recommend either exploding out
mount_argumentsinto individual parameters (probably not the best idea given the number of new arguments) or removing the individualmount_arguments.mount_point, etc. parameters and passing inmount_argumentsonly.@@ -38,0 +41,4 @@+ (flags.make_flags('newest', mount_arguments.newest) if mount_arguments.newest else ())+ (flags.make_flags('oldest', mount_arguments.oldest) if mount_arguments.oldest else ())+ (flags.make_flags('older', mount_arguments.older) if mount_arguments.older else ())+ (flags.make_flags('newer', mount_arguments.newer) if mount_arguments.newer else ())As you discovered, you can maybe use
borgmatic.borg.flags.make_flags_from_arguments()here to make this a little more succinct. That might take care of some of the other arguments too.@@ -322,2 +322,4 @@'--last', metavar='N', help='Only transfer last N archives after other filters are applied')transfer_group.add_argument('--oldest', metavar='TIMESPAN', help='Transfer archives within a specified time range starting from the timestamp of the oldest archive (e.g. 7d or 12m) [Borg 2.x+ only]'If you're going to use square brackets for the Borg 2 only part (which seems fine), it might be worth scrubbing through the rest of the arguments to standardize.
@@ -324,0 +325,4 @@'--oldest', metavar='TIMESPAN', help='Transfer archives within a specified time range starting from the timestamp of the oldest archive (e.g. 7d or 12m) [Borg 2.x+ only]')transfer_group.add_argument('--newest', metavar='TIMESPAN', help='Transfer archives within a time range that ends at newest archive\'s timestamp and starts a specified time range ago (e.g. 7d or 12m) [Borg 2.x+ only]'Minor nit: I think PEP 8 says to vary outer quotes instead of backslashing quotes in strings. So for instance:
"I won't"instead ofI won\'t. There might even be a flake8 rule for this.Is length of help text a problem?
Also, please let me know when this one is ready for review again. Thanks!
This is looking good! Note that there's unfortunately been some work in master that will pose minor conflicts with this branch. Hopefully it won't be a big deal to merge those in.
What's next? Test updates?
@@ -39,0 +34,4 @@+ flags.make_flags_from_arguments(mount_arguments,excludes=('repository', 'archive', 'mount_point', 'path', 'options'),I think
pathshould bepathshere...?Yes it should be paths
@@ -74,0 +73,4 @@+ flags.make_flags_from_arguments(prune_arguments,excludes=('repository', 'stats'),)Nice way to cut down on many of the explicit arguments.
@@ -324,0 +325,4 @@'--oldest', metavar='TIMESPAN', help='Transfer archives within a specified time range starting from the timestamp of the oldest archive (e.g. 7d or 12m) [Borg 2.x+ only]')transfer_group.add_argument('--newest', metavar='TIMESPAN', help='Transfer archives within a time range that ends at timestamp of the newest archive and starts a specified time range ago (e.g. 7d or 12m) [Borg 2.x+ only]'FYI I'm fine leaving the wording in these descriptions if you want to. It's accurate even if it's a little hard to parse (for me).
I couldn't find a way to simplify it further which can also be read easily. This definitely requires one or two re-reads for first time. I'll check back on this again after tests.
Sounds good. Maybe the Borg project already did the heavy lifting simplifying it as far as it could reasonably go.
No worries, I'll resolve them.
Yes, I got a little occupied with my exams. I'll write tests as soon as possible.
No worries. Exams are definitely more important. 😄
@witten I did not encounter any merge issue.
Can you give me pointers on testing? I suppose we can add a date-based matching test for each subcommand under
/tests/unit/borg. Is there anything else?Yeah, that sounds good to me.. just to make sure the new arguments get constructed correctly. There are plenty of existing tests to look at in that directory for examples, but I'd be happy to help out if something's not clear. Given that both
borgmatic/borg/prune.pyandborgmatic/borg/mount.pybuild up their flags to Borg in a main function rather than a separate flags-building function, a certain amount of mocking will be required.There's one other candidate for testing I can think of: In
tests/integration/borg/test_commands.py, there are integration tests for several borgmatic actions (subcommands) that usemake_flags_from_arguments(). The idea is that incorrect use ofmake_flags_from_arguments()can result in duplicated arguments passed to Borg, so those tests check for that. Given that you've introducedmake_flags_from_arguments()topruneandmount, you could potentially also add tests for them totest_commands.py.I don't think your
arguments.pychanges need tests, because there's no real logic there. There are some argument integration tests intests/integration/commands/test_arguments.py, but that's mostly for testing logic and interactions between arguments and actions.Thank you! Please let me know if you have any questions on this.
@witten Sorry for the delay. Exams still going on 😅.
I've updated/fixed existing tests and added a test to prune for your review. Can you please take a quick look and I'll push new commits and add tests for rest of subcommands?
@@ -109,0 +119,4 @@borgmatic.borg.prune.prune_archives(False, 'repo', {}, {}, '2.3.4', fuzz_argument(arguments, argument_name))This looks good to me! Thank you for adding it.
@@ -294,1 +335,4 @@retention_config=flexmock(),local_borg_version='1.2.3',prune_arguments=prune_arguments,)This test looks great!
WIP: Added TIMESPAN flags to match archive in various commands (Borg2 feature)to Added TIMESPAN flags to match archive in various commands (Borg2 feature)Almost done! Just a couple more mocks (noted in comments) and I think that's it. I appreciate you sticking with this.
@@ -233,0 +221,4 @@)def test_mount_archive_with_date_based_matching_calls_borg_with_date_based_flags():Given that this is a unit test instead of integration test, I think this should probably also mock out
make_flags_from_argumentslike you did in some your other tests.@@ -294,0 +314,4 @@)def test_prune_archives_with_date_based_matching_calls_borg_with_date_based_flags():I didn't catch the first time around, but: I also think this test should mock out
make_flags_from_arguments.@@ -509,0 +547,4 @@'--oldest','1w','repo',)Nice!
@witten I mostly merge with Github and seem to have messed up conflict resolving with merge as it's assigning other commits to this pull. Can you help?
Can you point me at where things are messed up and/or have conflicts? Looking at your branch, it appears to be up-to-date with master and only has your own commits as part of the PR!
Last time I checked I thought I was getting other commits in my files changed. It looks fine right now though.
This looks great to me!
@@ -127,0 +144,4 @@'2.3.4',fuzz_argument(arguments, argument_name),argparse.Namespace(log_json=False),)Given the changes you've made to the mount action, it looks like it should also be represented with a mount-specific test in this integration test file. However, so as to avoid another round of back and forth, I can just add it myself after merging!
Thank you so much for continuing to push on this through all the revisions. I really appreciate it!