borgmatic-collective/borgmatic
9
71
Fork 44
Code Issues 56 Pull Requests 6 Actions 1 Packages Releases 154 Activity

Custom command options for MongoDB hook #837 #1041

Merged
witten merged 3 commits from gautamaggarwal2810/borgmatic:main into main 2025-03-26 05:27:04 +00:00
Conversation 4 Commits 3 Files Changed 3 +156 -4
gautamaggarwal2810 commented 2025-03-24 04:05:25 +00:00
Contributor
Copy Link

Fixes #837.Successfully ran unit tests locally.Please review and suggest changes.

Fixes #837.Successfully ran unit tests locally.Please review and suggest changes.
gautamaggarwal2810 added 2 commits 2025-03-24 04:05:26 +00:00
Custom command options for MongoDB hook #837 d651813601
Made some changes in test file 6a470be924
witten reviewed 2025-03-24 19:44:17 +00:00
witten left a comment
Owner
Copy Link

Looking good! Mostly minor comments.

Looking good! Mostly minor comments.
tests/unit/hooks/data_source/test_mongodb.py
@@ -684,0 +715,4 @@
dry_run=False,
) == [process]
def test_build_dump_command_prevents_shell_injection():
witten commented 2025-03-24 19:43:32 +00:00
Owner
Copy Link

Do you think it's worth having a test like this for the restore command as well?

Do you think it's worth having a test like this for the restore command as well?
tests/unit/hooks/data_source/test_mongodb.py
@@ -684,0 +717,4 @@
def test_build_dump_command_prevents_shell_injection():
database = {
'name': 'testdb; rm -rf /', # Malicious input
witten commented 2025-03-24 19:37:40 +00:00
Owner
Copy Link

It's not a concern here, but generally I try not to include real malicious commands even in tests—on the off chance that something is broken and they actually get executed!

It's not a concern here, but generally I try not to include real malicious commands even in tests—on the off chance that something is broken and they actually get executed!
tests/unit/hooks/data_source/test_mongodb.py Outdated
@@ -684,0 +729,4 @@
dump_filename = '/path/to/dump'
dump_format = 'archive'
from borgmatic.hooks.data_source.mongodb import build_dump_command, build_restore_command # Import the functions
witten commented 2025-03-24 19:40:22 +00:00
Owner
Copy Link

You shouldn't need to do this here (or at all) if you instead use module.build_dump_command() and module.build_restore_command().

You shouldn't need to do this here (or at all) if you instead use `module.build_dump_command()` and `module.build_restore_command()`.
witten marked this conversation as resolved
gautamaggarwal2810 added 1 commit 2025-03-25 04:52:11 +00:00
Added test_build_restore_command_prevents_shell_injection() 532a97623c
gautamaggarwal2810 commented 2025-03-25 05:00:23 +00:00
Author
Contributor
Copy Link

Thank you for the feedback! I’ve applied all the suggested changes in the latest commit, including:

  • Added unit test test_build_restore_command_prevents_shell_injection( ).
  • Used module.build_dump_command( ) instead of importing the command.

Please let me know if there’s anything else I should adjust or improve.
All unit tests ran successfully.

Thank you for the feedback! I’ve applied all the suggested changes in the latest commit, including: - Added unit test test_build_restore_command_prevents_shell_injection( ). - Used module.build_dump_command( ) instead of importing the command. Please let me know if there’s anything else I should adjust or improve. All unit tests ran successfully.
witten commented 2025-03-26 04:30:38 +00:00
Owner
Copy Link

As far as I'm concerned, this looks set to merge! Please take off the "WIP" prefix when you are ready.

As far as I'm concerned, this looks set to merge! Please take off the "WIP" prefix when you are ready.
gautamaggarwal2810 changed title from WIP: Custom command options for MongoDB hook #837 to Custom command options for MongoDB hook #837 2025-03-26 04:45:34 +00:00
witten merged commit 76f7c53a1c into main 2025-03-26 05:27:04 +00:00
witten commented 2025-03-26 05:27:13 +00:00
Owner
Copy Link

Thanks!!

Thanks!!
❤️ 1
Sign in to join this conversation.
Reviewers
No Reviewers
witten
Labels
Clear labels
blocked
breaking
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
witten
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#1041
Delete Branch "gautamaggarwal2810/borgmatic:main"

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?