Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat(cli): addcoder open vscode#11191

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
mafredri merged 18 commits intomainfrommafredri/feat-cli-open
Jan 2, 2024
Merged

feat(cli): addcoder open vscode#11191

mafredri merged 18 commits intomainfrommafredri/feat-cli-open
Jan 2, 2024

Conversation

mafredri
Copy link
Member

@mafredrimafredri commentedDec 13, 2023
edited
Loading

This PR adds support forcoder open vscode that can be used on a local machine or remote.

On a local machine, the URI is automatically opened via platform specific open methods (open, xdg-open, etc.) and on a remote machine the URI is printed to the console (or if automatic open fails).

As a (maybe overkill?) security precaution, we don't print the API key by default, it has to be requested with--generate-token. The automatic open code-path always includes it, though. Just like the Web UI.

The most unfortunate part of this PR is the path handling (due to Windows). It could be simplified at the loss of convenience, or it could be refactor to be more robust, the latter I don't feel is in scope for this PR, though.

In a future incantation, we may even go as far as SSH into the workspace to expand the path which would resolve a lot of issues -- maybe even allow tab-autocompletion once that's are re-introduced.

Alternatively, in another future improvement, it would be nice if the agent reported the$HOME, this way we'd have a fallback for whenExpandedDirectory is empty.

Fixes#7667

@mafredrimafredriforce-pushed themafredri/feat-cli-open branch 9 times, most recently fromb4abb31 to36c7e20CompareDecember 13, 2023 15:50
@matifali
Copy link
Member

matifali commentedDec 13, 2023
edited
Loading

Do we need to specify workspace name everytime?
What if we run it from an ssh session that is already connected to the workspace?
In that case we should default the current workspace a user is connected to.
Also thought about making itcoder vscode only to make it more concise?
And probably in a follow up we can also havecoder gateway

@matifali
Copy link
Member

Perhaps the above should be behind the verbose flag.

Yes I agree it should be replaced with "Opening vscode desktop in workspace w"

mafredri reacted with thumbs up emoji

@mafredri
Copy link
MemberAuthor

Do we need to specify workspace name everytime? What if we run it from an ssh session that is already connected to the workspace? In that case we should default the current workspace a user is connected to.

This version was intended only for opening from a users local machine. Opening from within the workspace on the local machine is much more complicated, although I agree would be convenient.

Opening from the workplace is a much harder problem since we can’t tell the users machine to execute a command from within the SSH session. The best we can do for now within the workspace is to print the open uri and let the user open it themselves. Or, we can print a URL to the dashboard (solves the issue of providing the token without printing it in the terminal).

If getting the same automatic open experience remotely as locally is a hard requirement. Then I’d like to discuss with@deansheather about security implications and whether or not we should do it.

Also thought about making itcoder vscode only to make it more concise? And probably in a follow up we can also havecoder gateway

I did consider it, but I’m worried we’ll overload the top level scope with too many commands if we do that. Especially if more open commands are introduced.

@matifali
Copy link
Member

I did consider it, but I’m worried we’ll overload the top level scope with too many commands if we do that. Especially if more open commands are introduced.

I agree with naming.

For allowing opening from the web terminal, it's just convenient and saves user from having to install coder locally first before using this command.
I think if we can just print the URI its enough and user can click on it to launch the IDE.

mafredri reacted with thumbs up emoji

@deansheather
Copy link
Member

VSCode itself doesn't support "local open" in an SSH session outside of the integrated terminal so I don't think we should either. You can't SSH into a server and then runcode . to open a native VSCode window locally AFAIK.

There are security issues with adding a reverse shell or support for the workspace to open URLs on the local machine without input as well. A malicious workspace could run custom commands or could abuse custom scheme handlers to coerce other apps on the local system to do malicious actions. Or it could just DoS your local machine by spamming URLs.

mafredri and matifali reacted with thumbs up emoji

@mafredri

This comment was marked as outdated.

@mafredrimafredriforce-pushed themafredri/feat-cli-open branch 4 times, most recently from49025a5 to5d370fcCompareDecember 15, 2023 17:34
@mafredrimafredri marked this pull request as ready for reviewDecember 15, 2023 17:49
Copy link
Member

@matifalimatifali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Great Work. I am happy withe functionality and will defer code review to engineers.

Copy link
Member

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM 👍 I think we could probably extract some util / helper functions from the command though, especially around the windows path handling logic.

// Path starts with a drive letter.
return len(path) == 2 || (len(path) >= 4 && path[2] == '\\' && path[3] == '\\')
default:
return false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I tried/path/to/something,\path\to\something andC:/path/to/something which also both work in VSCode. Defaults to the drive Windows is installed on if there isn't a drive letter.

IDK how paths work in Windows containers either, unsure if they have drive letters or not.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Maybe I'll need to play around with this some more. I was actually planning on testing with agents on Windows today, but it turns out our win template is broken atm. I know at least the "expanded directory" resolves toC:\... via test runners. I suspected/ might work in VS Code but out of precaution I wanted to transform them.

Perhaps we can just make everything slash and only have the logic for detecting abs path on Windows.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I added some handling for the cases you mentioned, and relaxed the abs requirement to include/,\. I know there are more exceptions like\\.UNC\ paths and the like, but I don't know if it's worth adding preemptive handling for that.

Copy link
MemberAuthor

@mafredrimafredriDec 19, 2023
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Actually, I can't seem to make CI happy because Gofilepath.IsAbs on windows seems to think differently?https://github.com/coder/coder/actions/runs/7262677715/job/19786347550?pr=11191#step:5:496

(I kept the fallback tofilepath vs our impl. on Windows so that we could catch these types of inconsistencies, but I can just remove that if we don't care.)

@mafredrimafredriforce-pushed themafredri/feat-cli-open branch 2 times, most recently from7b54988 tocdea111CompareDecember 19, 2023 12:08
@mafredrimafredriforce-pushed themafredri/feat-cli-open branch 3 times, most recently from3c3564a tob1f10a7CompareDecember 19, 2023 12:47
Copy link
Member

@deansheatherdeansheather left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think it's probably fine if we don't cover every single edge case on Windows, especially since not many of our customers are using Windows workspaces. If we do find issues they can be fixed later

@github-actionsgithub-actionsbot added the staleThis issue is like stale bread. labelJan 2, 2024
@matifalimatifali removed the staleThis issue is like stale bread. labelJan 2, 2024
@mafredrimafredri merged commitdf3c310 intomainJan 2, 2024
@mafredrimafredri deleted the mafredri/feat-cli-open branchJanuary 2, 2024 18:46
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJan 2, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@johnstcnjohnstcnjohnstcn approved these changes

@deansheatherdeansheatherdeansheather left review comments

@matifalimatifalimatifali approved these changes

Assignees

@mafredrimafredri

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add a new commandcoder vscode to cli to open vscode

4 participants

@mafredri@matifali@deansheather@johnstcn

[8]ページ先頭

©2009-2025 Movatter.jp