extension: proxmox: rewrite in python3 - #13832
Draft
resmo wants to merge 5 commits into
Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR rewrites the Proxmox extension implementation from a Bash script to a Python 3 script, aiming to improve maintainability and reduce reliance on shell tooling.
Changes:
- Removed the legacy
proxmox.shBash-based extension implementation. - Added a new
proxmox.pyPython-based implementation covering lifecycle actions (prepare/create/start/stop/reboot/delete/status/statuses), console retrieval, and snapshot operations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| extensions/Proxmox/proxmox.sh | Removed the previous Bash implementation of the Proxmox extension. |
| extensions/Proxmox/proxmox.py | Added a Python implementation for Proxmox extension operations, including VM lifecycle, console access, and snapshot management. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13832 +/- ##
============================================
- Coverage 19.64% 19.64% -0.01%
+ Complexity 19790 19787 -3
============================================
Files 6368 6368
Lines 574889 574889
Branches 70353 70353
============================================
- Hits 112962 112952 -10
- Misses 449656 449666 +10
Partials 12271 12271
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
extensions/Proxmox/proxmox.py:107
- This script uses multiple Python 3.9/3.10+ language features (e.g.,
dict[str, Any]/list[str],str.removeprefix,@dataclass(slots=True), PEP 604 unions likeint | None, andzip(..., strict=False)). On distros where/usr/bin/python3is < 3.10, the script will fail to even start due to syntax errors. Either refactor to a Python version that matches CloudStack’s supported platforms (e.g., 3.8+) or explicitly document/enforce a minimum Python version for this extension in packaging/runtime checks.
@dataclass(slots=True)
class ProxmoxSettings:
url: str
user: str
token: str
secret: str
extensions/Proxmox/proxmox.py:88
_normalize_url()keeps any port present in the configured URL, butcall_api()always appends:8006. If the input is already likehttps://pve.example:8006, requests becomehttps://pve.example:8006:8006/...and will fail. Normalize the URL down toscheme://host(dropping any provided port/path) socall_api()can safely append the Proxmox API port exactly once.
def _normalize_url(url: str) -> str:
url = url.strip()
if not url.startswith(("http://", "https://")):
url = "https://" + url
return url.rstrip("/")
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
Suppressed comments (5)
extensions/Proxmox/proxmox.sh:20
- The wrapper executes proxmox.py directly, which requires the file to be marked executable and to have a working env-based shebang resolution; otherwise it can fail with 'Permission denied' or 'Exec format error'. More robust approach (mandatory): invoke it via python3 explicitly (e.g., python3 "$SCRIPT_PATH/proxmox.py" "$@").
SCRIPT_PATH="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
"$SCRIPT_PATH/proxmox.py" "$@"
extensions/Proxmox/proxmox.py:43
- fail() raises SystemExit and is called from within ProxmoxManager methods (e.g., parse_json/create validations). This makes the class harder to unit test/reuse and mixes process-exit behavior into lower-level logic. Recommended fix (mandatory): have ProxmoxManager raise ProxmoxError (or a validation exception) and keep SystemExit/printing centralized in main().
def fail(message: str) -> None:
print(json.dumps({"status": "error", "error": message}))
raise SystemExit(1)
extensions/Proxmox/proxmox.py:147
- fail() raises SystemExit and is called from within ProxmoxManager methods (e.g., parse_json/create validations). This makes the class harder to unit test/reuse and mixes process-exit behavior into lower-level logic. Recommended fix (mandatory): have ProxmoxManager raise ProxmoxError (or a validation exception) and keep SystemExit/printing centralized in main().
def parse_json(self) -> ProxmoxSettings:
try:
payload = json.loads(Path(self.config_path).read_text(encoding="utf-8"))
except FileNotFoundError:
fail(f"JSON file not found: {self.config_path}")
extensions/Proxmox/proxmox.py:655
- The usage message is missing the optional wait_time argument that the script actually accepts (sys.argv[3]). Consider updating it to reflect the real CLI signature so operator errors are easier to diagnose.
if len(sys.argv) < 3:
fail("Usage: proxmox.py <operation> '<json-file-path>'")
extensions/Proxmox/proxmox.py:141
- ssl._create_unverified_context() is a private API. If you want to keep the TLS-bypass feature, consider constructing an SSLContext via public APIs (e.g., create_default_context + verify_mode changes) to avoid relying on underscored implementation details.
self._ssl_context = (
ssl.create_default_context()
if self.data.verify_tls_certificate
else ssl._create_unverified_context() # noqa: SLF001 - intentional for admin-controlled TLS bypass
)
| return _string(value, "-") | ||
|
|
||
|
|
||
| @dataclass(slots=True) |
Comment on lines
+130
to
+131
| class ProxmoxManager: | ||
| def __init__(self, config_path: str, wait_time: int | None = None): |
| nic_map = _mapping(nic) | ||
| mac_addresses.append(_string(nic_map.get("mac"))) | ||
| vlan = _string(nic_map.get("broadcastUri")) | ||
| vlans.append(vlan.removeprefix("vlan://")) |
Comment on lines
+421
to
+423
| for idx, (mac, vlan) in enumerate( | ||
| zip(self.data.mac_addresses, self.data.vlans, strict=False) | ||
| ): |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
extensions/Proxmox/proxmox.sh:20
- proxmox.sh executes proxmox.py directly, which depends on the executable bit being set on the Python file. Invoking it via python3 (and using exec) makes the wrapper robust and preserves the child exit code/signals consistently.
SCRIPT_PATH="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
"$SCRIPT_PATH/proxmox.py" "$@"
extensions/Proxmox/proxmox.py:334
- vm_not_present() treats any "not found"-like error as absence. If vmid is missing/empty, the generated URL contains a double-slash (".../qemu//status/current"), which is very likely to 404 and be interpreted as "VM not present". That makes stop/delete incorrectly return success when the request is actually invalid. Fail fast when vmid is missing.
def vm_not_present(self) -> bool:
try:
self.call_api(
"GET", f"/nodes/{self.data.node}/qemu/{self.data.vmid}/status/current"
)
Comment on lines
+83
to
+87
| def _normalize_url(url: str) -> str: | ||
| url = url.strip() | ||
| if not url.startswith(("http://", "https://")): | ||
| url = "https://" + url | ||
| return url.rstrip("/") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
this PR rewrites the proxmox extension from shell to modern python3 for better maintainability.
Due to the fact, that ubuntu 22.04 already used py3.10 and all other have newer python3 or the possibility install a later version (rhel9), I'd like to keep py3.10 syntax.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?