Lee-W commented on code in PR #52700:
URL: https://github.com/apache/airflow/pull/52700#discussion_r2271904604
##########
providers/sftp/src/airflow/providers/sftp/hooks/sftp.py:
##########
@@ -86,10 +110,12 @@ def __init__(
self,
ssh_conn_id: str | None = "sftp_default",
host_proxy_cmd: str | None = None,
+ use_managed_conn: bool = True,
Review Comment:
Should we default to `False` to persist the original behavior?
##########
providers/sftp/src/airflow/providers/sftp/sensors/sftp.py:
##########
@@ -135,6 +137,20 @@ def poke(self, context: Context) -> PokeReturnValue | bool:
else:
files_found = actual_files_present
+ return files_found
+
+ def poke(self, context: Context) -> PokeReturnValue | bool:
+ self.hook = SFTPHook(self.sftp_conn_id,
use_managed_conn=self.use_managed_conn)
+
+ self.log.info("Poking for %s, with pattern %s", self.path,
self.file_pattern)
+ files_found = []
Review Comment:
I think we probably don't need this line if the linter does not complain
##########
providers/sftp/tests/unit/sftp/sensors/test_sftp.py:
##########
@@ -97,6 +97,14 @@ def test_file_new_enough(self, sftp_hook_mock):
sftp_hook_mock.return_value.close_conn.assert_not_called()
assert output
+ @patch("airflow.providers.sftp.sensors.sftp.SFTPHook")
+ def test_only_creating_one_connection_with_unmanaged_conn(self,
sftp_hook_mock):
+ sftp_hook_mock.return_value.isfile.return_value = True
+ sftp_hook_mock.return_value.get_mod_time.return_value =
"19700101000000"
+ sftp_sensor = SFTPSensor(task_id="test", path="path/to/whatever/test",
use_managed_conn=False)
+ sftp_sensor.poke({})
+ sftp_hook_mock.return_value.get_managed_conn.assert_called_once_with()
Review Comment:
```suggestion
assert sftp_hook_mock.return_value.get_managed_conn.called is True
```
##########
providers/sftp/tests/unit/sftp/sensors/test_sftp.py:
##########
@@ -220,13 +229,11 @@ def
test_multiple_old_files_present_with_pattern_and_newer_than(self, sftp_hook_
)
context = {"ds": "1970-01-00"}
output = sftp_sensor.poke(context)
- sftp_hook_mock.return_value.get_mod_time.assert_has_calls(
- [
- mock.call("/path/to/file/text_file1.txt"),
- mock.call("/path/to/file/text_file2.txt"),
- mock.call("/path/to/file/text_file3.txt"),
- ]
- )
+ sftp_hook_mock.return_value.get_mod_time.assert_has_calls([
+ mock.call("/path/to/file/text_file1.txt"),
+ mock.call("/path/to/file/text_file2.txt"),
+ mock.call("/path/to/file/text_file3.txt"),
+ ])
Review Comment:
```suggestion
assert sftp_hook_mock.return_value.get_mod_time.mock_call([
mock.call("/path/to/file/text_file1.txt"),
mock.call("/path/to/file/text_file2.txt"),
mock.call("/path/to/file/text_file3.txt"),
])
```
##########
providers/sftp/src/airflow/providers/sftp/sensors/sftp.py:
##########
@@ -76,14 +77,15 @@ def __init__(
self.hook: SFTPHook | None = None
self.sftp_conn_id = sftp_conn_id
self.newer_than: datetime | str | None = newer_than
+ self.use_managed_conn = use_managed_conn
self.python_callable: Callable | None = python_callable
self.op_args = op_args or []
self.op_kwargs = op_kwargs or {}
self.deferrable = deferrable
- def poke(self, context: Context) -> PokeReturnValue | bool:
- self.hook = SFTPHook(self.sftp_conn_id)
- self.log.info("Poking for %s, with pattern %s", self.path,
self.file_pattern)
+ def _get_files(self) -> list[str]:
+ files_from_pattern = []
+ actual_files_present = []
Review Comment:
I think we probably don't need these 2 lines if the linter does not complai.n
##########
providers/sftp/tests/unit/sftp/sensors/test_sftp.py:
##########
@@ -193,9 +201,10 @@ def
test_multiple_files_present_with_pattern_and_newer_than(self, sftp_hook_mock
)
context = {"ds": "1970-01-00"}
output = sftp_sensor.poke(context)
- sftp_hook_mock.return_value.get_mod_time.assert_has_calls(
- [mock.call("/path/to/file/text_file1.txt"),
mock.call("/path/to/file/text_file2.txt")]
- )
+ sftp_hook_mock.return_value.get_mod_time.assert_has_calls([
+ mock.call("/path/to/file/text_file1.txt"),
+ mock.call("/path/to/file/text_file2.txt"),
+ ])
Review Comment:
```suggestion
assert sftp_hook_mock.return_value.get_mod_time.mock_calls == [
mock.call("/path/to/file/text_file1.txt"),
mock.call("/path/to/file/text_file2.txt"),
)
```
##########
providers/sftp/src/airflow/providers/sftp/sensors/sftp.py:
##########
@@ -67,6 +67,7 @@ def __init__(
python_callable: Callable | None = None,
op_args: list | None = None,
op_kwargs: dict[str, Any] | None = None,
+ use_managed_conn: bool = True,
Review Comment:
same here
--
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]
