-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Fix implicit remoting proxy cmdlets to act on common parameters #20367
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.
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
Conversation
foreach (string commonParameterName in Cmdlet.CommonParameters) | ||
// need to exclude `ProgressAction` which may not exist for downlevel platforms | ||
var commonParameters = Cmdlet.CommonParameters; | ||
commonParameters.Remove("ProgressAction"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The calling to
Remove
here removesProgressAction
from the static hash sets_commonParameters
. It will likely cause problems with other places that depends ons_commonParameters
. - We should ignore
ProgressAction
only if the server side is a down-level version that doesn't support this common parameter. You may be able to use the same check code from here:
PowerShell/src/Microsoft.PowerShell.Commands.Utility/commands/utility/ImplicitRemotingCommands.cs
Lines 1576 to 1581 in 963bf1e
// For remote PS version 5.1 and greater, we need to include the new -PowerShellVersion parameter if ((Session.Runspace is RemoteRunspace remoteRunspace) && (remoteRunspace.ServerVersion != null) && (remoteRunspace.ServerVersion >= new Version(5, 1))) { powerShell.AddParameter("PowerShellVersion", PSVersionInfo.PSVersion); }
…remove progressaction for older PS
The 1 CodeFactor issue is by-design |
src/Microsoft.PowerShell.Commands.Utility/commands/utility/ImplicitRemotingCommands.cs
Outdated
Show resolved
Hide resolved
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR Summary
The PR to add
ProgressAction
common parameter caused this regression due to an internal helper to determine if the proxy is for a cmdlet. It expected CommonParameters to never change so if they differ, then the parameter is treated as custom so it doesn't work as expected. This works remoting from latest PS7 to latest PS7 since the common parameters match. However, latest to older PS7 or WinPS5.1 (like via WinCompat layer) fails as the newProgressAction
isn't found on the target.Fix is to exclude
ProgressAction
from this check to enable it to work for the other common parameters.PR Context
Fix #20209
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).