Enable specifying sshd subsystem to use via -Subsystem - #6603
Enable specifying sshd subsystem to use via -Subsystem#6603Travis Plunk (TravisEz13) merged 3 commits into
Conversation
|
restarted mac job due to restore failure |
Paul Higinbotham (PaulHigin)
left a comment
Paul Higinbotham (PaulHigin)
left a comment
There was a problem hiding this comment.
I question whether we should use 'ConfigurationName' to specify the SSH PowerShell subsystem. I feel it will only confuse users where 'ConfigurationName' specifies a WinRM based PowerShell configuration. Also we would like to add session configuration to SSH in the future and should reserve the name for that. Instead we should have a new parameter name such as 'PSSubsystemName' or just 'PSSubsystem'.
|
Paul Higinbotham (@PaulHigin) I'm ok with having it separate from cc Hemant Mahawar (@HemantMahawar) Joey Aiello (@joeyaiello) |
| private const string KeyFilePathParameter = "KeyFilePath"; | ||
| private const string IdentityFilePathAlias = "IdentityFilePath"; | ||
| private const string PortParameter = "Port"; | ||
| private const string ConfigurationNameParameter = "ConfigurationName"; |
There was a problem hiding this comment.
We should remove this parameter since we don't yet support it.
There was a problem hiding this comment.
Yes, accidentally left behind
| /// <summary> | ||
| /// This parameter specifies the SSH subsystem to use for the remote connection. | ||
| /// </summary> | ||
| [Parameter(ValueFromPipelineByPropertyName = true, |
There was a problem hiding this comment.
You shouldn't need this here. This property is picked up by the base class.
| /// <summary> | ||
| /// This parameter specifies the SSH subsystem to use for the remote connection. | ||
| /// </summary> | ||
| [Parameter(ValueFromPipelineByPropertyName = true, |
There was a problem hiding this comment.
This parameter definition should go in the PSRemotingBaseCmdlet, but this appears to be in the PSExecutionCmdlet class (which derives from PSRemotingBaseCmdlet). Please move it there under the SSHHostParameters section.
There was a problem hiding this comment.
This should be the only Subsystem parameter, other cmdlets should be inheriting from this one now.
| this.KeyFilePath, | ||
| port); | ||
| port, | ||
| ConfigurationName); |
There was a problem hiding this comment.
Should be Subsystem.
| ValidatePortInRange(port); | ||
|
|
||
| this.Port = (port != 0) ? port : DefaultPort; | ||
| this.Subsystem = (subsystem != null) ? subsystem : DefaultSubsystem; |
There was a problem hiding this comment.
I think we should check for null or empty here (string.IsNullOrEmpty).
| /// <summary> | ||
| /// This parameter specifies the SSH subsystem to use for the remote connection. | ||
| /// </summary> | ||
| [Parameter(ValueFromPipelineByPropertyName = true, |
There was a problem hiding this comment.
Please put this in the "SSH Parameters" section. This should also include the FilePathSSHHostParameterSet.
Paul Higinbotham (PaulHigin)
left a comment
Paul Higinbotham (PaulHigin)
left a comment
There was a problem hiding this comment.
LGTM
|
Travis Plunk (@TravisEz13) ok to merge? |
|
Steve Lee (@SteveL-MSFT) I updated the title and summary are they correct? |
Steve Lee (SteveL-MSFT)
changed the title
|
Travis Plunk (@TravisEz13) summary and title are now correct |
The current implementation of PSRP over SSH only enables remoting to one version of PowerShell Core 6. sshd_config allows specifying multiple subsystems that can start different versions of PowerShell Core 6 (or same versions with different parameters). Enable use of a new -Subsystem parameter to specify the subsystem to use with ssh.
3 participants
PR Summary
Current implementation of PSRP over SSH only enables remoting to one version of PowerShell Core 6.
sshd_config allows specifying multiple subsystems that can start different versions of PowerShell Core 6 (or same versions with different parameters).
Enable use of a new
-Subsystemparameter to specify the subsystem to use with ssh.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests