Various fixes around ascp file source#21372
Conversation
- Add an ascp handler to the frontend, so users can paste ascp:// urls - Add ssh key passphrase config option, without this downloads hang waiting for password - Bypass is_dir / is_file checks, they'll always fail. We assume files for now. - Implement top-level get_file. `_get_file` would require implementing `get_info` which doesn't really make sense for this file source.
| log.debug(f"Executing ascp command (key path hidden): {self._sanitize_cmd_for_log(cmd)}") | ||
|
|
||
| env = os.environ.copy() | ||
| env["ASPERA_SCP_PASS"] = self.ssh_key_passphrase |
There was a problem hiding this comment.
Is this actually required in general though or just for this service? This seems wrong to me in general.
There was a problem hiding this comment.
wrong in which way ? config file or env var are the only ways that this can be set afaict, https://www.ibm.com/docs/en/ahte/4.3.0?topic=line-ascp-command-reference
There was a problem hiding this comment.
Ah, you mean is this only required for ENA ? I think this depends on which server you're connecting to ... i suppose we could make it optional again ?
| @@ -660,7 +669,7 @@ def test_ssh_key_as_content(self): | |||
| with patch("os.fdopen"): | |||
| with patch("os.chmod"): | |||
There was a problem hiding this comment.
I did not see these unit tests when I reviewed the other PR 😭 - this is on me. I would remove basically most of them - they aren't testing integration at all - they have created a parallel set of mocks to mock everything interesting the plugin is doing right - this hurts maintenance not helps it? Maybe there is utility in the testing like say "test_missing_ascp_binary" but these heavily mocked out things are not something we want in the code base IMO.
There was a problem hiding this comment.
yep, i agree. I think the only interesting thing we can do is make sure we build the right command line
|
Thanks for the quick review @jmchilton! |
2 participants
_get_filewould require implementingget_infowhich doesn't really make sense for this file source.Deployments will likely require a layout that contains
How to test the changes?
(Select all options that apply)
License