[PR #16] [MERGED] Fix review findings: locale/Alpine bugs, de-duplication, and regression tests #16

New issue

Closed

opened 2026-06-03 17:01:12 +08:00 by ivenator1 · 0 comments

ivenator1 commented

2026-06-03 17:01:12 +08:00

Owner

📋 Pull Request Information

Original PR: https://github.com/ivenator1/proxmox-management/pull/16
Author: @ivenator1
Created: 6/3/2026
Status: ✅ Merged
Merged: 6/3/2026
Merged by: @ivenator1

Base: testing ← Head: claude/testing-branch-pull-tEqHP

📝 Commits (2)

aad0c8d Fix review findings: locale pinning, Alpine shell, dedup, and regression tests
774ff47 Lint tests/ with ruff and widen the ruff CI job to cover tests

📊 Changes

30 files changed (+526 additions, -331 deletions)

View changed files

📝 .github/workflows/ci.yml (+10 -0)
📝 proxmox_fleet/driver.py (+42 -78)
➕ proxmox_fleet/flows/_pkg.py (+60 -0)
📝 proxmox_fleet/flows/custom.py (+7 -5)
📝 proxmox_fleet/flows/lxc.py (+29 -24)
📝 proxmox_fleet/flows/node.py (+6 -12)
📝 proxmox_fleet/flows/remote.py (+5 -40)
📝 proxmox_fleet/flows/vm.py (+5 -40)
📝 proxmox_fleet/inventory.py (+32 -101)
📝 proxmox_fleet/models/settings.py (+13 -1)
📝 proxmox_fleet/models/state.py (+3 -2)
📝 proxmox_fleet/status.py (+3 -3)
📝 proxmox_fleet/steps.py (+8 -2)
📝 pyproject.toml (+11 -0)
📝 tests/unit/test_deps.py (+0 -1)
📝 tests/unit/test_flow_custom.py (+0 -1)
📝 tests/unit/test_flow_lxc.py (+49 -4)
📝 tests/unit/test_flow_node.py (+1 -2)
📝 tests/unit/test_flow_remote.py (+1 -2)
📝 tests/unit/test_flow_vm.py (+1 -2)

...and 10 more files

📄 Description

Summary

Addresses the findings from the code review of proxmox_fleet/: four correctness bugs, heavy duplication across the flows, and a few dead-code/doc items — each backed by new regression tests, plus a ruff CI guard so they can't silently drift back.

Per direction, the "unfinished feature" scaffolding is retained (orphaned Ansible primitives, MaintenanceWindow, dead helper functions, and the dump_for_ansible/from_raw machinery which is live for molecule).

Correctness bugs

Alpine shell — flows/lxc.py _read_version now reads ~/.scriptname with the OS-appropriate shell (ash on Alpine, not a hardcoded bash that Alpine containers often lack).
Locale — LC_ALL=C is pinned on every apt/apk/dnf upgrade + dpkg-hash command, and window.py uses a fixed weekday list instead of locale-sensitive strftime('%a'). Without this, change detection reports "changed" on every run on a non-English host.
step.delay — steps.run_steps now honours each step's delay between failed retries (injectable sleep keeps tests fast).
Node host_vars — inventory.load_proxmox_nodes merges host_vars so a node whose ansible_host lives only there resolves to the IP (it becomes the snapshot api_host, which must be an IP), not the bare name.
Integer kuma-map keys — GlobalSettings coerces {101: 5} keys to str so natural integer vmids in vars.yml load instead of raising a pydantic string_type error on startup.

De-duplication

New flows/_pkg.py centralises detect_pkg_mgr / upgrade_cmd / kuma_healthy (previously copy-pasted across vm, remote, lxc, custom, and node).
inventory.py — a single _iter_section scanner backs all four loaders (~130 lines of copy-paste removed).
driver.py — one _fold_outcome replaces five near-identical fold helpers.

Cleanup & guard

Removed unused imports/locals (ruff F-clean) across package and tests.
Reworded docstrings that referred to the deleted fleet-update.yml.
Added a ruff CI job + [tool.ruff] config covering proxmox_fleet/ tests/ to guard against dead-code regressions.

Tests

+41 regression tests, new tests/unit/test_pkg.py, plus cases for Alpine/locale, step-delay timing, integer kuma keys, and node/vm/remote host_vars.
Full suite: 467 passed (was 426). ruff and mypy both clean.

https://claude.ai/code/session_01J7Xw8eqUZADBzKo6cqiWLy

Generated by Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/ivenator1/proxmox-management/pull/16 **Author:** [@ivenator1](https://github.com/ivenator1) **Created:** 6/3/2026 **Status:** ✅ Merged **Merged:** 6/3/2026 **Merged by:** [@ivenator1](https://github.com/ivenator1) **Base:** `testing` ← **Head:** `claude/testing-branch-pull-tEqHP` --- ### 📝 Commits (2) - [`aad0c8d`](https://github.com/ivenator1/proxmox-management/commit/aad0c8d30a12c59908c7457ca97ae517b1f862bf) Fix review findings: locale pinning, Alpine shell, dedup, and regression tests - [`774ff47`](https://github.com/ivenator1/proxmox-management/commit/774ff479c329eefee7f4beb7ca2401cf69290afc) Lint tests/ with ruff and widen the ruff CI job to cover tests ### 📊 Changes **30 files changed** (+526 additions, -331 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/ci.yml` (+10 -0) 📝 `proxmox_fleet/driver.py` (+42 -78) ➕ `proxmox_fleet/flows/_pkg.py` (+60 -0) 📝 `proxmox_fleet/flows/custom.py` (+7 -5) 📝 `proxmox_fleet/flows/lxc.py` (+29 -24) 📝 `proxmox_fleet/flows/node.py` (+6 -12) 📝 `proxmox_fleet/flows/remote.py` (+5 -40) 📝 `proxmox_fleet/flows/vm.py` (+5 -40) 📝 `proxmox_fleet/inventory.py` (+32 -101) 📝 `proxmox_fleet/models/settings.py` (+13 -1) 📝 `proxmox_fleet/models/state.py` (+3 -2) 📝 `proxmox_fleet/status.py` (+3 -3) 📝 `proxmox_fleet/steps.py` (+8 -2) 📝 `pyproject.toml` (+11 -0) 📝 `tests/unit/test_deps.py` (+0 -1) 📝 `tests/unit/test_flow_custom.py` (+0 -1) 📝 `tests/unit/test_flow_lxc.py` (+49 -4) 📝 `tests/unit/test_flow_node.py` (+1 -2) 📝 `tests/unit/test_flow_remote.py` (+1 -2) 📝 `tests/unit/test_flow_vm.py` (+1 -2) _...and 10 more files_ </details> ### 📄 Description ## Summary Addresses the findings from the code review of `proxmox_fleet/`: four correctness bugs, heavy duplication across the flows, and a few dead-code/doc items — each backed by new regression tests, plus a ruff CI guard so they can't silently drift back. Per direction, the "unfinished feature" scaffolding is **retained** (orphaned Ansible primitives, `MaintenanceWindow`, dead helper *functions*, and the `dump_for_ansible`/`from_raw` machinery which is live for molecule). ## Correctness bugs - **Alpine shell** — `flows/lxc.py` `_read_version` now reads `~/.scriptname` with the OS-appropriate shell (`ash` on Alpine, not a hardcoded `bash` that Alpine containers often lack). - **Locale** — `LC_ALL=C` is pinned on every apt/apk/dnf upgrade + dpkg-hash command, and `window.py` uses a fixed weekday list instead of locale-sensitive `strftime('%a')`. Without this, change detection reports "changed" on every run on a non-English host. - **`step.delay`** — `steps.run_steps` now honours each step's `delay` between failed retries (injectable sleep keeps tests fast). - **Node host_vars** — `inventory.load_proxmox_nodes` merges `host_vars` so a node whose `ansible_host` lives only there resolves to the IP (it becomes the snapshot `api_host`, which must be an IP), not the bare name. - **Integer kuma-map keys** — `GlobalSettings` coerces `{101: 5}` keys to `str` so natural integer vmids in `vars.yml` load instead of raising a pydantic `string_type` error on startup. ## De-duplication - New `flows/_pkg.py` centralises `detect_pkg_mgr` / `upgrade_cmd` / `kuma_healthy` (previously copy-pasted across vm, remote, lxc, custom, and node). - `inventory.py` — a single `_iter_section` scanner backs all four loaders (~130 lines of copy-paste removed). - `driver.py` — one `_fold_outcome` replaces five near-identical fold helpers. ## Cleanup & guard - Removed unused imports/locals (ruff F-clean) across package and tests. - Reworded docstrings that referred to the deleted `fleet-update.yml`. - Added a **ruff CI job** + `[tool.ruff]` config covering `proxmox_fleet/ tests/` to guard against dead-code regressions. ## Tests - **+41 regression tests**, new `tests/unit/test_pkg.py`, plus cases for Alpine/locale, step-delay timing, integer kuma keys, and node/vm/remote host_vars. - Full suite: **467 passed** (was 426). `ruff` and `mypy` both clean. https://claude.ai/code/session_01J7Xw8eqUZADBzKo6cqiWLy --- _Generated by [Claude Code](https://claude.ai/code/session_01J7Xw8eqUZADBzKo6cqiWLy)_ --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

ivenator1

2026-06-03 17:01:12 +08:00