Skip to content

fix *-Content calls in psedit scripts and set ComputerName default #599

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 8, 2018
Merged

fix *-Content calls in psedit scripts and set ComputerName default #599

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a3393a1
fix *-Content calls in psedit scripts and set ComputerName default
TylerLeonhardt Jan 5, 2018
764db94
fix splatting
TylerLeonhardt Jan 6, 2018
1ff410d
change to MachineName
TylerLeonhardt Jan 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions src/PowerShellEditorServices/Session/RemoteFileManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,17 @@ public class RemoteFileManager
$filePathName = $_.FullName

# Get file contents
$contentBytes = Get-Content -Path $filePathName -Raw -Encoding Byte
$params = @{ Path=$filePathName; Raw=$true }
if ($PSVersionTable.PSEdition -eq 'Core')
{
$params += @{ AsByteStream=$true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be a bit more performant (and PS canonical IMO) to do this: $params['AsByteStream']=$true. This way we modify the original hashtable without creating a new to merge with the original one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated 🙂

}
else
{
$params += @{ Encoding='Byte' }
}

$contentBytes = Get-Content @params

# Notify client for file open.
New-Event -SourceIdentifier PSESRemoteSessionOpenFile -EventArguments @($filePathName, $contentBytes) > $null
Expand Down Expand Up @@ -85,7 +95,18 @@ public class RemoteFileManager
[byte[]] $Content
)

Set-Content -Path $RemoteFilePath -Value $Content -Encoding Byte -Force 2>&1
# Set file contents
$params = @{ Path=$RemoteFilePath; Value=$Content; Force=$true }
if ($PSVersionTable.PSEdition -eq 'Core')
{
$params += @{ AsByteStream=$true }
}
else
{
$params += @{ Encoding='Byte' }
}

Set-Content @params 2>&1
";

#endregion
Expand Down
2 changes: 1 addition & 1 deletion src/PowerShellEditorServices/Session/SessionDetails.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public static PSCommand GetDetailsCommand()
{
PSCommand infoCommand = new PSCommand();
infoCommand.AddScript(
"@{ 'computerName' = $env:ComputerName; 'processId' = $PID; 'instanceId' = $host.InstanceId }");
"@{ 'computerName' = if ($ExecutionContext.Host.Runspace.ConnectionInfo) {$ExecutionContext.Host.Runspace.ConnectionInfo.ComputerName} else {'localhost'}; 'processId' = $PID; 'instanceId' = $host.InstanceId }");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When connected to a remote session on Linux or macOS, does ComputerName contain anything? Aside from that I think this change is fine

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, when I tested on linux ConnectionInfo was null.

I could include a check for ComputerName being null to be safe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a remote session to a Linux machine ConnectionInfo was null? Pretty unfortunate if so, though I suppose $env:ComputerName never worked on Linux anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure the only time ConnectionInfo is set is when it's WSManConnectionInfo. Frustratingly, even NamedPipeConnectionInfo never gets set 😕

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about [Environment]::MachineName? Looks like it works as expected on my Ubuntu box at least. Maybe something like this just in case it's empty on some distros

if ([string]::IsNullOrWhitespace([Environment]::MachineName)) {
    'localhost'
} else {
    [Environment]::MachineName
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, [Environment]::MachineName seems good to me too, but I'd check to make sure it also works on PowerShell 3-5.1 just in case. I dunno if we can assume 'localhost' as default since we also run this on remote sessions. I think if it turns up empty we're in trouble no matter what :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Environment]::MachineName works on macOS and 5.1 too!

@daviwil I thought PSRP in vscode-powershell wasn't allowed in PS 3 and 4?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remoting works in PS4, but we should make sure that it works on the local machine for PS3/4 too. Though if it works in PS 5.1, I would assume it works in PS 3/4 too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS 3/4 have [Environment]::MachineName 🎉

Let's go with that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I changed it to [Environment]::MachineName. I kept the if just in case. Let me get some 👍's from you guys and I'll merge this it! 😃


return infoCommand;
}
Expand Down