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

Add "--repository" flag to common actions (where it makes sense) #652

Merged
witten merged 7 commits from nain/borgmatic:repository-ticket-#564 into master 2023-03-16 20:21:41 +00:00
Conversation 4 Commits 7 Files Changed 10 +294 -19
nain commented 2023-03-15 16:53:02 +00:00
Contributor
Copy Link

ticket #564 prune action

ticket #564 prune action
nain commented 2023-03-15 16:57:08 +00:00
Author
Contributor
Copy Link

Would like some feedback on this pull req.
Tried to implement --repository flag for prune action.

There is some complain by integration test though about " test_parse_arguments_disallows_repository_unless_action_consumes_it".
But we are consuming it in the action, so I don't understand why it's firing.
Help requested.

____________________________________________________________
test_parse_arguments_disallows_repository_unless_action_consumes_it
_____________________________________________________________

    def test_parse_arguments_disallows_repository_unless_action_consumes_it():
        flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default'])
    
>       with pytest.raises(SystemExit):
E       Failed: DID NOT RAISE <class 'SystemExit'>

tests/integration/commands/test_arguments.py:260: Failed

I did take care of the other (unit) test present and added new ones too, for this feature.

Would like some feedback on this pull req. Tried to implement --repository flag for prune action. There is some complain by integration test though about " test_parse_arguments_disallows_repository_unless_action_consumes_it". But we are consuming it in the action, so I don't understand why it's firing. Help requested. ``` ____________________________________________________________ test_parse_arguments_disallows_repository_unless_action_consumes_it _____________________________________________________________ def test_parse_arguments_disallows_repository_unless_action_consumes_it(): flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default']) > with pytest.raises(SystemExit): E Failed: DID NOT RAISE <class 'SystemExit'> tests/integration/commands/test_arguments.py:260: Failed ``` I did take care of the other (unit) test present and added new ones too, for this feature.
nain commented 2023-03-15 17:00:52 +00:00
Author
Contributor
Copy Link

ftr: I'll get to the other ones (check, compact, create) when this one becomes clearer

ftr: I'll get to the other ones (check, compact, create) when this one becomes clearer
witten commented 2023-03-15 18:28:40 +00:00
Owner
Copy Link

This is great! I'd say you can just delete that failing test, now that --repository isn't expected to error for the prune action any longer. Thanks! I'll have a look at the rest of the PR when I get a chance.

This is great! I'd say you can just delete that failing test, now that `--repository` isn't expected to error for the `prune` action any longer. Thanks! I'll have a look at the rest of the PR when I get a chance.
nain force-pushed repository-ticket-#564 from 9228994018 to cdbe6cdf3a 2023-03-15 18:45:05 +00:00 Compare
nain added 1 commit 2023-03-15 18:59:55 +00:00
witten requested changes 2023-03-15 23:30:16 +00:00
witten left a comment
Owner
Copy Link

This looks great! I added one suggestion about combining a couple tests, but this looks ready to merge after that's resolved (one way or the other). Thank you!

This looks great! I added one suggestion about combining a couple tests, but this looks ready to merge after that's resolved (one way or the other). Thank you!
👍 1 😆 1
tests/unit/actions/test_prune.py
@ -11,0 +26,4 @@
)
def test_run_prune_runs_with_no_explicit_repository():
witten commented 2023-03-15 23:28:42 +00:00
Owner
Copy Link

I might suggest combining this test with the previous one, as (unless I'm missing something) their setups are identical besides should_receive('execute_hook') expectation.

I might suggest combining this test with the previous one, as (unless I'm missing something) their setups are identical besides `should_receive('execute_hook')` expectation.
nain commented 2023-03-16 18:37:07 +00:00
Author
Contributor
Copy Link

Done.

Done.
witten marked this conversation as resolved
nain added 1 commit 2023-03-16 12:23:54 +00:00
nain requested review from witten 2023-03-16 14:34:05 +00:00
nain force-pushed repository-ticket-#564 from 011a45d680 to 480addd7ce 2023-03-16 14:41:41 +00:00 Compare
nain added 1 commit 2023-03-16 15:15:23 +00:00
nain added 1 commit 2023-03-16 17:16:35 +00:00
nain added 1 commit 2023-03-16 18:02:22 +00:00
3e22414613 Update tests 
Make them more explicit. Also formatting.
nain changed title from WIP: Add "--repository" flag to the "prune" action to WIP: Add "--repository" flag to common actions (where it makes sense) 2023-03-16 18:04:37 +00:00
nain changed title from WIP: Add "--repository" flag to common actions (where it makes sense) to Add "--repository" flag to common actions (where it makes sense) 2023-03-16 18:37:53 +00:00
witten merged commit 7a784b8eba into master 2023-03-16 20:21:41 +00:00
witten commented 2023-03-16 20:22:04 +00:00
Owner
Copy Link

Everything looks great! I just merged it. Thanks again for your work here.

Everything looks great! I just merged it. Thanks again for your work here.
Sign in to join this conversation.
Reviewers
No reviewers
witten
Labels
Clear labels   
bug

   
data loss

   
design finalized

   
good first issue

   
new feature area

   
question / support

   
security

   
waiting for response
No Label
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#652
No description provided.
Delete Branch "nain/borgmatic:repository-ticket-#564"

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?