srchilukoori commented on PR #65480: URL: https://github.com/apache/airflow/pull/65480#issuecomment-4305161763
Good progress — all 5 original blockers are resolved. A few remaining issues are likely causing the CI failures, plus two consistency nits. --- **[Blocker] Inner imports still in `_do_transfer`** — `triggers/sftp.py` The circular import is gone now that `SFTPOperation` lives in `constants.py`, so there is no reason for `os`, `Path`, `SFTPOperation`, and `SFTPHook` to be imported inside the method. Static checks (`ruff`/import-order) will fail on this pattern. Move them to the top of the file: ```python import os from pathlib import Path from airflow.providers.sftp.constants import SFTPOperation from airflow.providers.sftp.hooks.sftp import SFTPHook ``` --- **[Blocker] Inner imports still in new test methods** — `test_sftp.py` `test_sftp_operator_defers_when_deferrable_true` has an inner `from airflow.providers.sftp.triggers.sftp import SFTPOperatorTrigger` — `SFTPOperatorTrigger` is already imported at the top of the file. All three `TestSFTPOperatorTrigger` methods (`test_serialize_roundtrip`, `test_run_success`, `test_run_error`) import `SFTPOperatorTrigger`, `asyncio`, and `unittest.mock.patch` inside the method body. Move all of these to the top of the file with the existing imports. --- **[Fix] `remote_host or ""` passes empty string to SFTPHook** — `triggers/sftp.py` ```python # current — wrong: overrides the connection's host with "" when remote_host is None sftp_hook = SFTPHook(ssh_conn_id=self.ssh_conn_id, remote_host=self.remote_host or "") # fix — pass None so the hook uses the connection's host sftp_hook = SFTPHook(ssh_conn_id=self.ssh_conn_id, remote_host=self.remote_host) ``` --- **[Nit] Use `SFTPOperation.PUT` as default instead of raw string** — `triggers/sftp.py` line ~170 ```python # current operation: str = "put", # preferred — consistent with SFTPOperator operation: str = SFTPOperation.PUT, ``` --- Once the inner imports are moved to module level the static checks should pass. Run `prek run --from-ref main --stage pre-commit` locally before pushing to catch any remaining lint issues. -- 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]
