jedcunningham commented on a change in pull request #17236:
URL: https://github.com/apache/airflow/pull/17236#discussion_r680117132
##########
File path: airflow/providers/ssh/hooks/ssh.py
##########
@@ -56,7 +58,9 @@ class SSHHook(BaseHook):
:type key_file: str
:param port: port of remote host to connect (Default is paramiko SSH_PORT)
:type port: int
- :param timeout: timeout for the attempt to connect to the remote_host.
+ :param conn_timeout: timeout for the attempt to connect to the remote_host.
+ :type conn_timeout: int
+ :param timeout: (Deprecated). timeout for the attempt to connect to the
remote_host.
Review comment:
Should we mention what param to use instead?
##########
File path: airflow/providers/ssh/operators/ssh.py
##########
@@ -43,7 +46,11 @@ class SSHOperator(BaseOperator):
:type remote_host: str
:param command: command to execute on remote host. (templated)
:type command: str
- :param timeout: timeout (in seconds) for executing the command. The
default is 10 seconds.
+ :param conn_timeout: timeout (in seconds) for maintaining the connection.
The default is 10 seconds.
+ :type conn_timeout: int
+ :param cmd_timeout: timeout (in seconds) for executing the command. The
default is 10 seconds.
+ :type cmd_timeout: int
+ :param timeout: (deprecated) timeout (in seconds) for executing the
command. The default is 10 seconds.
Review comment:
Here as well?
##########
File path: tests/providers/ssh/hooks/test_ssh.py
##########
@@ -475,6 +504,120 @@ def
test_ssh_connection_with_no_host_key_where_no_host_key_check_is_false(self,
assert ssh_client.return_value.connect.called is True
assert
ssh_client.return_value.get_host_keys.return_value.add.called is False
+ @mock.patch('airflow.providers.ssh.hooks.ssh.paramiko.SSHClient')
+ def test_ssh_connection_with_conn_timeout(self, ssh_mock):
+ hook = SSHHook(
+ remote_host='remote_host',
+ port='port',
+ username='username',
+ password='password',
+ conn_timeout=20,
+ key_file='fake.file',
+ )
+
+ with hook.get_conn():
+ ssh_mock.return_value.connect.assert_called_once_with(
+ hostname='remote_host',
+ username='username',
+ password='password',
+ key_filename='fake.file',
+ timeout=20,
+ compress=True,
+ port='port',
+ sock=None,
+ look_for_keys=True,
+ )
+
+ @mock.patch('airflow.providers.ssh.hooks.ssh.paramiko.SSHClient')
+ def test_ssh_connection_with_conn_timeout_and_timeout(self, ssh_mock):
+ hook = SSHHook(
+ remote_host='remote_host',
+ port='port',
+ username='username',
+ password='password',
+ timeout=10,
+ conn_timeout=20,
+ key_file='fake.file',
+ )
+
+ with hook.get_conn():
+ ssh_mock.return_value.connect.assert_called_once_with(
+ hostname='remote_host',
+ username='username',
+ password='password',
+ key_filename='fake.file',
+ timeout=20,
+ compress=True,
+ port='port',
+ sock=None,
+ look_for_keys=True,
+ )
+
+ @mock.patch('airflow.providers.ssh.hooks.ssh.paramiko.SSHClient')
+ def test_ssh_connection_with_timeout_extra(self, ssh_mock):
+ hook = SSHHook(
+ ssh_conn_id=self.CONN_SSH_WITH_TIMEOUT_EXTRA,
+ remote_host='remote_host',
+ port='port',
+ username='username',
+ timeout=10,
+ )
+
+ with hook.get_conn():
+ ssh_mock.return_value.connect.assert_called_once_with(
+ hostname='remote_host',
+ username='username',
+ timeout=20,
Review comment:
I realize the existing behavior was for extras to win, but that seems
backwards to me. I'd expect the param to win.
Maybe we leave `timeout` as-is, but invert it for `conn_timeout` since it is
new?
(`hostname` is a good example)
--
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]
