-
Notifications
You must be signed in to change notification settings - Fork 121
remote ssh: add custom serverInstallPath setting #10017
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
base: main
Are you sure you want to change the base?
Conversation
E2E Tests 🚀 |
|
||
let commandOutput: { stdout: string; stderr: string }; | ||
if (platform === 'windows') { | ||
// If the default was not changed, adjust the path for PowerShell on Windows |
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.
We don't support windows anyway
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.
did not try it out, but the code looks good! happy to test it out on a daily build, since that seems maybe more straightforward? or, are you able to give repro steps?
const productJson = await getVSCodeProductJson(); | ||
|
||
const customServerBinaryName = vscode.workspace.getConfiguration('remoteSSH.experimental').get<string>('serverBinaryName', ''); | ||
const customDataFolderName = vscode.workspace.getConfiguration('remoteSSH').get<string>('serverInstallPath', ''); |
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.
SUPER SUPER nit, could set default here rather than later
const customDataFolderName = vscode.workspace.getConfiguration('remoteSSH').get<string>('serverInstallPath', ''); | |
const serverDataFolderName = vscode.workspace.getConfiguration('remoteSSH').get<string>('serverInstallPath', productJson.serverDataFolderName); |
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.
I think this needs just a few more tweaks:
- it is pretty easy to just type something like
.positron-server-2
into the setting if you aren't reading carefully, and the behavior is undefined in this case. If the user doesn't specify an absolute path, I think we should resolve the relative path against$HOME
by default - the setting is application scoped but this feels more like a per project/machine thing b/c you probably won't have the same needs for everything you connect to. maybe change the scope? another idea would be to make the value something you can override with an env var so it could be configured on the host side
- if the user specifies a path that they can't write to and we can't make for them, what happens? (think we need better error handling for this than we have today)
- do we actually expand tildes in the setting value? (wasn't clear to me but the default uses
$HOME
; if not, the example shouldn't use them)
Addresses #9013. Adds a
remoteSSH.serverInstallPath
setting that lets you customize the Positron server data directory, instead of the default~/.positron-server
.Release Notes
New Features
remoteSSH.serverInstallPath
setting lets you customize the directory for the Positron server Provide custompositron-server
install path? #9013Bug Fixes
QA Notes
The new setting should be respected. To test on this branch, you'll unfortunately need to build REH on the remote host and put it where it needs to be. It'll be easier to test on a daily build.