From 3fa7eeb0230ad6c1d68ed2c75703cbf30c81fe0a 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:09:54 +0000 Subject: [PATCH] refactor: route the container cmdlets through ContainerService (#126) (#199) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New-PveContainerSnapshot, Remove-PveContainerSnapshot and Restore-PveContainerSnapshot each built their own PveHttpClient and parsed the response inline while ContainerService.CreateContainerSnapshot / RemoveContainerSnapshot / RollbackContainerSnapshot carried the same three requests with no callers. The cmdlets now call the service, so the path and form each sends is asserted offline (ADR 0021). New-PveContainer already called ContainerService.CreateContainer but allocated its container ID with a private PveHttpClient hitting cluster/nextid; it now goes through ClusterConfigService.GetNextId, the seam Copy-PveVm, Copy-PveContainer, New-PveVm and Import-PveOva already use. ContainerService.ParseTask's UPID-string branch now stamps Status = "running", matching SnapshotService.ParseTask from the #126 pattern PR (#196). ParseTask is shared by every ContainerService lifecycle method, so this also changes the non-Wait Status output of ten cmdlets beyond the four converted here, from null to "running" — pinned with a dedicated test. Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com> --- .../Services/ContainerService.cs | 2 +- .../Containers/NewPveContainerCmdlet.cs | 9 +- .../NewPveContainerSnapshotCmdlet.cs | 22 +- .../RemovePveContainerSnapshotCmdlet.cs | 16 +- .../RestorePveContainerSnapshotCmdlet.cs | 16 +- .../Services/ContainerServiceTests.cs | 226 ++++++++++++++++++ 6 files changed, 240 insertions(+), 51 deletions(-) diff --git a/src/PSProxmoxVE.Core/Services/ContainerService.cs b/src/PSProxmoxVE.Core/Services/ContainerService.cs index dd43769..d6c8efb 100644 --- a/src/PSProxmoxVE.Core/Services/ContainerService.cs +++ b/src/PSProxmoxVE.Core/Services/ContainerService.cs @@ -513,7 +513,7 @@ namespace PSProxmoxVE.Core.Services { var data = JObject.Parse(response)["data"]; if (data?.Type == JTokenType.String) - return new PveTask { Upid = data.ToString(), Node = node }; + return new PveTask { Upid = data.ToString(), Node = node, Status = "running" }; var task = data?.ToObject() ?? new PveTask(); task.Node = node; diff --git a/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs index 56c67d4..aef0360 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs @@ -1,9 +1,6 @@ using System.Collections.Generic; using System.Management.Automation; -using System.Net; using System.Runtime.InteropServices; -using Newtonsoft.Json.Linq; -using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; using PSProxmoxVE.Core.Utilities; @@ -152,11 +149,7 @@ namespace PSProxmoxVE.Cmdlets.Containers } else { - // Auto-allocate the next available ID from the cluster. - using var allocClient = new PveHttpClient(session); - var nextIdJson = allocClient.GetAsync("cluster/nextid").GetAwaiter().GetResult(); - var nextIdData = JObject.Parse(nextIdJson)["data"]; - config["vmid"] = int.Parse(nextIdData!.ToString()); + config["vmid"] = new ClusterConfigService().GetNextId(session).ToString(); } if (!string.IsNullOrEmpty(Hostname)) config["hostname"] = Hostname!; diff --git a/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerSnapshotCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerSnapshotCmdlet.cs index d23ca1d..433eb2c 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerSnapshotCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerSnapshotCmdlet.cs @@ -1,8 +1,4 @@ -using System; -using System.Collections.Generic; using System.Management.Automation; -using Newtonsoft.Json.Linq; -using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; @@ -46,25 +42,15 @@ namespace PSProxmoxVE.Cmdlets.Containers return; var session = GetSession(); - using var client = new PveHttpClient(session); WriteVerbose($"Creating snapshot '{Name}' for container {VmId}..."); - var data = new Dictionary - { - ["snapname"] = Name - }; - if (!string.IsNullOrEmpty(Description)) data["description"] = Description!; + var service = new ContainerService(); + var task = service.CreateContainerSnapshot(session, Node, VmId, Name, Description); - var json = client.PostAsync($"nodes/{Uri.EscapeDataString(Node)}/lxc/{VmId}/snapshot", data).GetAwaiter().GetResult(); - var root = JObject.Parse(json); - var upid = root["data"]?.ToString() ?? string.Empty; - - var task = new PveTask { Upid = upid, Node = Node, Status = "running" }; - - if (Wait.IsPresent && !string.IsNullOrEmpty(upid)) + if (Wait.IsPresent && !string.IsNullOrEmpty(task.Upid)) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, upid); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Containers/RemovePveContainerSnapshotCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/RemovePveContainerSnapshotCmdlet.cs index 3c10e0f..68f9e3e 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/RemovePveContainerSnapshotCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/RemovePveContainerSnapshotCmdlet.cs @@ -1,7 +1,4 @@ -using System; using System.Management.Automation; -using Newtonsoft.Json.Linq; -using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; @@ -45,18 +42,13 @@ namespace PSProxmoxVE.Cmdlets.Containers return; WriteVerbose($"Removing snapshot '{Name}' from container {VmId}..."); - using var client = new PveHttpClient(session); + var service = new ContainerService(); + var task = service.RemoveContainerSnapshot(session, Node, VmId, Name); - var json = client.DeleteAsync($"nodes/{Uri.EscapeDataString(Node)}/lxc/{VmId}/snapshot/{Uri.EscapeDataString(Name)}").GetAwaiter().GetResult(); - var root = JObject.Parse(json); - var upid = root["data"]?.ToString() ?? string.Empty; - - var task = new PveTask { Upid = upid, Node = Node, Status = "running" }; - - if (Wait.IsPresent && !string.IsNullOrEmpty(upid)) + if (Wait.IsPresent && !string.IsNullOrEmpty(task.Upid)) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, upid); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Containers/RestorePveContainerSnapshotCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/RestorePveContainerSnapshotCmdlet.cs index 8a4a1c9..9873027 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/RestorePveContainerSnapshotCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/RestorePveContainerSnapshotCmdlet.cs @@ -1,7 +1,4 @@ -using System; using System.Management.Automation; -using Newtonsoft.Json.Linq; -using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; @@ -48,18 +45,13 @@ namespace PSProxmoxVE.Cmdlets.Containers return; WriteVerbose($"Restoring snapshot '{Name}' on container {VmId}..."); - using var client = new PveHttpClient(session); + var service = new ContainerService(); + var task = service.RollbackContainerSnapshot(session, Node, VmId, Name); - var json = client.PostAsync($"nodes/{Uri.EscapeDataString(Node)}/lxc/{VmId}/snapshot/{Uri.EscapeDataString(Name)}/rollback").GetAwaiter().GetResult(); - var root = JObject.Parse(json); - var upid = root["data"]?.ToString() ?? string.Empty; - - var task = new PveTask { Upid = upid, Node = Node, Status = "running" }; - - if (Wait.IsPresent && !string.IsNullOrEmpty(upid)) + if (Wait.IsPresent && !string.IsNullOrEmpty(task.Upid)) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, upid); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs index bb445fa..f5f2599 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/ContainerServiceTests.cs @@ -15,10 +15,236 @@ namespace PSProxmoxVE.Core.Tests.Services { private const string TestNode = "pve1"; private const int TestVmId = 100; + private const string CreateSnapshotUpid = "UPID:pve1:000ABC:00000001:5F1234AB:vzsnapshot:100:root@pam:"; private static PveSession CreateSession() => new PveSession("pve1.example.com", 8006, true, "PVE:root@pam:TEST_TOKEN"); + private sealed class CapturedPost + { + public int Calls { get; set; } + public string? Path { get; set; } + public Dictionary? Form { get; set; } + } + + private static string UpidJson(string upid) => $@"{{""data"": ""{upid}""}}"; + + private static CapturedPost CapturePost(Mock mockClient, string json) + { + var captured = new CapturedPost(); + mockClient.Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) + .Callback?>((path, form) => + { + captured.Calls++; + captured.Path = path; + captured.Form = form; + }) + .ReturnsAsync(json); + return captured; + } + + // --------------------------------------------------------------------- + // Snapshots: #126 (Containers area) + // --------------------------------------------------------------------- + + [Fact] + public void CreateContainerSnapshot_NameOnly_SendsOnlySnapname() + { + var mockClient = new Mock(); + var captured = CapturePost(mockClient, UpidJson(CreateSnapshotUpid)); + var service = new ContainerService(mockClient.Object); + + var task = service.CreateContainerSnapshot(CreateSession(), TestNode, TestVmId, "my-snap"); + + Assert.Equal(1, captured.Calls); + Assert.Equal($"nodes/{TestNode}/lxc/{TestVmId}/snapshot", captured.Path); + Assert.NotNull(captured.Form); + Assert.Equal("my-snap", captured.Form!["snapname"]); + Assert.False(captured.Form.ContainsKey("description")); + Assert.Single(captured.Form); + + Assert.Equal(CreateSnapshotUpid, task.Upid); + Assert.Equal(TestNode, task.Node); + Assert.Equal("running", task.Status); + } + + [Fact] + public void CreateContainerSnapshot_NullData_ReturnsEmptyUpidWithoutStatus() + { + var mockClient = new Mock(); + CapturePost(mockClient, @"{""data"": null}"); + var service = new ContainerService(mockClient.Object); + + var task = service.CreateContainerSnapshot(CreateSession(), TestNode, TestVmId, "my-snap"); + + Assert.Equal(string.Empty, task.Upid); + Assert.Equal(TestNode, task.Node); + Assert.Null(task.Status); + } + + [Fact] + public void CreateContainerSnapshot_ObjectShapedData_ReturnsTaskFields() + { + var json = $@"{{""data"": {{""upid"": ""{CreateSnapshotUpid}"", ""status"": ""stopped"", ""exitstatus"": ""OK""}}}}"; + var mockClient = new Mock(); + CapturePost(mockClient, json); + var service = new ContainerService(mockClient.Object); + + var task = service.CreateContainerSnapshot(CreateSession(), TestNode, TestVmId, "my-snap"); + + Assert.Equal(CreateSnapshotUpid, task.Upid); + Assert.Equal(TestNode, task.Node); + Assert.Equal("stopped", task.Status); + Assert.Equal("OK", task.ExitStatus); + } + + [Fact] + public void CreateContainerSnapshot_WithDescription_SendsDescription() + { + var mockClient = new Mock(); + var captured = CapturePost(mockClient, UpidJson(CreateSnapshotUpid)); + var service = new ContainerService(mockClient.Object); + + service.CreateContainerSnapshot(CreateSession(), TestNode, TestVmId, "my-snap", "Test snapshot"); + + Assert.Equal(1, captured.Calls); + Assert.NotNull(captured.Form); + Assert.Equal("my-snap", captured.Form!["snapname"]); + Assert.Equal("Test snapshot", captured.Form["description"]); + Assert.Equal(2, captured.Form.Count); + } + + [Fact] + public void CreateContainerSnapshot_EscapesNodeInPath() + { + var mockClient = new Mock(); + var captured = CapturePost(mockClient, UpidJson(CreateSnapshotUpid)); + var service = new ContainerService(mockClient.Object); + + service.CreateContainerSnapshot(CreateSession(), "pve node", TestVmId, "my-snap"); + + Assert.Equal($"nodes/pve%20node/lxc/{TestVmId}/snapshot", captured.Path); + } + + [Fact] + public void RemoveContainerSnapshot_CallsDeleteAsync_ReturnsRunningTask() + { + const string upid = "UPID:pve1:000DEF:00000002:5F1234AC:vzdelsnap:100:root@pam:"; + var mockClient = new Mock(); + mockClient.Setup(c => c.DeleteAsync(It.IsAny())) + .ReturnsAsync(UpidJson(upid)); + + var service = new ContainerService(mockClient.Object); + + var task = service.RemoveContainerSnapshot(CreateSession(), TestNode, TestVmId, "clean-install"); + + Assert.Equal(upid, task.Upid); + Assert.Equal(TestNode, task.Node); + Assert.Equal("running", task.Status); + mockClient.Verify(c => c.DeleteAsync($"nodes/{TestNode}/lxc/{TestVmId}/snapshot/clean-install"), Times.Once); + mockClient.VerifyNoOtherCalls(); + } + + [Fact] + public void RemoveContainerSnapshot_EscapesNodeAndSnapnameInPath() + { + var mockClient = new Mock(); + mockClient.Setup(c => c.DeleteAsync(It.IsAny())) + .ReturnsAsync(UpidJson("UPID:pve1:000DEF:00000002:5F1234AC:vzdelsnap:100:root@pam:")); + + var service = new ContainerService(mockClient.Object); + + service.RemoveContainerSnapshot(CreateSession(), "pve node", TestVmId, "snap name"); + + mockClient.Verify(c => c.DeleteAsync($"nodes/pve%20node/lxc/{TestVmId}/snapshot/snap%20name"), Times.Once); + } + + [Fact] + public void RollbackContainerSnapshot_CallsPostAsync_ReturnsRunningTask() + { + const string upid = "UPID:pve1:000GHI:00000003:5F1234AD:vzrollback:100:root@pam:"; + var mockClient = new Mock(); + var captured = CapturePost(mockClient, UpidJson(upid)); + + var service = new ContainerService(mockClient.Object); + + var task = service.RollbackContainerSnapshot(CreateSession(), TestNode, TestVmId, "clean-install"); + + Assert.Equal(upid, task.Upid); + Assert.Equal(TestNode, task.Node); + Assert.Equal("running", task.Status); + Assert.Equal($"nodes/{TestNode}/lxc/{TestVmId}/snapshot/clean-install/rollback", captured.Path); + Assert.Null(captured.Form); + Assert.Equal(1, captured.Calls); + } + + [Fact] + public void RollbackContainerSnapshot_EscapesNodeAndSnapnameInPath() + { + var mockClient = new Mock(); + var captured = CapturePost(mockClient, UpidJson("UPID:pve1:000GHI:00000003:5F1234AD:vzrollback:100:root@pam:")); + var service = new ContainerService(mockClient.Object); + + service.RollbackContainerSnapshot(CreateSession(), "pve node", TestVmId, "snap name"); + + Assert.Equal($"nodes/pve%20node/lxc/{TestVmId}/snapshot/snap%20name/rollback", captured.Path); + } + + [Fact] + public void CreateContainerSnapshot_NullSession_ThrowsArgumentNullException() + { + var service = new ContainerService(new Mock().Object); + + Assert.Throws("session", + () => service.CreateContainerSnapshot(null!, TestNode, TestVmId, "my-snap")); + } + + [Fact] + public void RemoveContainerSnapshot_NullSession_ThrowsArgumentNullException() + { + var service = new ContainerService(new Mock().Object); + + Assert.Throws("session", + () => service.RemoveContainerSnapshot(null!, TestNode, TestVmId, "my-snap")); + } + + [Fact] + public void RollbackContainerSnapshot_NullSession_ThrowsArgumentNullException() + { + var service = new ContainerService(new Mock().Object); + + Assert.Throws("session", + () => service.RollbackContainerSnapshot(null!, TestNode, TestVmId, "my-snap")); + } + + [Fact] + public void RollbackContainerSnapshot_WhitespaceSnapname_ThrowsArgumentNullException() + { + var service = new ContainerService(new Mock().Object); + + Assert.Throws("snapname", + () => service.RollbackContainerSnapshot(CreateSession(), TestNode, TestVmId, " ")); + } + + // --------------------------------------------------------------------- + // ParseTask "running" stamp: applies to every UPID-string response, + // not only the three snapshot methods (#126 reconciliation). + // --------------------------------------------------------------------- + + [Fact] + public void RemoveContainer_UpidStringResponse_StampsRunningStatus() + { + var mockClient = new Mock(); + mockClient + .Setup(c => c.DeleteAsync(It.IsAny())) + .ReturnsAsync("{\"data\":\"UPID:pve1:00001234:00005678:6A970AAB:vzdestroy:100:root@pam:\"}"); + + var service = new ContainerService(mockClient.Object); + var task = service.RemoveContainer(CreateSession(), TestNode, TestVmId); + + Assert.Equal("running", task.Status); + } + [Fact] public void RemoveContainer_WithForceTrue_IncludesForceInQueryString() {