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

Replace internal decoder with hcl-lang #281

Merged
merged 2 commits into from
Nov 6, 2020
Merged

Replace internal decoder with hcl-lang #281

merged 2 commits into from
Nov 6, 2020

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Oct 30, 2020

End-User Impact

The implementation generally enables a "progressively enhanced" experience. In practice this means:

  • Completion is available as soon as the server starts - so the user is never left with zero completion, but the data available is gradually enriched between these stages:
    • (1) universal schema which covers any 0.12+ version (just resource, data, and provider for now, but we can expand later)
    • (2) version-specific core schema (once terraform version ran)
    • (3) provider schemas (once terraform providers schema -json ran)
  • Symbol names no longer use any kind of "address" (e.g. data.random_string.refname) but a block is represented the same way it would be in config (e.g. data "random_string" "refname"). This prevents any confusion when displaying "unreferable" blocks or attributes such as terraform and allows us to load symbols early, before any schema (which would define the addressing scheme) is available.

Screenshot 2020-11-02 at 12 39 26

Implementation Notes

  • The upstream implementation actually already supports nested symbols (any blocks or attributes arbitrarily nested), but the sourcegraph/go-lsp library doesn't have the right type to expose this, so I left enabling this for another PR
  • We could now probably leverage the diagnostics generated from the initial parsing and avoid double parsing it, but I also left that for another PR
  • While the diff may seem big, most of it is actually deletions 💥 🎉 with the logic being upstreamed into hcl-lang and terraform-schema. The main "plumbing point" is basically RootModule methods.

Future

This makes it easier to solve the following issues:

It should be mostly (ignoring RHS completion which isn't supported at all yet) a matter of adding the appropriate schema to hashicorp/terraform-schema and bumping the dependency here.

Additional Links

https://github.com/hashicorp/hcl-lang
https://github.com/hashicorp/terraform-schema

@radeksimko radeksimko added the enhancement New feature or request label Oct 30, 2020
@radeksimko radeksimko marked this pull request as ready for review November 2, 2020 08:37
@radeksimko radeksimko requested a review from a team November 2, 2020 08:44
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

Overall approve, but a pairing session will go a long way for me I think

internal/filesystem/filesystem.go Show resolved Hide resolved
internal/terraform/rootmodule/types.go Show resolved Hide resolved
internal/terraform/rootmodule/types.go Show resolved Hide resolved
langserver/handlers/did_change.go Outdated Show resolved Hide resolved
langserver/handlers/did_open.go Outdated Show resolved Hide resolved
@radeksimko radeksimko force-pushed the f-hcl-decoder branch 3 times, most recently from 611622a to ffe4936 Compare November 4, 2020 12:57
@radeksimko radeksimko added this to the v0.9.0 milestone Nov 4, 2020
@radeksimko
Copy link
Member Author

radeksimko commented Nov 4, 2020

One potential bug was discovered, or at least an untested scenario during our call with @appilon where user requests completion in a file (or performs any other operation requiring schema) within a module that was not initialized. We need to make sure that the relevant schema is matched. AFAIK the current behaviour is just to provide reduced completion candidates (without any provider schema), which is incorrect.

This scenario is tested in some other contexts here:
https://github.com/hashicorp/terraform-ls/tree/master/internal/terraform/rootmodule/testdata/main-module-multienv
-> there we assume that the user can open modules/application/main.tf and have core + provider schema matched from env/{dev,prod,staging} root modules

I will need to look into this.

This is not the cleanest solution, but it serves the interest
in integrating hcl-lang & terraform-schema.

Some refactoring in the rootmodule package is certainly due by now.
@radeksimko
Copy link
Member Author

That bug was addressed in the last commit I believe, it wasn't actually that trivial, but I tried my best to avoid changing too much code, given how much is already being changed.

That said I think some of the rootmodule package is due for some refactoring.

Basically the key change this PR is introducing is that what we previously understood as "root module" was a module that is always terraform init'd (with .terraform folder in it), but now because we need to be able to work with uninitialized modules too - to provide symbols and basic early completion - we now see "root module" as any module - initialized or not.

So at the very least the naming should probably change from "root modules" to "modules", but I'd prefer to leave that for another PR.

@radeksimko radeksimko merged commit 8384c68 into master Nov 6, 2020
@radeksimko radeksimko deleted the f-hcl-decoder branch November 6, 2020 09:41
@ghost
Copy link

ghost commented Dec 6, 2020

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 Dec 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants