connectors.util: honour _temp_dir in askpass helpers#1666
connectors.util: honour _temp_dir in askpass helpers#1666wowi42 wants to merge 6 commits intopyinfra-dev:3.xfrom
Conversation
Fizzadar
left a comment
Fizzadar
left a comment
There was a problem hiding this comment.
Looks good, two things to fix.
| value = self.data.get("temp_dir") | ||
| if value: | ||
| return value | ||
| value = self.data.get("_temp_dir") |
There was a problem hiding this comment.
This shouldn't be required - we already apply global arguments in host data (so _temp_dir in host data will apply to all operations as an argument): https://github.com/pyinfra-dev/pyinfra/blob/3.x/src/pyinfra/api/arguments.py#L405
| return _ensure_askpass_set_for_host(host, "sudo_askpass_path", SUDO_ASKPASS_ENV_VAR) | ||
| def _ensure_sudo_askpass_set_for_host(host: "Host", temp_dir: Optional[str] = None): | ||
| return _ensure_askpass_set_for_host( | ||
| host, "sudo_askpass_path", SUDO_ASKPASS_ENV_VAR, temp_dir=temp_dir |
There was a problem hiding this comment.
We should add the path to the key here or we'll incorrectly cache the wrong askpass path if the value is changed (very unlikely, but worth guarding against).
| host, "sudo_askpass_path", SUDO_ASKPASS_ENV_VAR, temp_dir=temp_dir | |
| host, f"sudo_askpass_path:{temp_dir}", SUDO_ASKPASS_ENV_VAR, temp_dir=temp_dir |
Closes pyinfra-dev#1623. The SUDO_ASKPASS / SU_ASKPASS helper script is now placed under the same directory the caller requested, fixing the case where an operation-level _temp_dir (or a per-host default cascaded through the _temp_dir global argument on host.data) was respected for file ops but ignored by the internal askpass helpers. Resolution order (unchanged outside the askpass path): 1. Operation-level _temp_dir (explicit caller) 2. config.TEMP_DIR / host.data._temp_dir via the existing global-argument cascade 3. TmpDir fact (_get_temp_directory only) 4. config.DEFAULT_TEMP_DIR Changes: - _ensure_askpass_set_for_host accepts a temp_dir override and tracks the resolved directory next to the cached path; if a later call resolves a different temp_dir the cached path is invalidated so the script gets regenerated under the correct directory. - make_unix_command_for_host threads _temp_dir through to the askpass helpers so the op-level override takes effect. Tests cover the askpass path for default, config, and op-level temp directories, cache invalidation on temp_dir change, and the full path through make_unix_command_for_host. Tests use unique hostnames to avoid sharing the process-global memoize cache on _get_temp_directory.
wowi42
force-pushed
the
feature/per-host-temp-dir
branch
from
dd7d9b4 to
8798dd1
Compare
|
Thanks for the review, both addressed and rebased on Dropped the For the cache key, went with storing the resolved PR description is now a bit stale, I'll clean it up next. |
wowi42
changed the title
Fizzadar
left a comment
Fizzadar
left a comment
There was a problem hiding this comment.
This seems overly complicated, could we not just add the tmp path to the key at call time if set? so we end up with keys like:
sudo_askpass_path__/tmpsudo_askpass_path__/x
and so no? No need for the extra field or checks then (I think!)
Per-host askpass paths are now cached under
``{sudo,su}_askpass_path__<temp_dir>`` instead of a bare key paired
with a separate temp_dir tracker field. Each (host, temp_dir) pair
gets its own cache slot, so a different ``_temp_dir`` on a later call
just misses the cache and creates a fresh script under the right
directory.
- ``_ensure_askpass_set_for_host`` (and its sudo/su wrappers) now
return the resolved path. ``make_unix_command_for_host`` uses the
return value directly instead of reading ``connector_data``.
- ``remove_any_sudo_askpass_file`` walks every cached variant so
``host.disconnect()`` cleans up askpass scripts under any temp dir
the host produced during the run.
- New ``clear_askpass_cache`` helper drops every cached path without
touching the remote, used by ``server.reboot`` around the reboot
window where the previous connection (and its temp dir) is gone.
|
Good call, that is much cleaner. Pushed efc8158 with the simpler scheme:
Tests updated to seed the temp_dir-suffixed keys and cover both the multi-variant cleanup path and same-temp_dir cache reuse. |
3 participants
Closes #1623.
Problem
server.rebootand other ops respected_temp_dir(operation kwarg, or the per-host default cascaded fromhost.data._temp_dirvia the global-argument system) for file placement, but the internalSUDO_ASKPASS/SU_ASKPASShelper script was still dropped intoconfig.TEMP_DIR(usually/tmp). On hosts where/tmpis unusable (noexec, restricted tmpfs, TrueNAS, etc.) the askpass helper would fail even when the user had configured a working temp directory.Fix
_ensure_askpass_set_for_hostnow takes atemp_diroverride and uses it for the mkstemp template.make_unix_command_for_hostthreads the operation-level_temp_dirthrough to the askpass helpers.temp_diris stored alongside the cached path (<key>_temp_dir); if a later call resolves a differenttemp_dirthe cache is invalidated so the script gets regenerated under the correct directory. Callers that readhost.connector_data["sudo_askpass_path"]directly don't need to change.Per-host defaults don't need any new code path: setting
host.data._temp_diralready cascades as the default for the_temp_dirglobal argument on every op, which is what gets passed down.Test plan
config.TEMP_DIR, and op-level_temp_dir.temp_dirchanges between calls.make_unix_command_for_hostwith_sudo=True+_temp_dir.uv run pytest(1613 passed), ruff / ruff format / mypy /scripts/lint_arguments_sync.pyclean.3.x)scripts/dev-test.sh)scripts/dev-lint.sh)