[GitHub] [airflow] Aakcht commented on a diff in pull request #29347: SSH Provider: Add cmd_timeout to ssh connection extra

["Aakcht (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org