WIP: Adding "actions" runtime variable #657 #695

Closed
C-Duv wants to merge 1 commits from C-Duv/borgmatic:feat/adding_action_names_to_hook_context into main
First-time contributor

This PR is a attempt to implement #657.

I originally wanted to fill hook_context with list instead of plain string but, for consistency and simplicity went for the plain string right away.

I've chosen to sort action names when adding to hook_context to ease future checks in hooks.

I tried to add pertinent tests that checks:

  • hook_context do get the name: test_run_actions_adds_action_name_to_hook_context()
  • hook_context do get all the names: test_run_actions_adds_all_sorted_action_names_to_hook_context()

But, as it's my first attempt to test, don't hesitate to comment on more/better tests to add.

Documentation was modified to add this new variable and it's purpose.

There is a TODO: in the documentation as a placeholder for the version this variable will be released with: I can set the desired version or remove the note.

PR summary:

  • contains implementation
  • contains tests
  • contains updated documentation
  • has no leftover TODO:
This PR is a attempt to implement #657. I [originally wanted to fill `hook_context` with list](https://projects.torsion.org/borgmatic-collective/borgmatic/issues/657#issuecomment-5729) instead of plain string but, for consistency and simplicity went for the plain string right away. I've chosen to sort action names when adding to `hook_context` to ease future checks in hooks. I tried to add pertinent tests that checks: * `hook_context` do get the name: `test_run_actions_adds_action_name_to_hook_context()` * `hook_context` do get all the names: `test_run_actions_adds_all_sorted_action_names_to_hook_context()` But, as it's my first attempt to test, don't hesitate to comment on more/better tests to add. Documentation was modified to add this new variable and it's purpose. There is a `TODO:` in the documentation as a placeholder for the version this variable will be released with: I can set the desired version or remove the note. PR summary: - [x] contains implementation - [x] contains tests - [x] contains updated documentation - [ ] has no leftover `TODO:`
Owner

These tests look great! Docs changes look good. Honestly, with the one wording change and the TODO filled out, I think this would be ready to go. Thanks so much for taking the time.

These tests look great! Docs changes look good. Honestly, with the one wording change and the TODO filled out, I think this would be ready to go. Thanks so much for taking the time.
Author
First-time contributor

Honestly, with the one wording change

Sorry but I don't see the change you are talking about. Maybe you forgot to finish the code review on Gitea?

Do you want me to set to TODO (1.7.13? 1.8.0?) or I you want to keep it as is until you are ready to release?

I noticed a failure on test test_run_actions_adds_log_file_to_hook_context:

E       flexmock.exceptions.MethodSignatureError: Arguments for call run_create did not match expectations:
E         Received call:	run_create(<flexmock._api.MockClass object at 0x7fc666fd5350>, {'path': 'repo'}, {'repositories': []}, <flexmock._api.MockClass object at 0x7fc666fd6850>, {}, {'actions': 'create', 'repository': 'repo', 'repositories': '', 'log_file': 'foo'}, <flexmock._api.MockClass object at 0x7fc666fd5010>, <flexmock._api.MockClass object at 0x7fc666fd5e50>, <flexmock._api.MockClass object at 0x7fc666fd7210>, "", <flexmock._api.MockClass object at 0x7fc666fd4750>, <flexmock._api.MockClass object at 0x7fc666fd79d0>)
E         Expected call[1]:	run_create(config_filename=<class 'object'>, repository={'path': 'repo'}, location={'repositories': []}, storage=<class 'object'>, hooks={}, hook_context={'repository': 'repo', 'repositories': '', 'log_file': 'foo'}, local_borg_version=<class 'object'>, create_arguments=<class 'object'>, global_arguments=<class 'object'>, dry_run_label="", local_path=<class 'object'>, remote_path=<class 'object'>)

I wasn't sure I needed to modify this existing test :)
I'll amend my commit then.

> Honestly, with the one wording change Sorry but I don't see the change you are talking about. Maybe you forgot to finish the code review on Gitea? Do you want me to set to `TODO` (1.7.13? 1.8.0?) or I you want to keep it as is until you are ready to release? I noticed a failure on test `test_run_actions_adds_log_file_to_hook_context`: ``` E flexmock.exceptions.MethodSignatureError: Arguments for call run_create did not match expectations: E Received call: run_create(<flexmock._api.MockClass object at 0x7fc666fd5350>, {'path': 'repo'}, {'repositories': []}, <flexmock._api.MockClass object at 0x7fc666fd6850>, {}, {'actions': 'create', 'repository': 'repo', 'repositories': '', 'log_file': 'foo'}, <flexmock._api.MockClass object at 0x7fc666fd5010>, <flexmock._api.MockClass object at 0x7fc666fd5e50>, <flexmock._api.MockClass object at 0x7fc666fd7210>, "", <flexmock._api.MockClass object at 0x7fc666fd4750>, <flexmock._api.MockClass object at 0x7fc666fd79d0>) E Expected call[1]: run_create(config_filename=<class 'object'>, repository={'path': 'repo'}, location={'repositories': []}, storage=<class 'object'>, hooks={}, hook_context={'repository': 'repo', 'repositories': '', 'log_file': 'foo'}, local_borg_version=<class 'object'>, create_arguments=<class 'object'>, global_arguments=<class 'object'>, dry_run_label="", local_path=<class 'object'>, remote_path=<class 'object'>) ``` I wasn't sure I needed to modify this existing test :) I'll amend my commit then.
witten requested changes 2023-05-14 16:31:49 +00:00
@ -65,2 +65,4 @@
variables you can use here:
* `actions`
<span class="minilink minilink-addedin">New in version TODO:</span>:
Owner

If this is done soon, it'll probably be part of 1.7.13!

If this is done soon, it'll probably be part of 1.7.13!
Author
First-time contributor

Done :)

Done :)
C-Duv marked this conversation as resolved
@ -66,1 +66,4 @@
* `actions`
<span class="minilink minilink-addedin">New in version TODO:</span>:
comma-separated list of all the action(s) borgmatic will run
Owner

This is good, but I'd recommend changing the tense to something like:

comma-separated list of all the action(s) borgmatic is running

That's because hooks can run before or after the actions.

This is good, but I'd recommend changing the tense to something like: ``` comma-separated list of all the action(s) borgmatic is running ``` That's because hooks can run before _or_ after the actions.
Author
First-time contributor

I get it: wording was changed.

I get it: wording was changed.
C-Duv marked this conversation as resolved
Owner

Sorry but I don't see the change you are talking about. Maybe you forgot to finish the code review on Gitea?

You're correct. My apologies! I just submitted it now.

Do you want me to set to TODO (1.7.13? 1.8.0?) or I you want to keep it as is until you are ready to release?

You can set it to 1.7.13. (I can always change it if need be.)

I wasn't sure I needed to modify this existing test :)

Yeah, it looks like the expectation about the hook_context in particular is incorrect now. So it would be good to update it so the test passes. Thanks!

> Sorry but I don't see the change you are talking about. Maybe you forgot to finish the code review on Gitea? You're correct. My apologies! I just submitted it now. > Do you want me to set to TODO (1.7.13? 1.8.0?) or I you want to keep it as is until you are ready to release? You can set it to 1.7.13. (I can always change it if need be.) > I wasn't sure I needed to modify this existing test :) Yeah, it looks like the expectation about the `hook_context` in particular is incorrect now. So it would be good to update it so the test passes. Thanks!
C-Duv force-pushed feat/adding_action_names_to_hook_context from e8bcae3916 to f17fde399e 2023-05-14 17:13:10 +00:00 Compare
Author
First-time contributor

I pushed the changes but I still have 2 issues with the tests (ran wit tox -- tests/unit/commands/test_borgmatic.py):

test_run_actions_adds_action_name_to_hook_context:

AttributeError: 'MockClass' object has no attribute 'log_file'

I don't understand why it's complaining about log_file :'(

test_run_actions_adds_all_sorted_action_names_to_hook_context:

flexmock.exceptions.MethodCallError: run_check(config_filename=<class 'object'>, repository={'path': 'repo'}, location={'repositories': []}, storage=<class 'object'>, consistency=<class 'object'>, hooks={}, hook_context={'repository': 'repo', 'repositories': '', 'actions': 'check,prune'}, local_borg_version=<class 'object'>, check_arguments=<class 'object'>, global_arguments=<class 'object'>, local_path=<class 'object'>, remote_path=<class 'object'>) expected to be called exactly 1 time, called 0 times

For this one I guess it's because I've choosed the check action as an example for my test?

I pushed the changes but I still have 2 issues with the tests (ran wit `tox -- tests/unit/commands/test_borgmatic.py`): `test_run_actions_adds_action_name_to_hook_context`: > AttributeError: 'MockClass' object has no attribute 'log_file' I don't understand why it's complaining about `log_file` :'( `test_run_actions_adds_all_sorted_action_names_to_hook_context`: > flexmock.exceptions.MethodCallError: run_check(config_filename=<class 'object'>, repository={'path': 'repo'}, location={'repositories': []}, storage=<class 'object'>, consistency=<class 'object'>, hooks={}, hook_context={'repository': 'repo', 'repositories': '', 'actions': 'check,prune'}, local_borg_version=<class 'object'>, check_arguments=<class 'object'>, global_arguments=<class 'object'>, local_path=<class 'object'>, remote_path=<class 'object'>) expected to be called exactly 1 time, called 0 times For this one I guess it's because I've choosed the `check` action as an example for my test?
Owner

I apologize for the delay here.. I'm not sure I received an email notification for your most recent comment!

I pushed the changes but I still have 2 issues with the tests (ran wit tox -- tests/unit/commands/test_borgmatic.py):

test_run_actions_adds_action_name_to_hook_context:

AttributeError: 'MockClass' object has no attribute 'log_file'

I don't understand why it's complaining about log_file :'(

My guess is that's because this line:

arguments={'global': flexmock(dry_run=False), 'create': flexmock()},

... needs to look more like this:

arguments={'global': flexmock(dry_run=False, log_file='foo'), 'create': flexmock()},

You can check out some of the other tests in test_borgmatic.py for a comparison. If this doesn't solve it though, then I'd need to see more of the error message to help diagnose.

test_run_actions_adds_all_sorted_action_names_to_hook_context:

flexmock.exceptions.MethodCallError: run_check(config_filename=<class 'object'>, repository={'path': 'repo'}, location={'repositories': []}, storage=<class 'object'>, consistency=<class 'object'>, hooks={}, hook_context={'repository': 'repo', 'repositories': '', 'actions': 'check,prune'}, local_borg_version=<class 'object'>, check_arguments=<class 'object'>, global_arguments=<class 'object'>, local_path=<class 'object'>, remote_path=<class 'object'>) expected to be called exactly 1 time, called 0 times

For this one I guess it's because I've choosed the check action as an example for my test?

Yeah, my guess is that checks.repository_enabled_for_checks() is returning False, and therefore your mocked run_check() isn't getting called. So one thing you could do to solve this is to mock out checks.repository_enabled_for_checks() as well, forcing it to return True. Or you could just not use the check action in your test.

I apologize for the delay here.. I'm not sure I received an email notification for your most recent comment! > I pushed the changes but I still have 2 issues with the tests (ran wit tox -- tests/unit/commands/test_borgmatic.py): > > test_run_actions_adds_action_name_to_hook_context: > > AttributeError: 'MockClass' object has no attribute 'log_file' > > I don't understand why it's complaining about log_file :'( My guess is that's because this line: ``` arguments={'global': flexmock(dry_run=False), 'create': flexmock()}, ``` ... needs to look more like this: ``` arguments={'global': flexmock(dry_run=False, log_file='foo'), 'create': flexmock()}, ``` You can check out some of the other tests in `test_borgmatic.py` for a comparison. If this doesn't solve it though, then I'd need to see more of the error message to help diagnose. > test_run_actions_adds_all_sorted_action_names_to_hook_context: > > flexmock.exceptions.MethodCallError: run_check(config_filename=<class 'object'>, repository={'path': 'repo'}, location={'repositories': []}, storage=<class 'object'>, consistency=<class 'object'>, hooks={}, hook_context={'repository': 'repo', 'repositories': '', 'actions': 'check,prune'}, local_borg_version=<class 'object'>, check_arguments=<class 'object'>, global_arguments=<class 'object'>, local_path=<class 'object'>, remote_path=<class 'object'>) expected to be called exactly 1 time, called 0 times > > For this one I guess it's because I've choosed the check action as an example for my test? Yeah, my guess is that `checks.repository_enabled_for_checks()` is returning `False`, and therefore your mocked `run_check()` isn't getting called. So one thing you could do to solve this is to mock out `checks.repository_enabled_for_checks()` as well, forcing it to return `True`. Or you could just not use the `check` action in your test.
Owner

Just checking in.. Do you still plan to work on this? Thanks!

Just checking in.. Do you still plan to work on this? Thanks!
Owner

Closing this for now due to inactivity, but I'd be happy to revisit this if you're game!

Closing this for now due to inactivity, but I'd be happy to revisit this if you're game!
witten closed this pull request 2024-03-12 04:14:13 +00:00

Pull request closed

Sign in to join this conversation.
No reviewers
No Milestone
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#695
No description provided.