From 3f7f69b25024db1269cfe0c3b3868de7bab0cb27 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 00:11:50 +0000 Subject: [PATCH] refactor: route the CloudInit/Templates cmdlets through their service seams (#126) (#200) Get-PveCloudInitConfig and Get-PveTemplate each built their own PveHttpClient inline. They now go through CloudInitService and TemplateService, which is offline-testable per ADR 0021. Get-PveCloudInitConfig's shipped output is the full PveVmConfig (per its own synopsis), not the CI-key-filtered PveCloudInitConfig that CloudInitService.GetCloudInitConfig already returns for a different purpose. CloudInitService gains GetFullVmConfig, a thin delegation to the existing VmService.GetVmConfig (already used by Get-PveVmConfig) rather than a parallel HTTP implementation. Get-PveTemplate's TemplateService.GetTemplates now forwards VmService.GetVms's onNodeSkipped callback, matching Get-PveVm/Get-PveContainer, so an unreachable node during the all-nodes listing produces a warning instead of a silently short result. Two review-found regressions were fixed before commit: an empty -Node value used to mean "query all nodes" and now does again (it had started reaching the API literally as nodes//qemu), and the per-node warning above. Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com> --- .../Services/CloudInitService.cs | 28 ++++- .../Services/TemplateService.cs | 8 +- .../CloudInit/GetPveCloudInitConfigCmdlet.cs | 12 +- .../Cmdlets/Templates/GetPveTemplateCmdlet.cs | 64 +++------- .../Services/CloudInitServiceTests.cs | 49 ++++++++ .../Services/TemplateServiceTests.cs | 112 ++++++++++++++++++ 6 files changed, 215 insertions(+), 58 deletions(-) diff --git a/src/PSProxmoxVE.Core/Services/CloudInitService.cs b/src/PSProxmoxVE.Core/Services/CloudInitService.cs index 9252b7a..3446c52 100644 --- a/src/PSProxmoxVE.Core/Services/CloudInitService.cs +++ b/src/PSProxmoxVE.Core/Services/CloudInitService.cs @@ -22,16 +22,40 @@ namespace PSProxmoxVE.Core.Services "nameserver", "searchdomain", "cicustom" }; + private readonly VmService _vmService; + /// /// Initializes a new instance of the class. /// - public CloudInitService() { } + public CloudInitService() + { + _vmService = new VmService(); + } /// /// Initializes a new instance of the class with an injected HTTP client. /// /// The HTTP client to use for API calls. The caller owns its lifetime. - public CloudInitService(IPveHttpClient client) : base(client) { } + public CloudInitService(IPveHttpClient client) : base(client) + { + _vmService = new VmService(client); + } + + /// + /// Returns the full VM configuration (delegates to ). + /// Cloud-Init fields (ciuser, ipconfig*, etc.) are present as ordinary properties of + /// the returned config alongside every other setting. + /// + /// The authenticated PVE session. + /// The cluster node name. + /// The VM ID. + public PveVmConfig GetFullVmConfig(PveSession session, string node, int vmid) + { + if (session == null) throw new ArgumentNullException(nameof(session)); + if (string.IsNullOrWhiteSpace(node)) throw new ArgumentNullException(nameof(node)); + + return _vmService.GetVmConfig(session, node, vmid); + } /// /// Retrieves the Cloud-Init specific configuration fields for a VM. diff --git a/src/PSProxmoxVE.Core/Services/TemplateService.cs b/src/PSProxmoxVE.Core/Services/TemplateService.cs index 6f05627..569b830 100644 --- a/src/PSProxmoxVE.Core/Services/TemplateService.cs +++ b/src/PSProxmoxVE.Core/Services/TemplateService.cs @@ -37,11 +37,15 @@ namespace PSProxmoxVE.Core.Services /// /// The authenticated PVE session. /// Optional cluster node name to filter templates by node. - public PveVm[] GetTemplates(PveSession session, string? node = null) + /// + /// Optional callback invoked with the node name and the exception when a node is + /// skipped because it is unreachable, forwarded to . + /// + public PveVm[] GetTemplates(PveSession session, string? node = null, Action? onNodeSkipped = null) { if (session == null) throw new ArgumentNullException(nameof(session)); - var vms = _vmService.GetVms(session, node); + var vms = _vmService.GetVms(session, node, onNodeSkipped); return vms.Where(v => v.Template == 1).ToArray(); } diff --git a/src/PSProxmoxVE/Cmdlets/CloudInit/GetPveCloudInitConfigCmdlet.cs b/src/PSProxmoxVE/Cmdlets/CloudInit/GetPveCloudInitConfigCmdlet.cs index 5f9e8d7..7d74299 100644 --- a/src/PSProxmoxVE/Cmdlets/CloudInit/GetPveCloudInitConfigCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/CloudInit/GetPveCloudInitConfigCmdlet.cs @@ -1,8 +1,6 @@ -using System; using System.Management.Automation; -using Newtonsoft.Json.Linq; -using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Vms; +using PSProxmoxVE.Core.Services; namespace PSProxmoxVE.Cmdlets.CloudInit { @@ -31,14 +29,10 @@ namespace PSProxmoxVE.Cmdlets.CloudInit { var session = GetSession(); RequireVersion(session, "Cloud-Init management", 7, 2); - using var client = new PveHttpClient(session); + var service = new CloudInitService(); WriteVerbose($"Getting cloud-init config for VM {VmId}..."); - var json = client.GetAsync($"nodes/{Uri.EscapeDataString(Node)}/qemu/{VmId}/config").GetAwaiter().GetResult(); - var root = JObject.Parse(json); - var data = root["data"]; - - var config = data?.ToObject() ?? new PveVmConfig(); + var config = service.GetFullVmConfig(session, Node, VmId); WriteObject(config); } } diff --git a/src/PSProxmoxVE/Cmdlets/Templates/GetPveTemplateCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Templates/GetPveTemplateCmdlet.cs index 5df7ee5..ec406b2 100644 --- a/src/PSProxmoxVE/Cmdlets/Templates/GetPveTemplateCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Templates/GetPveTemplateCmdlet.cs @@ -1,8 +1,6 @@ -using System; using System.Management.Automation; -using Newtonsoft.Json.Linq; -using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Vms; +using PSProxmoxVE.Core.Services; namespace PSProxmoxVE.Cmdlets.Templates { @@ -28,57 +26,33 @@ namespace PSProxmoxVE.Cmdlets.Templates protected override void ProcessRecord() { var session = GetSession(); - using var client = new PveHttpClient(session); + var service = new TemplateService(); WriteVerbose("Getting templates..."); - var nodesToQuery = new System.Collections.Generic.List(); + var queryNode = string.IsNullOrEmpty(Node) ? null : Node; + var templates = service.GetTemplates(session, queryNode, + onNodeSkipped: (nodeName, ex) => WriteWarning($"Skipping node '{nodeName}': {ex.Message}")); - if (!string.IsNullOrEmpty(Node)) + foreach (var vm in templates) { - nodesToQuery.Add(Node!); - } - else - { - var nodesJson = client.GetAsync("nodes").GetAwaiter().GetResult(); - var nodesRoot = JObject.Parse(nodesJson); - var nodesData = nodesRoot["data"] as JArray ?? new JArray(); - foreach (var n in nodesData) - nodesToQuery.Add(n["node"]?.ToString() ?? string.Empty); - } + if (vm.Node == null && queryNode != null) vm.Node = queryNode; - foreach (var node in nodesToQuery) - { - if (string.IsNullOrEmpty(node)) continue; - - var json = client.GetAsync($"nodes/{Uri.EscapeDataString(node)}/qemu").GetAwaiter().GetResult(); - var root = JObject.Parse(json); - var data = root["data"] as JArray ?? new JArray(); - - foreach (var item in data) + if (!string.IsNullOrEmpty(Name) && vm.Name != null) { - var vm = item.ToObject()!; - if (vm.Node == null) vm.Node = node; - - // Only return templates - if (vm.Template != 1) continue; - - if (!string.IsNullOrEmpty(Name) && vm.Name != null) + var pattern = Name!.Replace("*", ""); + if (Name.Contains("*")) { - var pattern = Name!.Replace("*", ""); - if (Name.Contains("*")) - { - if (vm.Name.IndexOf(pattern, System.StringComparison.OrdinalIgnoreCase) < 0) - continue; - } - else - { - if (!string.Equals(vm.Name, Name, System.StringComparison.OrdinalIgnoreCase)) - continue; - } + if (vm.Name.IndexOf(pattern, System.StringComparison.OrdinalIgnoreCase) < 0) + continue; + } + else + { + if (!string.Equals(vm.Name, Name, System.StringComparison.OrdinalIgnoreCase)) + continue; } - - WriteObject(vm); } + + WriteObject(vm); } } } diff --git a/tests/PSProxmoxVE.Core.Tests/Services/CloudInitServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/CloudInitServiceTests.cs index fd898bc..1d20cac 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/CloudInitServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/CloudInitServiceTests.cs @@ -156,5 +156,54 @@ namespace PSProxmoxVE.Core.Tests.Services { Assert.Throws(() => new CloudInitService(null!)); } + + [Fact] + public void GetFullVmConfig_ReturnsFullConfig_IncludingCloudInitAndOtherFields() + { + // Arrange + var json = @"{""data"": { + ""ciuser"": ""ubuntu"", + ""ipconfig0"": ""ip=dhcp"", + ""cores"": 4, + ""memory"": 8192 + }}"; + var mockClient = new Mock(); + mockClient.Setup(c => c.GetAsync("nodes/pve1/qemu/100/config")).ReturnsAsync(json); + var service = new CloudInitService(mockClient.Object); + + // Act + var config = service.GetFullVmConfig(CreateSession(), "pve1", 100); + + // Assert — cloud-init fields and every other config field are both present + Assert.Equal("ubuntu", config.CiUser); + Assert.Equal("ip=dhcp", config.IpConfig0); + Assert.Equal(4, config.Cores); + Assert.Equal(8192, config.Memory); + mockClient.Verify(c => c.GetAsync("nodes/pve1/qemu/100/config"), Times.Once); + } + + [Fact] + public void GetFullVmConfig_EscapesNodeInPath() + { + // Arrange + var json = @"{""data"": {}}"; + var mockClient = new Mock(); + mockClient.Setup(c => c.GetAsync("nodes/pve%20node/qemu/100/config")).ReturnsAsync(json); + var service = new CloudInitService(mockClient.Object); + + // Act + service.GetFullVmConfig(CreateSession(), "pve node", 100); + + // Assert + mockClient.Verify(c => c.GetAsync("nodes/pve%20node/qemu/100/config"), Times.Once); + } + + [Fact] + public void GetFullVmConfig_NullSession_ThrowsArgumentNullException() + { + var service = new CloudInitService(new Mock().Object); + + Assert.Throws(() => service.GetFullVmConfig(null!, "pve1", 100)); + } } } diff --git a/tests/PSProxmoxVE.Core.Tests/Services/TemplateServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/TemplateServiceTests.cs index 2f9cb3b..0e90373 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/TemplateServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/TemplateServiceTests.cs @@ -1,10 +1,12 @@ using System; using System.Collections.Generic; +using System.Net; using System.Threading.Tasks; using Moq; using Xunit; using PSProxmoxVE.Core.Authentication; using PSProxmoxVE.Core.Client; +using PSProxmoxVE.Core.Exceptions; using PSProxmoxVE.Core.Services; namespace PSProxmoxVE.Core.Tests.Services @@ -101,5 +103,115 @@ namespace PSProxmoxVE.Core.Tests.Services { Assert.Throws("client", () => new TemplateService(null!)); } + + [Fact] + public void GetTemplates_SingleNode_ReturnsOnlyTemplateFlaggedVms() + { + // Arrange + var json = @"{""data"": [ + {""vmid"": 100, ""name"": ""web-template"", ""template"": 1}, + {""vmid"": 101, ""name"": ""running-vm"", ""template"": 0}, + {""vmid"": 102, ""name"": ""db-template"", ""template"": 1} + ]}"; + var mockClient = new Mock(); + mockClient.Setup(c => c.GetAsync($"nodes/{Node}/qemu")).ReturnsAsync(json); + var service = new TemplateService(mockClient.Object); + + // Act + var templates = service.GetTemplates(CreateSession(), Node); + + // Assert + Assert.Equal(2, templates.Length); + Assert.All(templates, t => Assert.Equal(1, t.Template)); + Assert.Contains(templates, t => t.VmId == 100); + Assert.Contains(templates, t => t.VmId == 102); + mockClient.Verify(c => c.GetAsync($"nodes/{Node}/qemu"), Times.Once); + } + + [Fact] + public void GetTemplates_EscapesNodeInPath() + { + // Arrange + var json = @"{""data"": []}"; + var mockClient = new Mock(); + mockClient.Setup(c => c.GetAsync("nodes/pve%20node/qemu")).ReturnsAsync(json); + var service = new TemplateService(mockClient.Object); + + // Act + service.GetTemplates(CreateSession(), "pve node"); + + // Assert + mockClient.Verify(c => c.GetAsync("nodes/pve%20node/qemu"), Times.Once); + } + + [Fact] + public void GetTemplates_NullSession_ThrowsArgumentNullException() + { + var service = new TemplateService(new Mock().Object); + + Assert.Throws("session", () => service.GetTemplates(null!, Node)); + } + + [Fact] + public void GetTemplates_AllNodes_AggregatesAcrossNodesAndStampsNode() + { + // Arrange + var mockClient = new Mock(); + mockClient.Setup(c => c.GetAsync("nodes")) + .ReturnsAsync(@"{""data"": [{""node"": ""pve1""}, {""node"": ""pve2""}]}"); + mockClient.Setup(c => c.GetAsync("nodes/pve1/qemu")) + .ReturnsAsync(@"{""data"": [{""vmid"": 100, ""template"": 1}, {""vmid"": 101, ""template"": 0}]}"); + mockClient.Setup(c => c.GetAsync("nodes/pve2/qemu")) + .ReturnsAsync(@"{""data"": [{""vmid"": 200, ""template"": 1}]}"); + var service = new TemplateService(mockClient.Object); + + // Act + var templates = service.GetTemplates(CreateSession()); + + // Assert + Assert.Equal(2, templates.Length); + Assert.Contains(templates, t => t.VmId == 100 && t.Node == "pve1"); + Assert.Contains(templates, t => t.VmId == 200 && t.Node == "pve2"); + } + + [Fact] + public void GetTemplates_AllNodes_UnreachableNodeIsSkippedAndReported() + { + // Arrange + var mockClient = new Mock(); + mockClient.Setup(c => c.GetAsync("nodes")) + .ReturnsAsync(@"{""data"": [{""node"": ""pve1""}, {""node"": ""pve2""}]}"); + mockClient.Setup(c => c.GetAsync("nodes/pve1/qemu")) + .ReturnsAsync(@"{""data"": [{""vmid"": 100, ""template"": 1}]}"); + mockClient.Setup(c => c.GetAsync("nodes/pve2/qemu")) + .ThrowsAsync(new PveApiException(HttpStatusCode.InternalServerError, "internal error", "nodes/pve2/qemu", "GET")); + var service = new TemplateService(mockClient.Object); + + // Act + var skipped = new List(); + var templates = service.GetTemplates(CreateSession(), onNodeSkipped: (node, ex) => skipped.Add(node)); + + // Assert + var template = Assert.Single(templates); + Assert.Equal(100, template.VmId); + Assert.Equal(new[] { "pve2" }, skipped); + } + + [Fact] + public void GetTemplates_AllNodes_PermissionErrorOnOneNodePropagates() + { + // Arrange + var mockClient = new Mock(); + mockClient.Setup(c => c.GetAsync("nodes")) + .ReturnsAsync(@"{""data"": [{""node"": ""pve1""}, {""node"": ""pve2""}]}"); + mockClient.Setup(c => c.GetAsync("nodes/pve1/qemu")) + .ReturnsAsync(@"{""data"": [{""vmid"": 100, ""template"": 1}]}"); + mockClient.Setup(c => c.GetAsync("nodes/pve2/qemu")) + .ThrowsAsync(new PveApiException(HttpStatusCode.Forbidden, "permission denied", "nodes/pve2/qemu", "GET")); + var service = new TemplateService(mockClient.Object); + + // Act & Assert + Assert.Throws(() => service.GetTemplates(CreateSession())); + } } }