-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Adding Working Directory to Start-Job cmdlet #10274
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1188,6 +1188,12 @@ public virtual ScriptBlock ScriptBlock | |
|
|
||
| private ScriptBlock _scriptBlock; | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the initial location of the script block. | ||
| /// This is used to set the location prior to executing the script block. | ||
| /// </summary> | ||
| public virtual string WorkingDirectory { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The file containing the script that the user has specified in the | ||
| /// cmdlet. This will be converted to a powershell before | ||
|
|
@@ -1843,6 +1849,13 @@ internal Pipeline CreatePipeline(RemoteRunspace remoteRunspace) | |
|
|
||
| pipeline.Commands.Clear(); | ||
|
|
||
| if (WorkingDirectory != null) | ||
| { | ||
| var command = new Command("Set-Location"); | ||
| command.Parameters.Add("LiteralPath", WorkingDirectory); | ||
| pipeline.Commands.Add(command); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something about this doesn't feel right. Starting with the test: Start-Job -ScriptBlock { Join-Path $pwd "" } -WorkingDirectory $tempPath I think this code does the equivalent of: Set-Location $tempPath | Join-Path $pwd ""Instead of: Set-Location $tempPath
Join-Path $pwd ""I think you need to call
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly what you are saying is that the object This means that the correct approach is to have something like this: _powershellV3 = System.Management.Automation.PowerShell.Create();
if (WorkingDirectory != null)
{
_powershellV3.AddStatement().
AddCommand("Microsoft.PowerShell.Management\\Set-Location").
AddParameter("LiteralPath", WorkingDirectory);
}I would assume that this statement should be added for both when the server is ps2 and ps5+. (P.S. thanks for the comment it helped me understand the code way better)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think what @lzybkr is trying to draw your attention to is what happens if
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Some secure endpoints disallow script (no language mode), so the I think in V3 we added support for multiple distinct statements via the _powershellV3.
AddCommand("Microsoft.PowerShell.Management\\Set-Location").
AddParameter("LiteralPath", WorkingDirectory).
AddStatement();I do see that the test passed though, and that surprised me. I'd do some more testing to verify my assumptions above. I don't recall specifics about going between the PowerShell api and the Pipeline api, and to be honest, I find the original code here (before the PR) a little confusing, so I'm reluctant to say that I should also mention the need for compatibility testing if |
||
| } | ||
|
|
||
| foreach (Command command in powershellToUse.Commands.Commands) | ||
| { | ||
| pipeline.Commands.Add(command); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -480,6 +480,24 @@ public virtual ScriptBlock InitializationScript | |
|
|
||
| private ScriptBlock _initScript; | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets a working directory of the process. | ||
| /// </summary> | ||
| [Parameter] | ||
| [ValidateNotNullOrEmpty] | ||
| public override string WorkingDirectory | ||
| { | ||
| get | ||
| { | ||
| return base.WorkingDirectory; | ||
| } | ||
|
|
||
| set | ||
| { | ||
| base.WorkingDirectory = value; | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be simplified to: get => base.WorkingDirectory;
set => base.WorkingDirectory = value;
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No strong opinion here but: I am tempted to leave this as it is and the reason for that is the rest of the file that I am touching follows this pattern for the get\sets source
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally speaking from what I've seen, when most contributors add new code, they do so using the current practices rather than match existing syntax. This keeps us forward moving. Eventually PRs come in to advance old syntax to catch up to newer syntax.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I will fix it in the next iteration |
||
| } | ||
|
|
||
| /// <summary> | ||
| /// Launches the background job as a 32-bit process. This can be used on | ||
| /// 64-bit systems to launch a 32-bit wow process for the background job. | ||
|
|
@@ -589,6 +607,18 @@ protected override void BeginProcessing() | |
| ThrowTerminatingError(errorRecord); | ||
| } | ||
|
|
||
| if (WorkingDirectory != null && !Directory.Exists(WorkingDirectory)) | ||
| { | ||
| string message = StringUtil.Format(RemotingErrorIdStrings.StartJobWorkingDirectoryNotFound, WorkingDirectory); | ||
| var errorRecord = new ErrorRecord( | ||
| new DirectoryNotFoundException(message), | ||
| "DirectoryNotFoundException", | ||
| ErrorCategory.InvalidOperation, | ||
| targetObject: null); | ||
|
|
||
| ThrowTerminatingError(errorRecord); | ||
| } | ||
|
|
||
| CommandDiscovery.AutoloadModulesWithJobSourceAdapters(this.Context, this.CommandOrigin); | ||
|
|
||
| if (ParameterSetName == DefinitionNameParameterSet) | ||
|
|
@@ -623,7 +653,7 @@ protected override void CreateHelpersForSpecifiedComputerNames() | |
| ErrorCategory.PermissionDenied, | ||
| Context.LanguageMode)); | ||
| } | ||
|
|
||
davinci26 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| NewProcessConnectionInfo connectionInfo = new NewProcessConnectionInfo(this.Credential); | ||
| connectionInfo.InitializationScript = _initScript; | ||
| connectionInfo.AuthenticationMechanism = this.Authentication; | ||
|
|
@@ -633,14 +663,14 @@ protected override void CreateHelpersForSpecifiedComputerNames() | |
| Utils.GetTypeTableFromExecutionContextTLS()); | ||
|
|
||
| remoteRunspace.Events.ReceivedEvents.PSEventReceived += OnRunspacePSEventReceived; | ||
|
|
||
| Pipeline pipeline = CreatePipeline(remoteRunspace); | ||
|
|
||
| IThrottleOperation operation = | ||
| new ExecutionCmdletHelperComputerName(remoteRunspace, pipeline); | ||
|
|
||
| Operations.Add(operation); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// The expression will be executed in the remote computer if a | ||
| /// remote runspace parameter or computer name is specified. If | ||
|
|
@@ -724,7 +754,7 @@ protected override void ProcessRecord() | |
| if (_firstProcessRecord) | ||
| { | ||
| _firstProcessRecord = false; | ||
|
|
||
| PSRemotingJob job = new PSRemotingJob(ResolvedComputerNames, Operations, | ||
| ScriptBlock.ToString(), ThrottleLimit, _name); | ||
|
|
||
|
|
||
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.
This should probably the module, like: