Skip to content
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

Implement workspace/executeCommand #81

Closed
radeksimko opened this issue Apr 30, 2020 · 7 comments
Closed

Implement workspace/executeCommand #81

radeksimko opened this issue Apr 30, 2020 · 7 comments
Assignees
Labels
enhancement New feature or request workspace

Comments

@radeksimko
Copy link
Member

Current Version

v0.1.0

Use-cases

In most cases server should facilitate the execution of Terraform and expose it in the form of relevant LSP methods, e.g. terraform fmt is essentially wrapped in textDocument/formatting.

However there will be valid cases where the client may need to execute commands on behalf of the user, e.g. when they pick an item from command palette. For example terraform init.

Attempted Solutions

Client can execute commands on its own, which risks drift in how Terraform is found and executed.

Proposal

The client should execute these commands via server - via workspace/executeCommand method, to ensure that there's agreement on how to find the right terraform binary and how it gets executed (with what ENV variables etc.).

Related LSP methods

@radeksimko radeksimko added the enhancement New feature or request label Apr 30, 2020
@radeksimko radeksimko added this to the v0.3.0 milestone Apr 30, 2020
@aeschright
Copy link
Contributor

Per this PR, I can see that we'll need to decide what info, if any, the LS will return to the client when those commands are run.

@radeksimko
Copy link
Member Author

radeksimko commented May 6, 2020

Good point @aeschright

Before I looked at the docs more closely I was more expecting the LSP spec to dictate a standard response interface with stdout, stderr, error and exit code. I'm not sure why is the interface so loose, but this is what I'd expect the implementation to return in one way or another.

Given that this is apparently treated by LSP as "implement it your way" as long as server and client agree, then I'm thinking whether we could also be whitelisting some commands, like terraform, as a precaution? I can't think of a way this could be abused, but I feel it's usually better safe than sorry?

@aeschright
Copy link
Contributor

My feeling is that it'll work best if we pick a small set of commands to explicitly enable -- I think I need to have it in the extension's package.lock in addition to the server registering them. And I'll pipe the output into the client's output channel?

@paultyng paultyng modified the milestones: v0.3.0, v0.4.0, v0.5.0 Jun 5, 2020
@radeksimko
Copy link
Member Author

radeksimko commented Jul 1, 2020

It looks like textDocument/codeAction is available so that client can ask the server what actions/commands are supported, before sending any workspace/executeCommand.

I suppose the result of textDocument/codeAction could also be somehow presented back to the user in the form of commands in the command palette.

This is also how gopls seems to be communicating its "execute capabilities", I saw this line in the log:

LSP: gopls: Supported execute commands: ['tidy', 'upgrade.dependency', 'generate']

@radeksimko
Copy link
Member Author

Also - and that's probably even more relevant, but unsure how it correlates with textDocument/codeAction, the server can send commands it supports in the initialize response as a server capability:

export interface ExecuteCommandOptions extends WorkDoneProgressOptions {
	/**
	 * The commands to be executed on the server
	 */
	commands: string[]
}

interface ServerCapabilities {
// ...
	/**
	 * The server provides execute command support.
	 */
	executeCommandProvider?: ExecuteCommandOptions;
// ...
}

@aeschright aeschright self-assigned this Oct 2, 2020
@aeschright
Copy link
Contributor

Closed by #274 and hashicorp/vscode-terraform#495

@ghost
Copy link

ghost commented Jan 17, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the context necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request workspace
Projects
None yet
Development

No branches or pull requests

3 participants