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

Add support for TraversalExpr #36

Merged
merged 1 commit into from
May 14, 2021
Merged

Add support for TraversalExpr #36

merged 1 commit into from
May 14, 2021

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Apr 28, 2021

Closes #2


(Schema) Reference types

In general the schema recognizes two types of references - (1) type-aware, and (2) type-less

In Terraform type-aware references are commonly used to set values of fields via reference. For example:

key_name = aws_key_pair.iac_in_action.key_name

Here key_name is of type string and relatedly aws_key_pair.iac_in_action.key_name points to a field of type string.

Type-less references are used to draw an explicit dependency between resources, e.g.

resource "aws_instance" "example" {
// ...
  depends_on = [ aws_s3_bucket.example ]
}
resource "aws_s3_bucket" "example" {
// ...
}

Here in this particular context of depends_on, aws_s3_bucket.example only represents a "pointer" to the lower block, but isn't concerned about what data (whether any) it points to.

Type Declaration

It is only possible to declare type explicitly via the schema. e.g. schema sets name to string and that's what it will always be.

More dynamic declaration and inference would be also useful in Terraform, specifically for variables, where variable type is declared within the config, or is inferred in case of locals. This seemed like a scope-creep in this MVP, so I left it out, but it's certainly something to revisit.

TODO: Create issue for this.


There can be cases where the same address (such as aws_s3_bucket.example) can be type-less and type-aware, it only depends on the context. This is why we allow references with the same address to exist and then filter based on context.

Functionality

This PR adds support for traversal expressions in the following areas:

Completion

This is the trickiest and most complex part with most edge cases - many of which are yet to be covered in follow-up PRs.

Currently we decode all steps of a traversal and also return such "fully expanded" traversals instead of letting the user complete step-by-step.

e.g. given a config & schema

resource "aws_lb" "example" {
  listener {
    port = 80
    protocol = "tcp"
  }
}

we would complete just aws_lb.example.listener[0].protocol for traversal of type string and aws_lb.example.listener[0].port for traversal of type number. This may be good/better UX in this particular context and small snippet, but might not scale with larger configurations.

The internal structure will allow us to explore some alternative approaches though and possibly even combine them, so that e.g. we can complete just all resource types (e.g. aws_lb), then after . we complete all reference names for that resource type (e.g. example), then after . all the attributes of that particular resource instance and so on.

The completion does not take any external sources (such as Terraform state) into effect, but relies purely on configuration and schema. This means that e.g. if a map/list/set has some elements computed outside of the configuration these would NOT be surfaced as elements of a given field via [0], [1], ["key"] etc. This will likely be a useful feature for Terraform, but it is intentionally left out from this MVP for complexity.

Another edge case which is not entirely related to this PR, but perhaps becomes more visible is that we don't provide completion inside nested expressions in most cases yet. I implemented some of the simple ones, such as

depends_on = [ HERE ]

where depends_on is SetExpr, but only where the wrapping expression (SetExpr, ListExpr) is empty - i.e. for the first element only. Again - this is certainly something that should be implemented, but was left out for complexity.

Hover

Due to the way things are structured, we would only provide hover data for a matching reference, for example

number_field = data.aws_instance.example.number_field # hover data here
number_field = data.aws_instance.example.string_field # hover data NOT available here

I believe this is ok trade-off for now, but we can revisit later, if necessary.

Semantic Tokens

The same limitation as in hover applies here - if the traversal expression doesn't exist/match, then it won't be reported as token at all.

There is a new token type lang.TokenTraversalStep which represents the parts between dots in addresses. This is cleaner than reporting the whole traversal as token, because clients could otherwise paint it all the same colour (including index numbers/keys, brackets and dots).

We could go further here and report individual tokens with some more context/meaning, but this would also require collecting such meaning via schema somehow. See hashicorp/vscode-terraform#574 for related discussion.

@@ -44,8 +41,6 @@ func (t BlockType) GoString() string {
return "BlockTypeObject"
case BlockTypeSet:
return "BlockTypeSet"
case BlockTypeTuple:
return "BlockTypeTuple"
Copy link
Member Author

Choose a reason for hiding this comment

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

@radeksimko radeksimko force-pushed the f-traversal-expr branch 9 times, most recently from c104799 to b675077 Compare May 4, 2021 16:11
@radeksimko radeksimko force-pushed the f-traversal-expr branch 3 times, most recently from 52408e2 to 5a674a2 Compare May 5, 2021 16:33
@radeksimko radeksimko marked this pull request as ready for review May 5, 2021 16:35
@radeksimko radeksimko force-pushed the f-traversal-expr branch 3 times, most recently from 7583bc9 to 6916ed8 Compare May 5, 2021 17:42
@radeksimko radeksimko requested a review from a team May 5, 2021 19:09
@radeksimko radeksimko force-pushed the f-traversal-expr branch 3 times, most recently from e5bdfb6 to 102ac67 Compare May 12, 2021 11:15
Copy link

@aeschright aeschright left a comment

Choose a reason for hiding this comment

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

👍

@radeksimko radeksimko merged commit 15d3afd into main May 14, 2021
@radeksimko radeksimko deleted the f-traversal-expr branch May 14, 2021 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Support for traversal expressions
2 participants