gsadmin/PSProxmoxVE
1
0
Fork 0
mirror of https://github.com/GoodOlClint/PSProxmoxVE.git synced 2026-09-06 20:19:01 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: address reviewer findings on -Session handling and test coverage

Browse Source 
Fixes from correctness and api-compat reviews:

1. Use BoundParameters to distinguish -Session omitted from -Session $null,
   preventing accidental active-session clear when $null is passed.

2. Warn and return early when -Session is supplied but not the active session,
   avoiding silent no-ops that leave the user's session variable populated and
   functional but with expectations misaligned (they passed a session to
   disconnect it, but disconnecting a non-active session is now explicit).

3. Use ReferenceEquals() explicitly instead of == for the identity check,
   future-proofing against PveSession ever gaining value-equality semantics.

4. Fix the lifecycle test to check observable behavior (warning output) instead
   of reaching into null PrivateData. Tests now verify both "no session to
   disconnect" and "non-active session supplied" paths.
This commit is contained in:
goodolclint-claude[bot]
2026-09-02 16:34:39 +00:00
committed by GitHub
parent e4a667e7fc
commit fa930db71a
2 changed files with 15 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/PSProxmoxVE/Cmdlets/Connection/DisconnectPveServerCmdlet.cs
+6 -3
View File
@@ -17,7 +17,8 @@ namespace PSProxmoxVE.Cmdlets.Connection
{ {
protected override void ProcessRecord() protected override void ProcessRecord()
{ {
var sessionToDisconnect = Session ?? ModuleState.ActiveSession; bool explicitSessionSupplied = MyInvocation.BoundParameters.ContainsKey(nameof(Session));
var sessionToDisconnect = explicitSessionSupplied ? Session : ModuleState.ActiveSession;
if (sessionToDisconnect is null) if (sessionToDisconnect is null)
{ {
@@ -28,11 +29,13 @@ namespace PSProxmoxVE.Cmdlets.Connection
if (!ShouldProcess($"{sessionToDisconnect.Hostname}:{sessionToDisconnect.Port}", "Disconnect")) if (!ShouldProcess($"{sessionToDisconnect.Hostname}:{sessionToDisconnect.Port}", "Disconnect"))
return; return;
if (Session is null || sessionToDisconnect == ModuleState.ActiveSession) if (!ReferenceEquals(sessionToDisconnect, ModuleState.ActiveSession))
{ {
ModuleState.ActiveSession = null; WriteWarning($"The supplied session for {sessionToDisconnect.Hostname}:{sessionToDisconnect.Port} is not the module-level session; nothing was changed. Discard the variable — PVE tickets cannot be revoked and expire on their own.");
return;
} }
ModuleState.ActiveSession = null;
WriteVerbose($"Disconnected from {sessionToDisconnect.Hostname}:{sessionToDisconnect.Port}."); WriteVerbose($"Disconnected from {sessionToDisconnect.Hostname}:{sessionToDisconnect.Port}.");
} }
} }
tests/PSProxmoxVE.Tests/Connection/Disconnect-PveServer.Tests.ps1
+9 -4
View File
@@ -59,10 +59,15 @@ Describe 'Disconnect-PveServer' {
} }
Context 'Active session lifecycle' { Context 'Active session lifecycle' {
It 'Should clear active session when disconnected without explicit -Session' { It 'Should report "no session" after disconnecting the active session' {
Disconnect-PveServer -Confirm:$false -ErrorAction SilentlyContinue Disconnect-PveServer -Confirm:$false -WarningVariable w
$Module = Get-Module PSProxmoxVE $w[0] | Should -Match 'No active Proxmox VE session'
$Module.PrivateData.ModuleState.ActiveSession | Should -BeNullOrEmpty }
It 'Should warn when disconnecting an explicit non-active session' {
$fakeSession = [PSCustomObject]@{ Hostname = "test.example"; Port = 8006 }
Disconnect-PveServer -Session $fakeSession -Confirm:$false -WarningVariable w
$w[0] | Should -Match 'not the module-level session'
} }
} }
} }