potiuk commented on PR #69885:
URL: https://github.com/apache/airflow/pull/69885#issuecomment-5146253585
Heads-up: #70091 merged a few minutes ago and has put this into conflict —
apologies, I reviewed them in sequence and the overlap only became visible once
the first landed.
Both PRs modify `build_posix_cleanup_command` in
`providers/ssh/src/airflow/providers/ssh/utils/remote_job.py`: #70091 changed
its body to `return f"rm -rf {shlex.quote(job_dir)}"`, while this PR changes
its signature to take `base_dir`. The two edits are adjacent, and this branch
is ~441 commits behind `main`, so a rebase is needed.
The resolution should be mechanical — keep your signature change and
`_validate_job_dir(job_dir, "posix", base_dir)`, and take
`shlex.quote(job_dir)` from `main` for the `rm -rf` line. Same pattern for the
kill/log-tail builders if you touch them.
On the fix itself, which I did review: the diagnosis is right.
`_validate_job_dir` compares against `POSIX_DEFAULT_BASE_DIR` unconditionally,
so any operator with a custom `remote_base_dir` fails cleanup with "Invalid job
directory" even though the path is legitimate. Passing the operator's own base
dir through is the correct fix, and defaulting to the OS constant keeps
existing behaviour intact.
No review comments beyond the rebase — happy to look again once it's green.
---
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]
