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

Validate configuration and publish diagnostics #27

Closed
radeksimko opened this issue Mar 14, 2020 · 2 comments · Fixed by #323
Closed

Validate configuration and publish diagnostics #27

radeksimko opened this issue Mar 14, 2020 · 2 comments · Fixed by #323
Assignees

Comments

@radeksimko
Copy link
Member

radeksimko commented Mar 14, 2020

Currently the language server ignores any potential errors or warnings.

In order for completion (and other future features) to work this is essential as we need to expect user will want to use LS features in incomplete and otherwise invalid configs and HCL has the ability to deal with such scenarios.

That said the user would benefit from having a feedback in the form of errors/warnings when their config is not up to scratch.

As part of this we need to assess what diagnostics and when to publish.

What

  • HCL parse errors
  • provider validation
  • anything else that terraform validate may run - perhaps consider even running that directly

terraform validate

terraform validate can parse config and provide this data in JSON readable format (via -json flag).
The only caveat is that it also reports what could be interpreted as false negatives in the context of the language server.

This is because the command was originally designed to test preparedness for plan/apply and as such it will also report missing required variables that could be provided as ENV variables in CI or elsewhere - generally somewhere outside the context of an editor and language server.

For these reasons we may need to suppress such errors, or just find a different way of validating configs.

Relatedly integrating validate at this point would be setting a new precedent of Terraform actually parsing the config and Terraform's parsing logic may differ (hopefully doesn't though) from how the language server one. It is therefore important to integrate validation with this in mind.

When

  • on textDocument/didChange
    • this could be potentially resource-intensive depending on how often do Language Clients send changes. Reparsing configuration on every keystroke may not be a good idea.
    • may not provide the best UX as the user is in the process of crafting a valid config and so they are likely to be aware that it's not valid yet.
  • on textDocument/didSave, textDocument/willSave or textDocument/willSaveWaitUntil
    • this could leave the user potentially puzzled while looking at invalid config wondering why it is invalid

Generally for when I think that some user testing/surveying should happen so we know when users expect their configs to be valid and when do they expect to receive feedback if it is not valid.

@radeksimko radeksimko changed the title Publish warnings & errors via textDocument/publishDiagnostics Validate configuration and publish diagnostics Apr 11, 2020
@radeksimko radeksimko added the enhancement New feature or request label May 6, 2020
@apparentlymart
Copy link

I don't know that it really changes anything about what you described here, but I just wanted to note that terraform validate is intended for use-cases like this (albeit more like "validate on save" than ongoing checking with language server, because there was no such thing as a language server when we designed it that way) and we consider the errors that appear for provider configurations to be a bug, tracked by hashicorp/terraform#21408.

I don't know if it would help given the rest of the constraints here, but I'd be open to considering fixing that bug to be part of the solution to this issue, if that's helpful.

@ghost
Copy link

ghost commented Jan 7, 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 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants