-
Notifications
You must be signed in to change notification settings - Fork 6
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
Feat: Added inputs variables dereferencing support [CFG-1501] #7
Conversation
0c0bd46
to
1cd7ce1
Compare
2fe3299
to
0902b59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left two small comments, non blocking. I think it looks good in general and it works! I tested this commit with the test.js
from Snyk CLI locally as well and the integration works and still returning results for the defaults. 🙌
575c1dd
to
5793283
Compare
@ipapast and @teodora-sandu can you please do a quick re-review to the changes I did following your feedback? :) |
67ffb7a
to
2914910
Compare
2914910
to
97b27df
Compare
} | ||
|
||
type VariableMap map[string]cty.Value | ||
func extractFromTfFile(file *hcl.File) (VariableMap, hcl.Diagnostics) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Would it be nicer to name this extractVariableBlocks
or something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that extractVariableBlocks
sounds more abstract but I personally think that extractFromTfFile
makes it more explicit what this function does and what it's being used for.
@@ -531,48 +532,115 @@ block "label_one" { | |||
} | |||
|
|||
func TestExtractVariablesSuccess(t *testing.T) { | |||
type TestInput struct { | |||
fileName string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Do we need the fileName
here? It was defaulting to "test"
before and the simpler this test table is the better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We added it to prepare for the next ticket where we will have different logic for .tf
and .tfvars
files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉 Left some minor nits but up to you if you want to implement them. Great job!
What this does
Adds supporting reading variable values in Terraform (.tf) files.
This is done to help on our vision to create better TF support for our product and find more issues that might have been ignored so far because they would be coming from variable values.
This PR is to support input variables, with or without default values.
Background context
In the Decision Document, we decided to create a new library in Go where the implementation for this would live.
We already have played a spike on how this could look like.
We plan to build a parser that can deal with (end goal):
As part of this ticket, we will only be adding support for input variables (and defaults).
Additional information