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

fix: avoid checking the schema in case of syntax errors #396

Merged
merged 4 commits into from
Jun 15, 2022

Conversation

i4ki
Copy link
Contributor

@i4ki i4ki commented Jun 14, 2022

The invalid config below:

stack {
    wants = 
}

generates an error when calling the hashicorp parse.ParseHCL() function but it populate its parse.Files() with an invalid data structure for the file's body... for the case above, it creates an attribute with name "wants" and value of type cty.DynamicValue that's not even nullable (attrVar.IsNull() returns false)...

So we cannot have syntax error + schema errors because they do not provide partial correct nodes.

The fix was making terramate check the schema only for the files successfully parsed (valid HCL).

I got this issue while experimenting with the VSCode extension. The Language Server crashed with log below:

attribute=wants stack=
2022-06-14T13:09:43+01:00 TRC Iterate over values. action=assignSet()
panic: can't use ElementIterator on unknown value

goroutine 22 [running]:
github.com/zclconf/go-cty/cty.Value.ElementIterator({{{0x7776c8?, 0x953768?}}, {0x6b9120?, 0x9534a0?}})
	/home/i4k/go/pkg/mod/github.com/zclconf/go-cty@v1.8.3/cty/value_ops.go:1118 +0xd8
github.com/mineiros-io/terramate/hcl.assignSet({0xc000258744, 0x5}, 0xc0001431d0, {{{0x7776c8?, 0x953768?}}, {0x6b9120?, 0x9534a0?}})
	/home/i4k/go/pkg/mod/github.com/mineiros-io/terramate@v0.1.5/hcl/hcl.go:652 +0x37c
github.com/mineiros-io/terramate/hcl.parseStack(0xc000143180, 0xc000245200)
	/home/i4k/go/pkg/mod/github.com/mineiros-io/terramate@v0.1.5/hcl/hcl.go:739 +0x919
github.com/mineiros-io/terramate/hcl.(*TerramateParser).parseTerramateSchema(0xc00028f950)
	/home/i4k/go/pkg/mod/github.com/mineiros-io/terramate@v0.1.5/hcl/hcl.go:1142 +0xb2f
github.com/mineiros-io/terramate/hcl.(*TerramateParser).Parse(0xc00028f950)
	/home/i4k/go/pkg/mod/github.com/mineiros-io/terramate@v0.1.5/hcl/hcl.go:207 +0x72
github.com/mineiros-io/terramate-ls.checkFiles({0xc00014bb40, 0x2, 0x2?}, {0xc0001f0237, 0x61}, {0xc000130c60, 0x146})
	/home/i4k/src/mineiros/terramate-ls/ls.go:401 +0x128
github.com/mineiros-io/terramate-ls.(*Server).checkAndReply(0xc0001f0230?, {0x777658, 0xc00012fc80}, 0xc00014ba60, {0xc0001f0237, 0x61}, {0xc000130c60, 0x146})
	/home/i4k/src/mineiros/terramate-ls/ls.go:328 +0x156
github.com/mineiros-io/terramate-ls.(*Server).handleDocumentOpen(0x2aa?, {0x777658, 0xc00012fc80}, 0xc00014f7b8?, {0x7fee44fc1f18, 0xc00025a420}, {{0x776d10, 0xc0001d84d0}, 0xff, {0x0, ...}, ...})
	/home/i4k/src/mineiros/terramate-ls/ls.go:188 +0x11f
github.com/mineiros-io/terramate-ls.(*Server).Handler(0xc000132510, {0x777658, 0xc00012fc80}, 0xc00014ba60, {0x7fee44fc1f18?, 0xc00025a420?})
	/home/i4k/src/mineiros/terramate-ls/ls.go:93 +0x4ff
go.lsp.dev/jsonrpc2.(*conn).run(0xc000100c80, {0x777658, 0xc00012fc80}, 0xc0001d8790)
	/home/i4k/go/pkg/mod/go.lsp.dev/jsonrpc2@v0.10.0/conn.go:206 +0x24b
created by go.lsp.dev/jsonrpc2.(*conn).Go
	/home/i4k/go/pkg/mod/go.lsp.dev/jsonrpc2@v0.10.0/conn.go:189 +0xb0
[Error - 1:09:43 PM] Connection to server got closed. Server will not be restarted.

@i4ki i4ki requested a review from katcipis June 14, 2022 12:50
@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

Merging #396 (ab82f6c) into main (3830aae) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #396      +/-   ##
==========================================
- Coverage   69.63%   69.62%   -0.02%     
==========================================
  Files          37       37              
  Lines        7276     7270       -6     
==========================================
- Hits         5067     5062       -5     
+ Misses       1959     1958       -1     
  Partials      250      250              
Flag Coverage Δ
tests 69.62% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
hcl/hcl.go 87.55% <100.00%> (-0.09%) ⬇️
manager.go 63.10% <0.00%> (+0.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3830aae...ab82f6c. Read the comment docs.

@katcipis
Copy link
Contributor

"generates an error when calling the hashicorp parse.ParseHCL() function but it populate its parse.Files() with an invalid data structure for the file's body... for the case above, it creates an attribute with name "wants" and value of type cty.DynamicValue that's not even nullable (attrVar.IsNull() returns false)..."


Copy link
Contributor

@soerenmartius soerenmartius left a comment

Choose a reason for hiding this comment

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

lgtm! Good job @i4ki

@i4ki i4ki merged commit 4957232 into main Jun 15, 2022
@i4ki i4ki deleted the i4k-avoid-schema-check-if-syntax-err branch June 15, 2022 10:11
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.

None yet

3 participants