-
Notifications
You must be signed in to change notification settings - Fork 12
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
Wider vscode support #98
Conversation
} | ||
await remoteSSHconfig.update('configFile', localAppSSHConfigPath, vscode.ConfigurationTarget.Global); | ||
private async ensureRemoteSSHExtInstalled(flow: UserFlowTelemetryProperties): Promise<boolean> { | ||
const isOfficialVscode = vscode.env.uriScheme === 'vscode' || vscode.env.uriScheme === 'vscode-insiders'; |
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.
Should not we ensure that yours extension is installed then? Should we also collect your extension version then too as part of error reporting/tracing/analytics?
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.
it's tricky because cursor forked my extension so it has another id 🤦
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 mean we can have a case if it is not cursor then?
But the best would be that Cursor folks contribute back to your extension and don't cause discrepancy cc @mbrevoort if you know whom to cc :)
@@ -177,11 +178,12 @@ export class RemoteService extends Disposable implements IRemoteService { | |||
await SSHConfiguration.saveGitpodSSHConfig(gitpodConfig); | |||
} | |||
|
|||
private getHostSSHConfig(host: string, launcher: string, proxyScript: string, extIpcPort: number, logLevel:string) { | |||
private getHostSSHConfig(host: string, launcher: string, proxyScript: string, extIpcPort: number, logLevel: string) { | |||
const extraArgs = (process.versions['electron'] && process.versions['microsoft-build']) ? '--ms-enable-electron-run-as-node' : ''; |
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.
should not we rather rely on vscode.env.uriScheme === 'vscode' || vscode.env.uriScheme === 'vscode-insiders'? and encapsualte it somewhere?
I'm not sure about stability of microsoft-build
version
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 check is done in vscode source code too so it should be stable I guess
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.
What is drawback not relying on it but on stable uriScheme?
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 both are reliable, though uriScheme can be changed in product.json manually
No description provided.