fix *-Content calls in psedit scripts and set ComputerName default #599
Conversation
| $params = @{ Path=$filePathName; Raw=$true } | ||
| if ($PSVersionTable.PSEdition -eq 'Core') | ||
| { | ||
| $params += @{ AsByteStream=$true } |
There was a problem hiding this comment.
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.
| @@ -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 }"); | |||
There was a problem hiding this comment.
When connected to a remote session on Linux or macOS, does ComputerName contain anything? Aside from that I think this change is fine
There was a problem hiding this comment.
So, when I tested on linux ConnectionInfo was null.
I could include a check for ComputerName being null to be safe.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm pretty sure the only time ConnectionInfo is set is when it's WSManConnectionInfo. Frustratingly, even NamedPipeConnectionInfo never gets set 😕
There was a problem hiding this comment.
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
}There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
[Environment]::MachineName works on macOS and 5.1 too!
@daviwil I thought PSRP in vscode-powershell wasn't allowed in PS 3 and 4?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
PS 3/4 have [Environment]::MachineName 🎉
Let's go with that.
There was a problem hiding this comment.
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! 😃
|
Moving this comment out of the outdated comment... Ok @daviwil, @rkeithhill, @SeeminglyScience! I changed |
|
|
This change fixes an issue where SQL's SSASCMDLETS module cannot be loaded into the interactive console because the host currently has a pre-1.0 version number. For now we will fix this by reporting our version as 1.0 since we're pretty close to 1.0 anyway.
Fix PowerShell#599: SSASCMDLETS module cannot be loaded
*-Content -Encoding Bytewas changed to*-Content -AsByteStreamin PSCore. This PR adds a check for the version of PowerShell... and uses the correct param.Also, since
$env:ComputerNameis not set on Linux or Mac, we get null ref exceptions so we are using[Environment]::MachineNameinstead and defaulting to "localhost" if it does not exist.This allowed me to remote edit/debug a file on an Ubuntu VM from my Mac 🎉
Resolves #597
Resolves PowerShell/vscode-powershell#789