Auto update IDE settings for serverless ssh mode#4559
Conversation
In serverless ssh mode (ssh connect command without cluster flag and with the ide flag set), we need to set up the desired server ports (or socket connection mode) for the connection to go through (as the majority of the localhost ports on the remote side are blocked by iptable rules). Plus the platform (always linux), and extensions (python and jupyter), to make the initial experience smoother. This is done by checking the IDE settings file and updating it if necessary, prompting the user for confirmation before doing so. If the settings file is not found (fresh IDE install), we create one with the recommended configuration. If there are any other issues during the update process, we log the error and print the manual instructions for the user, proceeding with the connection anyway.
ilia-db
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
ilia-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
Commit: 66ae021
15 interesting tests: 7 KNOWN, 7 SKIP, 1 RECOVERED
Top 26 slowest tests (at least 2 minutes):
|
ilia-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| } | ||
|
|
||
| func saveSettings(path string, settings map[string]any) error { | ||
| data, err := json.MarshalIndent(settings, "", " ") |
There was a problem hiding this comment.
this is quite destructive to the user settings file:
- all user comments in their settings are stripped
- the order of the fields is not preserved
2 suggestions to already existing mitigation of creating a backup file:
- be explicit about the settings file might lose comments and current formatting / ordering
- show a preview to the user of which settings need to be added - so that they have a chance to paste those settings themselves
There was a problem hiding this comment.
Good point, updated the logic to use hujson lib that preserves formatting and comments
| } | ||
|
|
||
| if missing.listenOnSocket { | ||
| settings[listenOnSocketKey] = true |
There was a problem hiding this comment.
is there any way to limit the scope of this setting to the current connection? if not, we might want to be explicit about that this setting is global and might break other connections the user might be using
There was a problem hiding this comment.
There's a way - we can copy user setting into a separate folder, modify them there, and launch vscode/cursor with --user-data-dir argument. We'll have to copy all possible user data to it though - settings, tasks, keybindings, etc. And after that they are fully separate, with no easy way to sync with the main user data. We may want to resort to it, but for now I'll add a warning
| return fmt.Errorf("failed to prompt user: %w", err) | ||
| } | ||
| if !shouldUpdate { | ||
| log.Infof(ctx, "Skipping IDE settings update") |
There was a problem hiding this comment.
if the settings update is skipped i suggest we should still show to the user what settings we are suggesting. otherwise this could lead to a silently broken workflow.
There was a problem hiding this comment.
I'm now showing what settings we plan to update when we ask for agreement
There was a problem hiding this comment.
it would still be good to add a disclaimer in case the user chooses "no", that if their connection does not work they should update their vscode with the settings displayed above
| return nil | ||
| } | ||
|
|
||
| backupPath := path + ".bak" |
There was a problem hiding this comment.
two consequent runs of ssh connect might leave user with no good backup of their settings, we can improve here by ensuring that at least one backup is stored on user's disk keeping the settings as they were before user started using this command.
| return fmt.Errorf("failed to marshal settings: %w", err) | ||
| } | ||
|
|
||
| if err := os.WriteFile(path, data, 0o600); err != nil { |
There was a problem hiding this comment.
we should acquire the file lock before writing here to prevent data loss on concurrent writes
There was a problem hiding this comment.
We can add file locking with platform specific APIs (or using ready made lib for it), plus add --force-lock option as an escape hatch. It's heavy handed approach and doesn't seem necessary given the use-case, but we can revisit it later on
ilia-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilia-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilia-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilia-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilia-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
anton-107
left a comment
anton-107
left a comment
There was a problem hiding this comment.
LGTM, left a few nit comments
| return fmt.Errorf("failed to prompt user: %w", err) | ||
| } | ||
| if !shouldUpdate { | ||
| log.Infof(ctx, "Skipping IDE settings update") |
There was a problem hiding this comment.
it would still be good to add a disclaimer in case the user chooses "no", that if their connection does not work they should update their vscode with the settings displayed above
|
|
||
| if err := os.WriteFile(path, data, 0o600); err != nil { | ||
| func saveSettings(path string, v *hujson.Value) error { | ||
| v.Format() |
There was a problem hiding this comment.
nit: this formats the entire file (so it might be annoying to those user that take extra care their settings.json), ideally we can skip this and only format our own patched segment
| lines = append(lines, fmt.Sprintf(" \"%s\": {\"%s\": \"%s\"}", remotePlatformKey, connectionName, remotePlatform)) | ||
| } | ||
| if missing.listenOnSocket { | ||
| lines = append(lines, fmt.Sprintf(" \"%s\": true // Global setting that affects all remote ssh connections", listenOnSocketKey)) |
There was a problem hiding this comment.
nit: this is a bit confusing choice of message format - i was expecting this comment to add up in my settings file (since VSCode supports comments there), but the comment is not being saved
There was a problem hiding this comment.
It's mainly to get the message across during the approval prompt, I don't think it's necessary in the file itself.
…t work as expected
ilia-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
To avoid user-defined formatting of the settings JSON.
ilia-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
3 participants
Changes
In serverless ssh mode (ssh connect command without cluster flag and with the ide flag set), we need to set up the desired server ports (or socket connection mode) for the connection to go through (as the majority of the localhost ports on the remote side are blocked by iptable rules). Plus the platform (always linux), and extensions (python and jupyter), to make the initial experience smoother.
This is done by checking the IDE settings file and updating it if necessary, prompting the user for confirmation before doing so.
If the settings file is not found (fresh IDE install), we create one with the recommended configuration. If there are any other issues during the update process, we log the error and print the manual instructions for the user, proceeding with the connection anyway.
Tests
New unit tests, plus manual tests on all platforms (WIP)