feat: support multiple exclude pattern for restore operation #182
Conversation
5aaa8d0 to
ce74181
Compare
mtlynch
left a comment
There was a problem hiding this comment.
Thanks for adding this and filling in the missing docs!
I just have a small note about protecting clients who depended on the old behavior.
|
|
||
| if exclude: | ||
| cmd.extend(['--exclude', exclude]) | ||
| for exclude_path in exclude: |
There was a problem hiding this comment.
There's a bit of a dangerous gotcha here for clients that depended on the previous semantics.
Can we add a check here for if exclude is a string and then throw an Exception in that case?
Because otherwise, if a consuming app was calling restic.restore(exclude='/foo/bar'), we're going to turn it into --exclude / --exclude f --exclude o...
We could theoretically check if it's a string vs. a list and handle both, but I'd rather just fail hard on strings to reduce complexity.
There was a problem hiding this comment.
Good catch, I thought about it while writing this as I am actually using the lib in this configuration but was unsure about how to address it.
Your proposal is fair and I implemented it in a separated commit. Thanks !
There was a problem hiding this comment.
Thanks for those edits!
I made a few small changes:
- I added a more specific error for this case, as I generally prefer module-level exceptions for things that are scoped to a particular module.
- I got rid of the mock execute in the test, since it doesn't actually get called and I'd rather keep it simple.
- I added back the test for a single exclude path using the new semantics
restic/internal/restore_test.py
Outdated
| ]) | ||
|
|
||
| @mock.patch.object(restore.command_executor, 'execute') | ||
| def test_restore_specific_snapshot_id_and_exclude(self, mock_execute): |
There was a problem hiding this comment.
Following from the other comment, can we leave this test's call as-is but change the assertion to be that it throws the right exception?
There was a problem hiding this comment.
Done, I am unsure about how to use mock_execute so I just set it to empty string (to avoid unused variable warning).
|
@S0obi - Thanks for adding this! This is now released in 1.2.0: |
Contributing process
What type of PR is this? (check all applicable)
Description
This PR will actually support multiple
excludepatterns for therestoreoperation. Before, only one exclude pattern can be provided, now, similarly to therewriteAPI, multiple exclude patterns can be provided as a list.Did you update the API documentation?
Have you added or updated tests?