InstalledCode: Fix bug in validate_filepath_executable for SSH
#5787
Conversation
|
This bug was discovered in today's hackathon. It only occurs for a computer with |
sphuber
added
the
pr/blocked
sphuber
force-pushed
the
fix/validate-filepath-executable
branch
from
8ed6f71 to
0186c3f
Compare
sphuber
removed
the
pr/blocked
sphuber
force-pushed
the
fix/validate-filepath-executable
branch
2 times, most recently
from
511ecf7 to
4275455
Compare
| @@ -72,11 +72,11 @@ def validate_filepath_executable(self): | |||
| try: | |||
| with override_log_level(): # Temporarily suppress noisy logging | |||
| with self.computer.get_transport() as transport: | |||
| file_exists = transport.isfile(self.filepath_executable) | |||
| except Exception: # pylint: disable=broad-except | |||
| file_exists = transport.isfile(str(self.filepath_executable)) | |||
There was a problem hiding this comment.
If local returned a str here and ssh a Path, should we then not also aim to harmonize the behavior of the filepath_executable property?
There was a problem hiding this comment.
They both returned a pathlib.Path, the problem is that LocalTransport.isfile accepts a pathlib.Path, whereas SshTransport only accepts strings. We should probably update the transport interfaces to add support for pathlib.Path objects.
There was a problem hiding this comment.
Thanks - yes, that would make sense. Up to you whether you want to do it here or not
There was a problem hiding this comment.
I think it is better to do the whole interface in one go. Have opened a feature request for it #5797
If the code was attached to a `Computer` with `core.ssh` transport, the `validate_filepath_executable` would always return that the computer could not be connected to. This is because the actual exception that was caught does not come from `Computer.get_transport` but is a `TypeError` raised by `transport.isfile` since it receives a `pathlib.Path` were it expects a `str`. The problem went undetected since it only tested against a computer with `core.local` transport. A new test is added to also test against a computer with `core.ssh` transport`.
sphuber
force-pushed
the
fix/validate-filepath-executable
branch
from
4275455 to
6af9422
Compare
Fixes #5795
If the code was attached to a
Computerwithcore.sshtransport, thevalidate_filepath_executablewould always return that the computercould not be connected to. This is because the actual exception that was
caught does not come from
Computer.get_transportbut is aTypeErrorraised by
transport.isfilesince it receives apathlib.Pathwere itexpects a
str.The problem went undetected since it only tested against a computer with
core.localtransport. A new test is added to also test against acomputer with
core.sshtransport`.