RF: Make SSHConnection.open() errors actionable for callers() #3776
Conversation
Changes the return value semantics (see inside) and raises `CommandError` when no ControlMaster can be started. This enables to act on error on the caller side without causing spurious warnings. It turns the output given in dataladgh-3450: ``` % datalad publish --to bareremote --transfer-data all mih@datalad-test: Permission denied (publickey,password). [WARNING] Failed to run cmd ['ssh', '-fN', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=15m', '-o', 'ControlPath=/home/mih/.cache/datalad/sockets/c5820c82', 'datalad-test']. Exit code=255 | stdout: None | stderr: None [ERROR ] Cmd('/usr/lib/git-annex.linux/git') failed due to: exit code(128) | cmdline: /usr/lib/git-annex.linux/git fetch --progress -v bareremote | stderr: 'fatal: Could not read from remote repository. | | Please make sure you have the correct access rights | and the repository exists.' [cmd.py:wait:412] (GitCommandError) ``` into a leaning, easier to comprehend ``` % datalad publish --to bareremote --transfer-data all mih@datalad-test: Permission denied (publickey,password). CommandError: command '['ssh', '-fN', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=15m', '-o', 'ControlPath=/home/mih/.cache/datalad/sockets/c5820c82', 'datalad-test']' failed with exitcode 255 Failed to open SSH connection (could not start ControlMaster process) ``` and thereby fixes dataladgh-3450
|
appveyor failure is same old stalled |
|
Unrelated to the change: I wonder why ssh reports that it tried password whenever it seems it didn't |
| if exit_code != 0: | ||
| raise CommandError( | ||
| str(cmd), | ||
| 'Failed to open SSH connection (could not start ControlMaster process)', |
There was a problem hiding this comment.
What about defining a dedicated exception subclassed from CommandError? With adding a debug level message with details and custom str for exception we could avoid dumping into user's face that long cryptic invocation line too
There was a problem hiding this comment.
Considering how CommandError makes it too often into the user's face, we might want to change CommandError instead, anyway. We could change its __str__ to only consider self.msg and make the former __str__ a details property, that can be accessed by debug level log.
There was a problem hiding this comment.
I RF'ed to use a dedicated exception. I agree that further optimization along the suggested lines should be done in a generic fashion and in a separate PR.
| if exit_code != 0: | ||
| raise CommandError( | ||
| str(cmd), | ||
| 'Failed to open SSH connection (could not start ControlMaster process)', |
There was a problem hiding this comment.
Considering how CommandError makes it too often into the user's face, we might want to change CommandError instead, anyway. We could change its __str__ to only consider self.msg and make the former __str__ a details property, that can be accessed by debug level log.
Codecov Report
@@ Coverage Diff @@
## master #3776 +/- ##
===========================================
+ Coverage 46.62% 80.79% +34.17%
===========================================
Files 270 273 +3
Lines 35964 35969 +5
===========================================
+ Hits 16769 29062 +12293
+ Misses 19195 6907 -12288
Continue to review full report at Codecov.
|
Changes the return value semantics (see inside) and raises
CommandErrorwhen no ControlMaster can be started. This enables to act on error on the caller side without causing spuriouswarnings.
It turns the output given in gh-3450:
into a leaner, easier to comprehend
and thereby fixes gh-3450
See #3450 (comment) for more info on the wider implications and future work.
@adswa when this is merged, it will simplify the error captured at https://github.com/datalad-handbook/book/blame/master/docs/basics/101-135-help.rst#L166