Ensure that BatchedCommandError is raised if the subprocesses of BatchedCommand fails or raises a CommandError#7068
Conversation
This commit modifies the behavior of `BatchedCommand` to raise an exception of the underlying command exits with a non-zero return code. Previously any exceptions were caught within `BatchedCommand` and logged.
This commit introduces code that will catch all CommandErrors from subprocesses and raise an appropriate BatchedCommandError that contains in addition to all the CommandError fields also the last successfully executed request.
This commit adds code to handle the situation where git-annex in batch-mode does not signal an error, here: an unknown remote, by returning a JSON object with the error description, but by exiting with an error code. This has become necessary after `BatchedCommand` was modified so that it reports errors of subprocesses to its callers. Remark: The code added in this commit only fixes `AnnexRepo.is_available()`. It maps all error exits of git-annex to "non-availability", which might be too unspecific. There also might be other places in the code, where process-exits should be caught and mapped onto method-results.
Codecov ReportBase: 74.76% // Head: 75.96% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## maint #7068 +/- ##
==========================================
+ Coverage 74.76% 75.96% +1.19%
==========================================
Files 354 355 +1
Lines 58945 59105 +160
Branches 6310 6318 +8
==========================================
+ Hits 44072 44897 +825
+ Misses 14858 14194 -664
+ Partials 15 14 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
bpoldrack
left a comment
bpoldrack
left a comment
There was a problem hiding this comment.
From looking at the code, I have one comment, otherwise looks right to me.
yarikoptic
added
the
CHANGELOG-missing
github-actions
Bot
removed
the
CHANGELOG-missing
yarikoptic
left a comment
yarikoptic
left a comment
There was a problem hiding this comment.
I agree with @bpoldrack recommendation on raising with preserving "origin". I also have some odd gut feeling that we might see some odd fails in parallel executions, but it might be only for good since we might really need to look into making those batched commands (more) thread-safe. But overall - I approve at this stage of our datalad development ;-) thanks @christian-monch
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
This commit changes the name of `BatchedCommandError.request` to `BatchedCommandError.last_processed_request`, and adds documentation for it.
Raising BatchedCommandErorrs `from` the causing CommandError in order to keep the original context. Thx @bpoldrack
|
scratch that -- we already added a stub, but you might like to adjust it : https://github.com/datalad/datalad/pull/7068/files#diff-602c7a1f3f833aef726d071f72e97d752a35433b7792c2b76f038e5f57644b91R3 |
|
PR released in |
4 participants
Fixes #6834
This PR ensures that
BatchedCommandraises aBatchedCommandErrorif the subprocesses ofBatchedCommandexits with a non-zero value or raises aCommandError.Changelog
🐛 Bug Fixes
BatchedCommand-tests to actual behavior. Fixes test_command_fail_1 fails on debian testing py 3.9 (only) #6834💫 Enhancements and new features
BatchedCommandare signaled to the caller viaBatchedCommandError-exception