-
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
Add -StrictMode
to Invoke-Command
to allow specifying strict mode when invoking command locally
#16545
Add -StrictMode
to Invoke-Command
to allow specifying strict mode when invoking command locally
#16545
Conversation
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 overall!
I left some feedback on how you can just use the StrictModeVersion property (instead of creating a field since you don't have any additional logic for this property's setter method and can just use the default setter with the property. @PaulHigin if using the Property instead of additionally creating a field isn't recommended practice in PowerShell project specifically please let me know.
EDIT: since older PowerShell classes use field backing it's up to the author to make this change if they wish but not neccessary.
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Show resolved
Hide resolved
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.
Sorry, I didn't notice that these comments were not yet submitted.
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
a453e4d
to
a734fc5
Compare
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
test/powershell/Modules/Microsoft.PowerShell.Core/Invoke-Command.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Core/Invoke-Command.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Core/Invoke-Command.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Core/Invoke-Command.Tests.ps1
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
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
@Thomas-Yu Thanks for your contribution!
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Core/Invoke-Command.Tests.ps1
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
Outdated
Show resolved
Hide resolved
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.
For my part I am good with these changes.
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) |
@@ -263,6 +263,72 @@ public override SwitchParameter UseSSL | |||
} | |||
} | |||
|
|||
private sealed class ArgumentToPSVersionTransformationAttribute : ArgumentToVersionTransformationAttribute |
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.
@Thomas-Yu , @daxian-dbw For the record, I don't think it was necessary to split the attribute/conversion classes into two separate implementations. Soon,Set-StrictMode
will also take the off
value for strict mode version, which means merging these two classes back together. In the mean time, there was no harm in having one set of attribute/conversion implementations. Now when Set-StrictMode
is updated to also support off
this work will have to be refactored yet again.
@Thomas-Yu Please create an Issue in PowerShell repo that adds off
to the Set-StrictMode -Version
parameter. The committee would like to see it work the same as for Invoke-Command
here for consistency, although it is not necessary to include the changes in this PR.
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. @Thomas-Yu Thank you for your contribution!
Some of my comments may have resulted in contradictions throughout the review, sorry about that and thanks for your patience!
@PaulHigin, the issue #16736 has been opened by @Thomas-Yu per your ask. I will merge this PR once you approve it. |
-StrictMode
to Invoke-Command
to allow specifying strict mode when invoking command locally
… when invoking command locally (PowerShell#16545)
🎉 Handy links: |
PR Summary
Fix #2692
Add
-StrictMode
toInvoke-Command
to allow specifying strict mode when invoking command locally.The
-StrictMode
parameter sets the provided version to the process. Once the process completes, theStrictMode
version is set back to what it was before theInvoke-Command
.It does not support asynchronous jobs on remote machines. Applying a
StrictMode
version for a remote session is not a trivial change. TheStrictMode
version would have to be communicated to the target so that it can be applied to the Runspace in the remote session.In addition to the implementation, an
Invoke-Command
test file was created with-StrictMode
tests.PR Context
This pull request was opened in response to the "Invoke-Command should support -StrictMode #2692" issue. Now, Invoke-Command supports a
-StrictMode
parameter.StrictMode
establishes and enforces coding rules in expressions, scripts, and script blocks. This can be useful for running scripts with basic best-practice coding rules. Even though this is not an implementation for async jobs, there are still many useful applications forStrictMode
in local processes.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).