Aakcht commented on code in PR #29347:
URL: https://github.com/apache/airflow/pull/29347#discussion_r1111214874
##########
airflow/providers/ssh/hooks/ssh.py:
##########
@@ -173,6 +182,14 @@ def __init__(
if "conn_timeout" in extra_options and self.conn_timeout is
None:
self.conn_timeout = int(extra_options["conn_timeout"])
+ if self.cmd_timeout is None:
+ if "cmd_timeout" in extra_options:
+ self.cmd_timeout = (
+ int(extra_options["cmd_timeout"]) if
extra_options["cmd_timeout"] else None
+ )
+ else:
+ self.cmd_timeout = CMD_TIMEOUT
+
Review Comment:
Hi, @Taragolis ! Good point about inconsistency between direct hook
parameters and connection parameters - I changed it to `negative value means
no timeout` (and now it's possible to set it both in the hook and in the
connection).
However I'm not sure I agree with NOTSET approach. It will (somewhat) bring
back the issue with inconsistency between hook/connection parameters when you
try to turn off cmd_timeout. Because in this case:
- To pass "no timeout" via hook parameters you should pass `None` as a
hook argument.
- To pass "no timeout" via connection parameters you should specify
`null` in connection extra: `{"cmd_timeout": null}`. (It's possible to use
empty string or negative values instead of null here, but I feel like it'll be
even more inconsistent with hook parameters).
I fill like specifying a parameter in connection extra and setting it to
null is a bit counterintuitive. If you think it's ok, I'll change it to NOTSET
approach, but I feel like negative values both in hook and connection for `no
timeout` would be a little more clear.
The unittests are already present at
https://github.com/apache/airflow/pull/29347/files#diff-8a4026fba1b53549c99ae768b6308aefddee93e54f3c8105aa454a15ee7cc075R824
.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
