-
Notifications
You must be signed in to change notification settings - Fork 356
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
chore: Rename show_ssh_command in CLI and add message for VSCode WSL Users #8387
Conversation
✅ Deploy Preview for determined-ui canceled.
|
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.
suggested edits
cc @ighodgao |
docs/integrations/ide/vscode.rst
Outdated
############################ | ||
Addendum for Windows Users | ||
############################ |
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.
are these instructions really a good idea?
To me, these instructions are basically so difficult that we may as well just say "you can't run --show-ssh-command from inside wsl; that's not supported". Having to follow these instructions for every shell a user starts is crazy.
Alternatively, we could detect if we were inside wsl in the python code:
if "WSL" in os.uname().release:
...
Though I'm not sure if this affects enough users to fix it. It probably does affect enough users that we could emit a warning when somebody asks for --show-ssh-command within WSL though, that's probably the lower-limit on what a fix should entail.
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 there's value in having documentation for users who REALLY want to use this feature. I like adding the warning and adding a disclaimer to the docs along the lines of "we do not recommend doing this"
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'd much prefer telling those users "install just determined in not-wsl for this feature". That's two sentences and it's actually usable for end users.
harness/determined/cli/shell.py
Outdated
@@ -90,6 +90,14 @@ def show_ssh_command(args: Namespace) -> None: | |||
) | |||
|
|||
|
|||
@authentication.required |
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 don't think this is necessary, since the next function you call is already wrapped (right?)
I think it might actually cause additional network traffic, so I think it's not harmless.
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.
wait... did the warning for vscode users disappear somewhere? I swear you had it written once but I don't see it now...
Description
Windows Users are having issues with VSCode remote-ssh integration. This is due to Windows defaulting to CMD with OpenSSH instead of using WSL bash and the proper ssh tool for the job. Adding warning to CLI and also changing
det shell show_ssh_command
todet shell show-ssh-command
Test Plan
Start a remote shell.
Be a Windows user
det shell show-ssh-command
should show a VSCode-specific warning in WSL shell.Be any user
det shell show_ssh_command
should show a deprecation warning.Commentary (optional)
This should address #7726
Checklist
docs/release-notes/
.See Release Note for details.
Ticket
MLG-871