WIP: Adding "actions" runtime variable #657 #695
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "(deleted):feat/adding_action_names_to_hook_context"
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?
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:
TODO:
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.
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
:I wasn't sure I needed to modify this existing test :)
I'll amend my commit then.
@ -65,2 +65,4 @@
variables you can use here:
* `actions`
<span class="minilink minilink-addedin">New in version TODO:</span>:
If this is done soon, it'll probably be part of 1.7.13!
Done :)
@ -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
This is good, but I'd recommend changing the tense to something like:
That's because hooks can run before or after the actions.
I get it: wording was changed.
You're correct. My apologies! I just submitted it now.
You can set it to 1.7.13. (I can always change it if need be.)
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!e8bcae3916
tof17fde399e
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
:I don't understand why it's complaining about
log_file
:'(test_run_actions_adds_all_sorted_action_names_to_hook_context
:For this one I guess it's because I've choosed the
check
action as an example for my test?I apologize for the delay here.. I'm not sure I received an email notification for your most recent comment!
My guess is that's because this line:
... needs to look more like this:
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.Yeah, my guess is that
checks.repository_enabled_for_checks()
is returningFalse
, and therefore your mockedrun_check()
isn't getting called. So one thing you could do to solve this is to mock outchecks.repository_enabled_for_checks()
as well, forcing it to returnTrue
. Or you could just not use thecheck
action in your test.Just checking in.. Do you still plan to work on this? Thanks!
Closing this for now due to inactivity, but I'd be happy to revisit this if you're game!
Pull request closed