-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Invoke-Command $using improvements #24025
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
aa0bdbc
to
c61690c
Compare
c61690c
to
191a1ce
Compare
191a1ce
to
3cb92a1
Compare
fa5b5fa
to
af25933
Compare
The @PowerShell/wg-powershell-cmdlets discussed this. Although you have provided a technical solution, we don't believe this is a real world problem. We expect users to either install Windows PowerShell 5.1 or PowerShell 7 . Windows Server 2012/2012R2 end of life was Oct 2023. Windows Server 2016+ has 5.1. Alternatively, users who hit this problem likely already have implemented the workarounds. Although the change itself is small, we don't believe we should add support for PSv3/4 into PS7. It may make sense to explicitly add the workaround example in our documentation. |
@SteveL-MSFT I think you may have misunderstood what this PR is trying to do. The problem is that PowerShell today expects that the remote target is running v2 so that if
This does not fix support for older version but rather drops support for older versions so that when doing |
@jborean93 yes, it seems we misinterpreted the intent of this PR. I've marked it to discuss again in next WG meeting which will be mid-Jan |
Thanks, while discussing it then could the WG decide whether the baseline for WSMan should be either the V3/4 one as done in this PR or V5? I stuck with V3/4 because 2012/2012 R2 is still supported in Azure and it solves the immediate problem of referencing functions with |
The @PowerShell/wg-powershell-cmdlets discussed this again with the understanding that the intent is to remove support for v2 (which is completely out of support now) and enhance behavior for v3/4 so that users don't have to make the workaround which is not obvious. So there isn't a breaking change here since it used to not work (without the workaround) and now will work as expected from a PS7 client to a v3/4 target. |
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.
Looks good. Left 1 comment.
Always use the v5.1 $using: logic when dealing with any connection type that is not based on the WSManConnectionInfo. This allows the caller to use the more advanced $using features like vars with special names, $using vars inside sub scopes and more. WSMan based connections might still communicate with older versions but as the oldest supported version is v3 on Server 2012 we don't need to keep the default as v2 to support some of the newer features.
I've done a rebase push as this branch was very much out of date. |
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
Co-authored-by: Travis Plunk <travis.plunk@microsoft.com>
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
📣 Hey @@jborean93, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
Thanks for the review/merge @daxian-dbw |
PR Summary
Always use the v5.1 $using: logic when dealing with any connection type that is not based on the WSManConnectionInfo. This allows the caller to use the more advanced $using features like vars with special names, $using vars inside sub scopes and more. WSMan based connections might still communicate with older versions but as the oldest supported version is v3 on Server 2012 we don't need to keep the default as v2 to support some of the newer features.
PR Context
There are three different ways that
Invoke-Command
deals with$using:
inside a ScriptBlock:$using:var
to$__using_var
and modifies the param block to pass in that variable$using:var
and passes it to the remote pipeline through--%
$using:
in child scopesThe V2 method is quite rudimentary, it fails when using a variable with an invalid char in like
-
, or in a very improbable scenario someone named their var as$__using_var
themselves. The-
problem is typically encountered when you try to inject a function into the scriptblock, for example this fails todayThe workaround is to store the function in an intermediate var or define the function without
-
in the name:The V3/4, V5 logic works just fine with vars with special chars in the name
The current logic for determining what logic to use is as follows
NewProcessConnectionInfo
use V5Start-Job
or if using-Session $s
where the$s
is fromNew-PSSession -UseWindowsPowerShell
PSSession
through the-Session
parameter use V2PSSession
supports disconnection operationsPSVersionTable.PSVersion
to determine the versionThis means that V3/4, V5 is only used when the
PSSession
fromNew-PSSession -UseWindowsPowerShell
orNew-PSSession -ComputerName ...
is given toInvoke-Command -Session ...
. The more common scenarios likeInvoke-Command -ComputerName ...
orInvoke-Command -HostName ...
will always use the old V2 logic and thus cannot use the newer$using:
features.This PR simplifies the checks to set the baseline to V3/4 for a WSMan based connection and treat the rest of the connection types as V5. This is because only the WSMan connection was supported before 5.1 and can guarantee the target is PowerShell 5.1 or newer. The baseline for WSMan was set to V3/4 as V2 was last in box with Windows 7/Server 2008/08 R2 which is end of life. V3/4 is also strictly end of life but considering it's still supported in Azure I didn't think it was time to drop that just yet.
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.$using:
- [ ] Issue filed:
(which runs in a different PS Host).