-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Invoke-Command: improve handling of variables with $using: expression #16113
Invoke-Command: improve handling of variables with $using: expression #16113
Conversation
I wonder why we need the workaround if we can prepare full fix. If I understand correctly, with ComputerName we create new runspace but do not open it in |
Implementing proper version detection for the |
Can I get some guidance on the two tests that have failed? It looks like I'd also appreciate any suggestions on writing a Pester test for this issue. I don't think any of the current remoting tests target a remote computer, so they don't offer much guidance. So far I've been testing against computers available to me, but obviously that's too context-dependent to be of use to anyone else. Are there any designated endpoints for this sort of testing? |
I restarted CI-static. |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
@@ -1261,7 +1261,8 @@ internal string ToStringForSerialization(Tuple<List<VariableExpressionAst>, stri | |||
var varAst = ast as VariableExpressionAst; | |||
if (varAst != null) | |||
{ | |||
string varName = varAst.VariablePath.UserPath; | |||
var varPath = varAst.VariablePath; |
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.
Please use the type name instead of var
here
@@ -2329,7 +2330,8 @@ internal string GetParamTextWithDollarUsingHandling(IEnumerator<VariableExpressi | |||
// We are done processing the current ParameterAst | |||
if (astStartOffset >= endOffset) { break; } | |||
|
|||
string varName = varAst.VariablePath.UserPath; | |||
var varPath = varAst.VariablePath; |
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.
Ditto
@@ -2374,7 +2374,8 @@ private string GetConvertedScript(out List<string> newParameterNames, out List<o | |||
|
|||
foreach (var varAst in usingVariables) | |||
{ | |||
string varName = varAst.VariablePath.UserPath; | |||
var varPath = varAst.VariablePath; |
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.
Ditto
Not sure what the state of testing is for |
Tagging @PaulHigin for review as well |
Git/GitHub novice question: when updating from the master branch, I should have used a fast-forward merge to avoid creating a separate merge commit, right? How much of a problem is it that I, uh, didn't do that? |
@dwtaber Never mind - we use "squash and merge" to get one commit in main branch. |
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.
Changes look good to me.
@rjmholt, judging by its file history, it looks like |
@dwtaber Changes from this PR exists in the master branch. The PR will not be included in 7.2.0-rc.1 though. It will be included in 7.3.0-preview.1. |
Ah, okay. Thanks for clarifying! |
🎉 Handy links: |
I really wish this had made it into the stable release of 7.2, I try to avoid using preview-exclusive stuff in my code, so having to wait for 7.3's stable release really sucks. Is there any chance this could be slipped into a 7.2 build, i.e. 7.2.2? |
@PowerShell/powershell-maintainers |
PR Summary
This PR improves
Invoke-Command
's handling of cases that combine$using:
expressions with variable namespace notation when the-ComputerName
parameter is used.Fixes #9204
Fixes #16019
PR Context
When the
-ComputerName
parameter is used,Invoke-Command
is unable to check which version of PowerShell is available on the remote computer and assumes v2.0. Because PowerShell 2.0 can't parse$using:
expressions, theScriptBlock
is modified before sending, with$using:
variables renamed to replace the$using:
expression with$__using_
. Currently, the code that renames these variables doesn't check whether the variable path is qualified. In cases where variable namespace notation is used, this code produces variable names like$__using_env:foo
, preventing the remote computer from parsing the variables correctly. The changes in this PR add handling for qualified variable paths, producing variable names like$__using_env_foo
This PR does not fix all issues with the handling of
$using:
expressions. Variable names that include special characters still aren't parsed correctly, for instance. Since a user technically could name a variable$__using_env_foo
, this might be considered an "unlikely grey area" breaking change. Long-term, it might be worth reconsidering howInvoke-Command
handles the-ComputerName
parameter; always falling back to v2.0 seems unnecessarily restrictive.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).