Skip to content

Commit 45eb1b4

Browse files
deansheatherdeansheather
authored
feat: improve terraform template parsing errors (coder#2331)
1 parent 6cf483b commit 45eb1b4

File tree

6 files changed

+278
-199
lines changed

6 files changed

+278
-199
lines changed

‎go.mod

+1-1
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ require (
8181
github.com/kirsle/configdir v0.0.0-20170128060238-e45d2f54772f
8282
github.com/lib/pq v1.10.6
8383
github.com/mattn/go-isatty v0.0.14
84+
github.com/mitchellh/go-wordwrap v1.0.1
8485
github.com/mitchellh/mapstructure v1.5.0
8586
github.com/moby/moby v20.10.17+incompatible
8687
github.com/open-policy-agent/opa v0.41.0
@@ -190,7 +191,6 @@ require (
190191
github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 // indirect
191192
github.com/mgutz/ansi v0.0.0-20170206155736-9520e82c474b // indirect
192193
github.com/miekg/dns v1.1.45 // indirect
193-
github.com/mitchellh/go-wordwrap v1.0.1 // indirect
194194
github.com/moby/term v0.0.0-20210619224110-3f7ff695adc6 // indirect
195195
github.com/muesli/ansi v0.0.0-20211031195517-c9f0611b6c70 // indirect
196196
github.com/muesli/reflow v0.3.0 // indirect

‎provisioner/terraform/parse.go

+51-5
Original file line numberDiff line numberDiff line change
@@ -2,24 +2,26 @@ package terraform
22

33
import (
44
"encoding/json"
5+
"fmt"
56
"os"
7+
"path/filepath"
8+
"strings"
69

710
"github.com/hashicorp/terraform-config-inspect/tfconfig"
11+
"github.com/mitchellh/go-wordwrap"
812
"golang.org/x/xerrors"
913

1014
"github.com/coder/coder/provisionersdk/proto"
1115
)
1216

1317
// Parse extracts Terraform variables from source-code.
1418
func (*terraform) Parse(request *proto.Parse_Request, stream proto.DRPCProvisioner_ParseStream) error {
15-
defer func() {
16-
_ = stream.CloseSend()
17-
}()
18-
19+
// Load the module and print any parse errors.
1920
module, diags := tfconfig.LoadModule(request.Directory)
2021
if diags.HasErrors() {
21-
return xerrors.Errorf("load module: %w", diags.Err())
22+
return xerrors.Errorf("load module: %s", formatDiagnostics(request.Directory, diags))
2223
}
24+
2325
parameters := make([]*proto.ParameterSchema, 0, len(module.Variables))
2426
for _, v := range module.Variables {
2527
schema, err := convertVariableToParameter(v)
@@ -83,3 +85,47 @@ func convertVariableToParameter(variable *tfconfig.Variable) (*proto.ParameterSc
8385

8486
return schema, nil
8587
}
88+
89+
// formatDiagnostics returns a nicely formatted string containing all of the
90+
// error details within the tfconfig.Diagnostics. We need to use this because
91+
// the default format doesn't provide much useful information.
92+
func formatDiagnostics(baseDir string, diags tfconfig.Diagnostics) string {
93+
var msgs strings.Builder
94+
for _, d := range diags {
95+
// Convert severity.
96+
severity := "UNKNOWN SEVERITY"
97+
switch {
98+
case d.Severity == tfconfig.DiagError:
99+
severity = "ERROR"
100+
case d.Severity == tfconfig.DiagWarning:
101+
severity = "WARN"
102+
}
103+
104+
// Determine filepath and line
105+
location := "unknown location"
106+
if d.Pos != nil {
107+
filename, err := filepath.Rel(baseDir, d.Pos.Filename)
108+
if err != nil {
109+
filename = d.Pos.Filename
110+
}
111+
location = fmt.Sprintf("%s:%d", filename, d.Pos.Line)
112+
}
113+
114+
_, _ = msgs.WriteString(fmt.Sprintf("\n%s: %s (%s)\n", severity, d.Summary, location))
115+
116+
// Wrap the details to 80 characters and indent them.
117+
if d.Detail != "" {
118+
wrapped := wordwrap.WrapString(d.Detail, 78)
119+
for _, line := range strings.Split(wrapped, "\n") {
120+
_, _ = msgs.WriteString(fmt.Sprintf("> %s\n", line))
121+
}
122+
}
123+
}
124+
125+
spacer := " "
126+
if len(diags) > 1 {
127+
spacer = "\n\n"
128+
}
129+
130+
return spacer + strings.TrimSpace(msgs.String())
131+
}

‎provisioner/terraform/parse_test.go

+84-58
Original file line numberDiff line numberDiff line change
@@ -38,83 +38,99 @@ func TestParse(t *testing.T) {
3838
}()
3939
api := proto.NewDRPCProvisionerClient(provisionersdk.Conn(client))
4040

41-
for _, testCase := range []struct {
41+
testCases := []struct {
4242
Name string
4343
Files map[string]string
4444
Response *proto.Parse_Response
45-
}{{
46-
Name: "single-variable",
47-
Files: map[string]string{
48-
"main.tf": `variable "A" {
45+
// If ErrorContains is not empty, then response.Recv() should return an
46+
// error containing this string before a Complete response is returned.
47+
ErrorContains string
48+
}{
49+
{
50+
Name: "single-variable",
51+
Files: map[string]string{
52+
"main.tf": `variable "A" {
4953
description = "Testing!"
5054
}`,
51-
},
52-
Response: &proto.Parse_Response{
53-
Type: &proto.Parse_Response_Complete{
54-
Complete: &proto.Parse_Complete{
55-
ParameterSchemas: []*proto.ParameterSchema{{
56-
Name: "A",
57-
RedisplayValue: true,
58-
AllowOverrideSource: true,
59-
Description: "Testing!",
60-
DefaultDestination: &proto.ParameterDestination{
61-
Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
62-
},
63-
}},
55+
},
56+
Response: &proto.Parse_Response{
57+
Type: &proto.Parse_Response_Complete{
58+
Complete: &proto.Parse_Complete{
59+
ParameterSchemas: []*proto.ParameterSchema{{
60+
Name: "A",
61+
RedisplayValue: true,
62+
AllowOverrideSource: true,
63+
Description: "Testing!",
64+
DefaultDestination: &proto.ParameterDestination{
65+
Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
66+
},
67+
}},
68+
},
6469
},
6570
},
6671
},
67-
}, {
68-
Name: "default-variable-value",
69-
Files: map[string]string{
70-
"main.tf": `variable "A" {
72+
{
73+
Name: "default-variable-value",
74+
Files: map[string]string{
75+
"main.tf": `variable "A" {
7176
default = "wow"
7277
}`,
73-
},
74-
Response: &proto.Parse_Response{
75-
Type: &proto.Parse_Response_Complete{
76-
Complete: &proto.Parse_Complete{
77-
ParameterSchemas: []*proto.ParameterSchema{{
78-
Name: "A",
79-
RedisplayValue: true,
80-
AllowOverrideSource: true,
81-
DefaultSource: &proto.ParameterSource{
82-
Scheme: proto.ParameterSource_DATA,
83-
Value: "wow",
84-
},
85-
DefaultDestination: &proto.ParameterDestination{
86-
Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
87-
},
88-
}},
78+
},
79+
Response: &proto.Parse_Response{
80+
Type: &proto.Parse_Response_Complete{
81+
Complete: &proto.Parse_Complete{
82+
ParameterSchemas: []*proto.ParameterSchema{{
83+
Name: "A",
84+
RedisplayValue: true,
85+
AllowOverrideSource: true,
86+
DefaultSource: &proto.ParameterSource{
87+
Scheme: proto.ParameterSource_DATA,
88+
Value: "wow",
89+
},
90+
DefaultDestination: &proto.ParameterDestination{
91+
Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
92+
},
93+
}},
94+
},
8995
},
9096
},
9197
},
92-
}, {
93-
Name: "variable-validation",
94-
Files: map[string]string{
95-
"main.tf": `variable "A" {
98+
{
99+
Name: "variable-validation",
100+
Files: map[string]string{
101+
"main.tf": `variable "A" {
96102
validation {
97103
condition = var.A == "value"
98104
}
99105
}`,
100-
},
101-
Response: &proto.Parse_Response{
102-
Type: &proto.Parse_Response_Complete{
103-
Complete: &proto.Parse_Complete{
104-
ParameterSchemas: []*proto.ParameterSchema{{
105-
Name: "A",
106-
RedisplayValue: true,
107-
ValidationCondition: `var.A == "value"`,
108-
ValidationTypeSystem: proto.ParameterSchema_HCL,
109-
AllowOverrideSource: true,
110-
DefaultDestination: &proto.ParameterDestination{
111-
Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
112-
},
113-
}},
106+
},
107+
Response: &proto.Parse_Response{
108+
Type: &proto.Parse_Response_Complete{
109+
Complete: &proto.Parse_Complete{
110+
ParameterSchemas: []*proto.ParameterSchema{{
111+
Name: "A",
112+
RedisplayValue: true,
113+
ValidationCondition: `var.A == "value"`,
114+
ValidationTypeSystem: proto.ParameterSchema_HCL,
115+
AllowOverrideSource: true,
116+
DefaultDestination: &proto.ParameterDestination{
117+
Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
118+
},
119+
}},
120+
},
114121
},
115122
},
116123
},
117-
}} {
124+
{
125+
Name: "bad-syntax",
126+
Files: map[string]string{
127+
"main.tf": "a;sd;ajsd;lajsd;lasjdf;a",
128+
},
129+
ErrorContains: `The ";" character is not valid.`,
130+
},
131+
}
132+
133+
for _, testCase := range testCases {
118134
testCase := testCase
119135
t.Run(testCase.Name, func(t *testing.T) {
120136
t.Parallel()
@@ -133,11 +149,21 @@ func TestParse(t *testing.T) {
133149

134150
for {
135151
msg, err := response.Recv()
136-
require.NoError(t, err)
152+
if err != nil {
153+
if testCase.ErrorContains != "" {
154+
require.ErrorContains(t, err, testCase.ErrorContains)
155+
break
156+
}
157+
158+
require.NoError(t, err)
159+
}
137160

138161
if msg.GetComplete() == nil {
139162
continue
140163
}
164+
if testCase.ErrorContains != "" {
165+
t.Fatal("expected error but job completed successfully")
166+
}
141167

142168
// Ensure the want and got are equivalent!
143169
want, err := json.Marshal(testCase.Response)

‎provisioner/terraform/provision.go

+28-29
Original file line numberDiff line numberDiff line change
@@ -70,30 +70,6 @@ func (t *terraform) Provision(stream proto.DRPCProvisioner_ProvisionStream) erro
7070
return xerrors.Errorf("terraform version %q is too old. required >= %q", version.String(), minimumTerraformVersion.String())
7171
}
7272

73-
statefilePath := filepath.Join(start.Directory, "terraform.tfstate")
74-
if len(start.State) > 0 {
75-
err := os.WriteFile(statefilePath, start.State, 0600)
76-
if err != nil {
77-
return xerrors.Errorf("write statefile %q: %w", statefilePath, err)
78-
}
79-
}
80-
81-
reader, writer := io.Pipe()
82-
defer reader.Close()
83-
defer writer.Close()
84-
go func() {
85-
scanner := bufio.NewScanner(reader)
86-
for scanner.Scan() {
87-
_ = stream.Send(&proto.Provision_Response{
88-
Type: &proto.Provision_Response_Log{
89-
Log: &proto.Log{
90-
Level: proto.LogLevel_DEBUG,
91-
Output: scanner.Text(),
92-
},
93-
},
94-
})
95-
}
96-
}()
9773
terraformEnv := map[string]string{}
9874
// Required for "terraform init" to find "git" to
9975
// clone Terraform modules.
@@ -113,15 +89,38 @@ func (t *terraform) Provision(stream proto.DRPCProvisioner_ProvisionStream) erro
11389
if err != nil {
11490
return xerrors.Errorf("set terraform env: %w", err)
11591
}
116-
terraform.SetStdout(writer)
117-
t.logger.Debug(shutdown, "running initialization")
92+
93+
statefilePath := filepath.Join(start.Directory, "terraform.tfstate")
94+
if len(start.State) > 0 {
95+
err := os.WriteFile(statefilePath, start.State, 0600)
96+
if err != nil {
97+
return xerrors.Errorf("write statefile %q: %w", statefilePath, err)
98+
}
99+
}
100+
101+
reader, writer := io.Pipe()
102+
go func(reader *io.PipeReader) {
103+
scanner := bufio.NewScanner(reader)
104+
for scanner.Scan() {
105+
_ = stream.Send(&proto.Provision_Response{
106+
Type: &proto.Provision_Response_Log{
107+
Log: &proto.Log{
108+
Level: proto.LogLevel_ERROR,
109+
Output: scanner.Text(),
110+
},
111+
},
112+
})
113+
}
114+
}(reader)
115+
116+
terraform.SetStderr(writer)
118117
err = terraform.Init(shutdown)
118+
_ = reader.Close()
119+
_ = writer.Close()
119120
if err != nil {
120121
return xerrors.Errorf("initialize terraform: %w", err)
121122
}
122-
t.logger.Debug(shutdown, "ran initialization")
123-
_ = reader.Close()
124-
terraform.SetStdout(io.Discard)
123+
terraform.SetStderr(io.Discard)
125124

126125
env := os.Environ()
127126
env = append(env,

0 commit comments

Comments
 (0)