From f1a3676723fd9838278fa6d5249ea58c3c2fd21e Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Tue, 24 Mar 2026 18:40:10 -0500 Subject: [PATCH] fix: address Copilot review feedback on PR #27 - Extract ParseLinks helper to PveCmdletBase for shared link parsing with WriteWarning on malformed entries (was duplicated in 3 cmdlets) - Fix GetClusterConfig to return data payload, not full API envelope - Fix OutputType on GetPveClusterConfigCmdlet to JObject - Fix link doc comments to use correct key format (link0..link7) - Add null-safe Properties hashtable conversion in HA rule cmdlets - Use case-insensitive Mode comparison in MovePveHaResourceCmdlet - Remove unused using directives Co-Authored-By: Claude Opus 4.6 (1M context) --- .../Services/ClusterConfigService.cs | 5 ++-- .../Cluster/AddPveClusterConfigNodeCmdlet.cs | 13 +--------- .../Cluster/AddPveClusterMemberCmdlet.cs | 13 +--------- .../Cluster/GetPveClusterConfigCmdlet.cs | 2 +- .../Cmdlets/Cluster/NewPveClusterCmdlet.cs | 13 +--------- .../Cmdlets/HA/MovePveHaResourceCmdlet.cs | 2 +- .../Cmdlets/HA/NewPveHaRuleCmdlet.cs | 6 ++++- .../Cmdlets/HA/SetPveHaRuleCmdlet.cs | 6 ++++- src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs | 25 +++++++++++++++++++ 9 files changed, 43 insertions(+), 42 deletions(-) diff --git a/src/PSProxmoxVE.Core/Services/ClusterConfigService.cs b/src/PSProxmoxVE.Core/Services/ClusterConfigService.cs index a135b68..8eaa47f 100644 --- a/src/PSProxmoxVE.Core/Services/ClusterConfigService.cs +++ b/src/PSProxmoxVE.Core/Services/ClusterConfigService.cs @@ -41,7 +41,8 @@ namespace PSProxmoxVE.Core.Services try { var response = client.GetAsync("cluster/config").GetAwaiter().GetResult(); - return JObject.Parse(response); + var data = JObject.Parse(response)["data"]; + return data as JObject ?? new JObject(); } finally { @@ -54,7 +55,7 @@ namespace PSProxmoxVE.Core.Services /// /// The authenticated PVE session. /// The name for the new cluster. - /// Optional Corosync link addresses (e.g., "0=10.0.0.1,1=10.0.1.1"). + /// Optional Corosync link addresses, using keys link0..link7 (e.g., "link0=10.0.0.1"). /// Optional node ID for this node. /// Optional number of quorum votes for this node. /// The UPID of the cluster creation task. diff --git a/src/PSProxmoxVE/Cmdlets/Cluster/AddPveClusterConfigNodeCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Cluster/AddPveClusterConfigNodeCmdlet.cs index 6d15a29..22160a6 100644 --- a/src/PSProxmoxVE/Cmdlets/Cluster/AddPveClusterConfigNodeCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Cluster/AddPveClusterConfigNodeCmdlet.cs @@ -1,4 +1,3 @@ -using System.Collections.Generic; using System.Management.Automation; using PSProxmoxVE.Core.Services; @@ -55,17 +54,7 @@ namespace PSProxmoxVE.Cmdlets.Cluster var session = GetSession(); var service = new ClusterConfigService(); - Dictionary? linkDict = null; - if (Links != null) - { - linkDict = new Dictionary(); - foreach (var link in Links) - { - var parts = link.Split(new[] { '=' }, 2); - if (parts.Length == 2) - linkDict[parts[0]] = parts[1]; - } - } + var linkDict = ParseLinks(Links); WriteVerbose($"Adding node '{Node}' to cluster configuration..."); var upid = service.AddConfigNode(session, Node, NewNodeIp, linkDict, NodeId, Votes, diff --git a/src/PSProxmoxVE/Cmdlets/Cluster/AddPveClusterMemberCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Cluster/AddPveClusterMemberCmdlet.cs index 820dbdb..2bc7b5b 100644 --- a/src/PSProxmoxVE/Cmdlets/Cluster/AddPveClusterMemberCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Cluster/AddPveClusterMemberCmdlet.cs @@ -1,5 +1,4 @@ using System; -using System.Collections.Generic; using System.Management.Automation; using System.Runtime.InteropServices; using System.Security; @@ -64,17 +63,7 @@ namespace PSProxmoxVE.Cmdlets.Cluster ptr = Marshal.SecureStringToGlobalAllocUnicode(Password); var plainPassword = Marshal.PtrToStringUni(ptr)!; - Dictionary? linkDict = null; - if (Links != null) - { - linkDict = new Dictionary(); - foreach (var link in Links) - { - var parts = link.Split(new[] { '=' }, 2); - if (parts.Length == 2) - linkDict[parts[0]] = parts[1]; - } - } + var linkDict = ParseLinks(Links); WriteVerbose($"Joining cluster via '{Hostname}'..."); var upid = service.JoinCluster(session, Hostname, Fingerprint, plainPassword, diff --git a/src/PSProxmoxVE/Cmdlets/Cluster/GetPveClusterConfigCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Cluster/GetPveClusterConfigCmdlet.cs index 88cf7cd..b5424a8 100644 --- a/src/PSProxmoxVE/Cmdlets/Cluster/GetPveClusterConfigCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Cluster/GetPveClusterConfigCmdlet.cs @@ -12,7 +12,7 @@ namespace PSProxmoxVE.Cmdlets.Cluster /// /// [Cmdlet(VerbsCommon.Get, "PveClusterConfig")] - [OutputType(typeof(PSObject))] + [OutputType(typeof(JObject))] public sealed class GetPveClusterConfigCmdlet : PveCmdletBase { protected override void ProcessRecord() diff --git a/src/PSProxmoxVE/Cmdlets/Cluster/NewPveClusterCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Cluster/NewPveClusterCmdlet.cs index 503c2f3..f1f5be6 100644 --- a/src/PSProxmoxVE/Cmdlets/Cluster/NewPveClusterCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Cluster/NewPveClusterCmdlet.cs @@ -1,4 +1,3 @@ -using System.Collections.Generic; using System.Management.Automation; using PSProxmoxVE.Core.Services; @@ -44,17 +43,7 @@ namespace PSProxmoxVE.Cmdlets.Cluster var session = GetSession(); var service = new ClusterConfigService(); - Dictionary? linkDict = null; - if (Links != null) - { - linkDict = new Dictionary(); - foreach (var link in Links) - { - var parts = link.Split(new[] { '=' }, 2); - if (parts.Length == 2) - linkDict[parts[0]] = parts[1]; - } - } + var linkDict = ParseLinks(Links); WriteVerbose($"Creating cluster '{ClusterName}'..."); var upid = service.CreateCluster(session, ClusterName, linkDict, NodeId, Votes); diff --git a/src/PSProxmoxVE/Cmdlets/HA/MovePveHaResourceCmdlet.cs b/src/PSProxmoxVE/Cmdlets/HA/MovePveHaResourceCmdlet.cs index 21bc4b0..413fb39 100644 --- a/src/PSProxmoxVE/Cmdlets/HA/MovePveHaResourceCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/HA/MovePveHaResourceCmdlet.cs @@ -39,7 +39,7 @@ namespace PSProxmoxVE.Cmdlets.HA var service = new HaService(); WriteVerbose($"{Mode} HA resource '{Sid}' to node '{Node}'..."); - if (Mode == "Relocate") + if (string.Equals(Mode, "Relocate", System.StringComparison.OrdinalIgnoreCase)) service.RelocateResource(session, Sid, Node); else service.MigrateResource(session, Sid, Node); diff --git a/src/PSProxmoxVE/Cmdlets/HA/NewPveHaRuleCmdlet.cs b/src/PSProxmoxVE/Cmdlets/HA/NewPveHaRuleCmdlet.cs index a46f3e4..ad53281 100644 --- a/src/PSProxmoxVE/Cmdlets/HA/NewPveHaRuleCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/HA/NewPveHaRuleCmdlet.cs @@ -47,7 +47,11 @@ namespace PSProxmoxVE.Cmdlets.HA if (Properties != null) { foreach (var key in Properties.Keys) - data[key.ToString()!] = Properties[key]!.ToString()!; + { + var value = Properties[key]?.ToString(); + if (key != null && value != null) + data[key.ToString()!] = value; + } } WriteVerbose($"Creating HA rule of type '{Type}'..."); diff --git a/src/PSProxmoxVE/Cmdlets/HA/SetPveHaRuleCmdlet.cs b/src/PSProxmoxVE/Cmdlets/HA/SetPveHaRuleCmdlet.cs index 8c43535..bb1d1d9 100644 --- a/src/PSProxmoxVE/Cmdlets/HA/SetPveHaRuleCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/HA/SetPveHaRuleCmdlet.cs @@ -48,7 +48,11 @@ namespace PSProxmoxVE.Cmdlets.HA if (Properties != null) { foreach (var key in Properties.Keys) - data[key.ToString()!] = Properties[key]!.ToString()!; + { + var value = Properties[key]?.ToString(); + if (key != null && value != null) + data[key.ToString()!] = value; + } } WriteVerbose($"Updating HA rule '{Rule}'..."); diff --git a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs index 40b38f0..2f6b775 100644 --- a/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs +++ b/src/PSProxmoxVE/Cmdlets/PveCmdletBase.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Management.Automation; using Newtonsoft.Json.Linq; using PSProxmoxVE.Core.Authentication; @@ -158,5 +159,29 @@ namespace PSProxmoxVE.Cmdlets task.Upid ?? "unknown", TimeSpan.FromSeconds(timeoutSeconds)); } + + /// + /// Parses an array of Corosync link strings (e.g. "link0=10.0.0.1") into a dictionary. + /// Emits a warning for entries that do not match the expected "key=value" format. + /// + /// Array of link strings in "linkN=address" format. + /// Dictionary of parsed link entries, or null if input is null. + protected Dictionary? ParseLinks(string[]? links) + { + if (links == null) return null; + + var result = new Dictionary(); + foreach (var link in links) + { + var parts = link.Split(new[] { '=' }, 2); + if (parts.Length != 2 || string.IsNullOrWhiteSpace(parts[0]) || string.IsNullOrWhiteSpace(parts[1])) + { + WriteWarning($"Ignoring malformed link entry '{link}'. Expected format: 'link0=10.0.0.1'"); + continue; + } + result[parts[0].Trim()] = parts[1].Trim(); + } + return result.Count > 0 ? result : null; + } } }