potiuk commented on PR #68298:
URL: https://github.com/apache/airflow/pull/68298#issuecomment-5270102667
Closing this — not because the feature isn't wanted, but because the branch
has drifted into a state that can't be reviewed or merged, and it has been
through five rounds of review without the requested changes landing.
What the current diff does, independent of the feature itself:
1. **Removes an existing security protection.** `retrieve_directory` and
`retrieve_directory_concurrently` in `hooks/sftp.py` lose their
`_validate_within_directory(...)` calls, so remote directory entries containing
`..` can write outside the destination again. A similar helper is re-added
further down the file, but nothing calls it.
2. **Guts `providers/sftp/pyproject.toml`.** The version is rolled back from
`6.0.1` to `5.8.2` and 71 lines are deleted, including
`[project.entry-points."apache_airflow_provider"]`, `[tool.flit.module]` and
`[tool.flit.sdist]`. Without those the provider does not build and is not
discovered by Airflow.
3. **Hand-edits generated documentation.** `docs/index.rst` is reverted to
5.8.2, including the release download table. That file is generated, not edited.
4. **Adds a newsfragment under `providers/`.** Provider changelogs are
regenerated from `git log`; newsfragments there are never consumed.
5. **Changes dependencies without justification.** A new `asgiref`
requirement (pinned `>=3.11.1` in `pyproject.toml` but documented as
`>=3.8.1`), and a `paramiko<5.0.0` upper cap carrying a `# use next version`
comment.
6. **Bundles a rewrite of the existing synchronous transfer path** into the
same PR as the new deferrable option, which puts current users' code path at
risk for a change they did not ask for.
Most of this looks like the result of repeatedly merging `main` into the
branch and resolving conflicts in the wrong direction, rather than anything
deliberate. It is also why the earlier reviewers kept reporting that requested
changes were not present: the summary comments state that everything is
resolved, while the diff shows reverted files.
Deferrable mode for `SFTPOperator` is a reasonable thing to want, and you
are welcome to reopen or to open a fresh PR. If you do, please:
- branch from current `main` and **rebase** rather than merge, so the diff
contains only your own changes;
- keep the PR to the deferrable feature alone — no version bumps, no
generated-doc edits, no newsfragment, no unrelated refactor;
- verify the final diff on the *Files changed* tab before asking for review
— that is the authoritative view of what would be merged;
- default `deferrable` to `conf.getboolean("operators",
"default_deferrable", fallback=False)`, as other deferrable operators do.
The quality bar is described in
[contributing-docs/05_pull_requests.rst](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst)
— worth a read before the next attempt, particularly the sections on keeping
PRs focused and on verifying your own diff.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
