Skip to content

Add error for Get-PSSession -ComputerName on Unix #21009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
iSazonov merged 2 commits into from
Dec 5, 2024
Merged

Add error for Get-PSSession -ComputerName on Unix #21009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc3e7a6
Get-PSSession: Add error for -ComputerName on Unix
jborean93 Jan 5, 2024
186a173
Fix up test
jborean93 Jan 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/System.Management.Automation/engine/remoting/commands/getrunspacecommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Management.Automation.Internal;
using System.Management.Automation.Remoting;
using System.Management.Automation.Runspaces;
using System.Runtime.InteropServices;

using Dbg = System.Management.Automation.Diagnostics;

Expand Down Expand Up @@ -341,8 +342,22 @@ public string CertificateThumbprint
/// </summary>
protected override void BeginProcessing()
{
base.BeginProcessing();
#if UNIX
if (ComputerName?.Length > 0)
Copy link
Collaborator

@iSazonov iSazonov Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our common pattern is to check whether a parameter presents.

if (MyInvocation.BoundParameters.ContainsKey(nameof(ComputerName)))

Copy link
Collaborator Author

@jborean93 jborean93 Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason for this. The code uses the ComputerName property value and thus it makes more sense to do the check on the actual property rather than a pwsh-ism here. It's also cleaner and shorter than accessing a dictionary to check if a key exists vs just checking the property value.

Copy link
Collaborator

@SeeminglyScience SeeminglyScience Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah there are situations where it's better but I think it's fine here.

{
ErrorRecord err = new(
new NotImplementedException(
PSRemotingErrorInvariants.FormatResourceString(
RemotingErrorIdStrings.UnsupportedOSForRemoteEnumeration,
RuntimeInformation.OSDescription)),
"PSSessionComputerNameUnix",
ErrorCategory.NotImplemented,
null);
ThrowTerminatingError(err);
}
#endif

base.BeginProcessing();
ConfigurationName ??= string.Empty;
}

Expand Down
1 change: 1 addition & 0 deletions src/System.Management.Automation/engine/remoting/common/remotingexceptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ internal enum PSRemotingErrorId : uint
// OS related 1-9
DefaultRemotingExceptionMessage = 0,
OutOfMemory = 1,
UnsupportedOSForRemoteEnumeration = 2,

// Pipeline related range: 10-99
PipelineIdsDoNotMatch = 10,
Expand Down
3 changes: 3 additions & 0 deletions src/System.Management.Automation/resources/RemotingErrorIdStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@
<data name="OutOfMemory" xml:space="preserve">
<value>Out of process memory.</value>
</data>
<data name="UnsupportedOSForRemoteEnumeration" xml:space="preserve">
<value>Remote PSSession enumeration with -ComputerName is only supported on Windows and not "{0}".</value>
Copy link
Collaborator

@iSazonov iSazonov Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes no sense to point OS version. Please remove:

Suggested change
<value>Remote PSSession enumeration with -ComputerName is only supported on Windows and not "{0}".</value>
<value>Remote PSSession enumeration with -ComputerName is only supported on Windows.</value>

Copy link
Collaborator Author

@jborean93 jborean93 Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it provides a few extra debug details and doesn't hurt to include. It's also somewhat helpful to identify where the cmdlet was run when it comes to remoting ones as people may be confused if the OS platform is local or remote based. Including the local OS information in the error helps to avoid that ambiguity.

</data>
<data name="PipelineIdsDoNotMatch" xml:space="preserve">
<value>Pipeline ID "{0}" does not match the InstanceId of the pipeline that is currently running, "{1}".</value>
</data>
Expand Down