Skip to content

Commit 4f6f193

Browse files
geokatgeokat
committed
better error reporting and other small fixes
1 parent ab9a741 commit 4f6f193

4 files changed

Lines changed: 92 additions & 51 deletions

File tree

‎override.go‎

Lines changed: 42 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -23,21 +23,24 @@ type primaryState struct {
2323
// and returns a new FS where override content has been merged into primary
2424
// files using Terraform's override semantics.
2525
// If no override files are found, the original FS is returned unchanged.
26-
// If an error is encountered while processing HCL, diagnostics are returned in
27-
// addition to a non-nil error.
26+
// If an error is encountered, diagnostics are returned in addition to a
27+
// non-nil error.
28+
// Warning diagnostics may also be returned on success (e.g. for skipped
29+
// .tf.json files).
2830
//
2931
// Override files are identified by Terraform's naming convention:
3032
// "override.tf", "*_override.tf", and their .tf.json variants. We only support
3133
// .tf files; .tf.json files get a diagnostic warning and are excluded from
3234
// override merging.
3335
//
34-
// https://developer.hashicorp.com/terraform/language/files/override
36+
// Ref: https://developer.hashicorp.com/terraform/language/files/override
3537
//
36-
// Note: Terraform validates primary blocks before merging overrides, so it
38+
// NOTE: Terraform validates primary blocks before merging overrides, so it
3739
// rejects a primary block that is missing a required attribute even if the
3840
// override would supply it. We merge first, so this edge case passes
3941
// validation. This is immaterial in practice: Coder runs terraform plan
40-
// during template import, which would catch it before the template is saved.
42+
// during template import, which would catch it before the template is
43+
// published.
4144
func mergeOverrides(origFS fs.FS) (fs.FS, hcl.Diagnostics, error) {
4245
// Group files by directory, separating primary from override files.
4346
// Walk the entire tree, not just the root directory, because Trivy's
@@ -46,6 +49,8 @@ func mergeOverrides(origFS fs.FS) (fs.FS, hcl.Diagnostics, error) {
4649
type dirFiles struct {
4750
primaries []string
4851
overrides []string
52+
// Used to generate warnings at merge stage.
53+
jsonPrimaries []string
4954
}
5055
dirs := make(map[string]*dirFiles)
5156

@@ -65,25 +70,29 @@ func mergeOverrides(origFS fs.FS) (fs.FS, hcl.Diagnostics, error) {
6570
return nil
6671
}
6772

68-
// Only .tf files are supported for override merging. Skip .tf.json
69-
// files entirely — they remain in the FS for Trivy to parse directly
70-
// but never participate in override merging.
73+
dir := path.Dir(p)
74+
if dirs[dir] == nil {
75+
dirs[dir] = &dirFiles{}
76+
}
77+
78+
// We don't support parsing .tf.json files. They remain in the
79+
// FS for Trivy to parse directly but never participate in
80+
// override merging.
7181
if ext == ".tf.json" {
7282
if isOverrideFile(d.Name()) {
7383
warnings = warnings.Append(&hcl.Diagnostic{
7484
Severity: hcl.DiagWarning,
75-
Summary: "Unmerged .tf.json override file",
76-
Detail: fmt.Sprintf("Not merging override file %q that uses JSON format", p),
85+
Summary: "Override file uses unsupported .tf.json format",
86+
Detail: fmt.Sprintf("%s skipped for override merging", p),
7787
})
88+
} else {
89+
// Save the name of the .tf.json primary so we issue a
90+
// warning only if we do merging for the dir (less noise).
91+
dirs[dir].jsonPrimaries = append(dirs[dir].jsonPrimaries, p)
7892
}
7993
return nil
8094
}
8195

82-
dir := path.Dir(p)
83-
if dirs[dir] == nil {
84-
dirs[dir] = &dirFiles{}
85-
}
86-
8796
if isOverrideFile(d.Name()) {
8897
dirs[dir].overrides = append(dirs[dir].overrides, p)
8998
} else {
@@ -92,7 +101,7 @@ func mergeOverrides(origFS fs.FS) (fs.FS, hcl.Diagnostics, error) {
92101
return nil
93102
})
94103
if err != nil {
95-
return nil, warnings, fmt.Errorf("walk filesystem: %w", err)
104+
return nil, warnings, fmt.Errorf("error reading template files: %w", err)
96105
}
97106

98107
hasOverrides := false
@@ -117,17 +126,25 @@ func mergeOverrides(origFS fs.FS) (fs.FS, hcl.Diagnostics, error) {
117126
continue
118127
}
119128

129+
for _, jp := range dir.jsonPrimaries {
130+
warnings = warnings.Append(&hcl.Diagnostic{
131+
Severity: hcl.DiagWarning,
132+
Summary: "Primary file uses .tf.json format",
133+
Detail: fmt.Sprintf("%s skipped for override merging", jp),
134+
})
135+
}
136+
120137
// Parse all primary files upfront so override files can be applied
121138
// sequentially, each merging into the already-merged result.
122139
primaries := make([]*primaryState, 0, len(dir.primaries))
123140
for _, path := range dir.primaries {
124141
content, err := fs.ReadFile(origFS, path)
125142
if err != nil {
126-
return nil, warnings, fmt.Errorf("read file %s: %w", path, err)
143+
return nil, warnings, fmt.Errorf("error reading file %s: %w", path, err)
127144
}
128145
f, diags := hclwrite.ParseConfig(content, path, hcl.Pos{Line: 1, Column: 1})
129146
if diags.HasErrors() {
130-
return nil, warnings.Extend(diags), fmt.Errorf("parse file %s", path)
147+
return nil, warnings.Extend(diags), errors.New("error parsing file")
131148
}
132149
primaries = append(primaries, &primaryState{path: path, file: f})
133150
}
@@ -136,7 +153,7 @@ func mergeOverrides(origFS fs.FS) (fs.FS, hcl.Diagnostics, error) {
136153
// rejects them.
137154
diags := checkDuplicatePrimaryBlocks(primaries)
138155
if diags.HasErrors() {
139-
return nil, warnings.Extend(diags), errors.New("check duplicate primary blocks")
156+
return nil, warnings.Extend(diags), errors.New("error checking duplicate primary blocks")
140157
}
141158

142159
// Process each override file sequentially. If multiple override files
@@ -145,12 +162,12 @@ func mergeOverrides(origFS fs.FS) (fs.FS, hcl.Diagnostics, error) {
145162
for _, path := range dir.overrides {
146163
content, err := fs.ReadFile(origFS, path)
147164
if err != nil {
148-
return nil, warnings, fmt.Errorf("read file %s: %w", path, err)
165+
return nil, warnings, fmt.Errorf("error reading file %s: %w", path, err)
149166
}
150167

151168
f, diags := hclwrite.ParseConfig(content, path, hcl.Pos{Line: 1, Column: 1})
152169
if diags.HasErrors() {
153-
return nil, warnings.Extend(diags), fmt.Errorf("parse file %s", path)
170+
return nil, warnings.Extend(diags), errors.New("error parsing file")
154171
}
155172

156173
for _, oblock := range f.Body().Blocks() {
@@ -160,7 +177,7 @@ func mergeOverrides(origFS fs.FS) (fs.FS, hcl.Diagnostics, error) {
160177
if oblock.Type() == "locals" {
161178
diags := mergeLocalsBlock(primaries, oblock, path)
162179
if diags.HasErrors() {
163-
return nil, warnings.Extend(diags), fmt.Errorf("merge locals block")
180+
return nil, warnings.Extend(diags), errors.New("error merging locals block")
164181
}
165182
continue
166183
}
@@ -312,7 +329,7 @@ func mergeLocalsBlock(primaries []*primaryState, override *hclwrite.Block, overr
312329
diags = diags.Append(&hcl.Diagnostic{
313330
Severity: hcl.DiagError,
314331
Summary: "Missing base local value definition to override",
315-
Detail: fmt.Sprintf("No local %q found in primary files; defined in %s.", name, overridePath),
332+
Detail: fmt.Sprintf("Local %q in %s has no base definition to override", name, overridePath),
316333
})
317334
}
318335
}
@@ -376,8 +393,8 @@ func checkDuplicatePrimaryBlocks(primaries []*primaryState) hcl.Diagnostics {
376393
if firstFile, exists := seen[key]; exists {
377394
diags = diags.Append(&hcl.Diagnostic{
378395
Severity: hcl.DiagError,
379-
Summary: "Duplicate block in primary",
380-
Detail: fmt.Sprintf("Block %s defined in both %s and %s; Terraform rejects duplicates.", key, firstFile, primary.path),
396+
Summary: fmt.Sprintf("Duplicate block %q", key),
397+
Detail: fmt.Sprintf("Block %q defined in both %s and %s", key, firstFile, primary.path),
381398
})
382399
} else {
383400
seen[key] = primary.path

‎override_test.go‎

Lines changed: 41 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -361,6 +361,21 @@ func readFile(t *testing.T, fsys fs.FS, name string) []byte {
361361
return buf
362362
}
363363

364+
// testMergeOverrides wraps mergeOverrides and asserts the contract:
365+
// error-severity diagnostics are always accompanied by a non-nil error.
366+
func testMergeOverrides(t *testing.T, fsys fs.FS) (fs.FS, hcl.Diagnostics, error) {
367+
t.Helper()
368+
result, diags, err := mergeOverrides(fsys)
369+
if err == nil {
370+
for _, d := range diags {
371+
if d.Severity == hcl.DiagError {
372+
t.Fatal("mergeOverrides returned error diagnostic without non-nil error")
373+
}
374+
}
375+
}
376+
return result, diags, err
377+
}
378+
364379
func TestMergeOverrideFiles(t *testing.T) {
365380
t.Parallel()
366381

@@ -370,7 +385,7 @@ func TestMergeOverrideFiles(t *testing.T) {
370385
"main.tf": &fstest.MapFile{Data: []byte(`resource "a" "b" { x = 1 }`)},
371386
"override.tf": &fstest.MapFile{Data: []byte(``)},
372387
}
373-
result, diags, err := mergeOverrides(fsys)
388+
result, diags, err := testMergeOverrides(t, fsys)
374389
require.NoError(t, err)
375390
assert.Empty(t, diags)
376391

@@ -383,7 +398,7 @@ func TestMergeOverrideFiles(t *testing.T) {
383398
original := fstest.MapFS{
384399
"main.tf": &fstest.MapFile{Data: []byte(`resource "a" "b" { x = 1 }`)},
385400
}
386-
result, diags, err := mergeOverrides(original)
401+
result, diags, err := testMergeOverrides(t, original)
387402
require.NoError(t, err)
388403
assert.Empty(t, diags)
389404
// Should return the exact same FS when there are no overrides.
@@ -398,7 +413,7 @@ func TestMergeOverrideFiles(t *testing.T) {
398413
y = 2
399414
}`)},
400415
}
401-
_, _, err := mergeOverrides(fsys)
416+
_, _, err := testMergeOverrides(t, fsys)
402417
require.Error(t, err)
403418
assert.Contains(t, err.Error(), "no matching block")
404419
})
@@ -414,7 +429,7 @@ func TestMergeOverrideFiles(t *testing.T) {
414429
y = 99
415430
}`)},
416431
}
417-
result, _, err := mergeOverrides(fsys)
432+
result, _, err := testMergeOverrides(t, fsys)
418433
require.NoError(t, err)
419434

420435
// Read the merged primary file.
@@ -428,7 +443,7 @@ func TestMergeOverrideFiles(t *testing.T) {
428443
"main.tf": &fstest.MapFile{Data: []byte(`resource "a" "b" { x = 1 }`)},
429444
"override.tf": &fstest.MapFile{Data: []byte(`resource "a" "b" { x = 2 }`)},
430445
}
431-
result, _, err := mergeOverrides(fsys)
446+
result, _, err := testMergeOverrides(t, fsys)
432447
require.NoError(t, err)
433448

434449
_, err = result.Open("override.tf")
@@ -443,7 +458,7 @@ func TestMergeOverrideFiles(t *testing.T) {
443458
"foo_override.tf": &fstest.MapFile{Data: []byte(`resource "a" "b" { x = 2 }`)},
444459
"other.txt": &fstest.MapFile{Data: []byte("hello")},
445460
}
446-
result, _, err := mergeOverrides(fsys)
461+
result, _, err := testMergeOverrides(t, fsys)
447462
require.NoError(t, err)
448463

449464
f, err := result.Open(".")
@@ -481,7 +496,7 @@ func TestMergeOverrideFiles(t *testing.T) {
481496
from_b = "bbb"
482497
}`)},
483498
}
484-
result, _, err := mergeOverrides(fsys)
499+
result, _, err := testMergeOverrides(t, fsys)
485500
require.NoError(t, err)
486501

487502
content := readFile(t, result, "main.tf")
@@ -502,7 +517,7 @@ func TestMergeOverrideFiles(t *testing.T) {
502517
y = 42
503518
}`)},
504519
}
505-
result, _, err := mergeOverrides(fsys)
520+
result, _, err := testMergeOverrides(t, fsys)
506521
require.NoError(t, err)
507522

508523
// Root file should be unchanged (no overrides in root).
@@ -525,7 +540,7 @@ func TestMergeOverrideFiles(t *testing.T) {
525540
"main.tf": &fstest.MapFile{Data: []byte(`resource "a" "b" { x = 1 }`)},
526541
"override.tf.json": &fstest.MapFile{Data: []byte(`{}`)},
527542
}
528-
result, diags, err := mergeOverrides(fsys)
543+
result, diags, err := testMergeOverrides(t, fsys)
529544
require.NoError(t, err)
530545

531546
// Should warn about the .tf.json override file.
@@ -543,10 +558,10 @@ func TestMergeOverrideFiles(t *testing.T) {
543558
"main.tf": &fstest.MapFile{Data: []byte(`resource "a" "b" { x = 1 }`)},
544559
"other.tf.json": &fstest.MapFile{Data: []byte(`{}`)},
545560
}
546-
result, diags, err := mergeOverrides(fsys)
561+
result, diags, err := testMergeOverrides(t, fsys)
547562
require.NoError(t, err)
548563

549-
// No warnings for primary .tf.json files.
564+
// No warnings for primary .tf.json files because there are no overrides.
550565
assert.Empty(t, diags)
551566

552567
// Original FS returned since no overrides exist.
@@ -561,9 +576,11 @@ func TestMergeOverrideFiles(t *testing.T) {
561576
"extra.tf.json": &fstest.MapFile{Data: jsonContent},
562577
"override.tf": &fstest.MapFile{Data: []byte(`resource "a" "b" { x = 2 }`)},
563578
}
564-
result, diags, err := mergeOverrides(fsys)
579+
result, diags, err := testMergeOverrides(t, fsys)
565580
require.NoError(t, err)
566-
assert.Empty(t, diags)
581+
582+
require.Len(t, diags, 1)
583+
assert.Contains(t, diags[0].Summary, "Primary file uses .tf.json format")
567584

568585
// .tf.json file should still be accessible in the result FS.
569586
content := readFile(t, result, "extra.tf.json")
@@ -579,7 +596,7 @@ func TestMergeOverrideFiles(t *testing.T) {
579596
"a_override.tf": &fstest.MapFile{Data: []byte(`resource "a" "b" { x = 99 }`)},
580597
"b_override.tf.json": &fstest.MapFile{Data: []byte(`{}`)},
581598
}
582-
result, diags, err := mergeOverrides(fsys)
599+
result, diags, err := testMergeOverrides(t, fsys)
583600
require.NoError(t, err)
584601

585602
// Warning for the .tf.json override only.
@@ -603,7 +620,7 @@ func TestMergeOverrideFiles(t *testing.T) {
603620
foo = "overridden"
604621
}`)},
605622
}
606-
result, diags, err := mergeOverrides(fsys)
623+
result, diags, err := testMergeOverrides(t, fsys)
607624
require.NoError(t, err)
608625
assert.Empty(t, diags)
609626

@@ -627,7 +644,7 @@ func TestMergeOverrideFiles(t *testing.T) {
627644
from_b = "overridden_b"
628645
}`)},
629646
}
630-
result, diags, err := mergeOverrides(fsys)
647+
result, diags, err := testMergeOverrides(t, fsys)
631648
require.NoError(t, err)
632649
assert.Empty(t, diags)
633650

@@ -660,7 +677,7 @@ locals {
660677
y = "y_from_b"
661678
}`)},
662679
}
663-
result, diags, err := mergeOverrides(fsys)
680+
result, diags, err := testMergeOverrides(t, fsys)
664681
require.NoError(t, err)
665682
assert.Empty(t, diags)
666683

@@ -682,12 +699,12 @@ locals {
682699
nonexistent = "nope"
683700
}`)},
684701
}
685-
result, diags, err := mergeOverrides(fsys)
702+
result, diags, err := testMergeOverrides(t, fsys)
686703
require.Error(t, err)
687704
assert.Nil(t, result)
688705
require.Len(t, diags, 1)
689706
assert.Equal(t, hcl.DiagError, diags[0].Severity)
690-
assert.Contains(t, diags[0].Summary, "Missing base local value definition")
707+
assert.Contains(t, diags[0].Summary, "Missing base local")
691708
})
692709

693710
t.Run("DuplicatePrimaryBlockErrors", func(t *testing.T) {
@@ -704,12 +721,12 @@ data "coder_parameter" "foo" {
704721
name = "bar"
705722
}`)},
706723
}
707-
result, diags, err := mergeOverrides(fsys)
724+
result, diags, err := testMergeOverrides(t, fsys)
708725
require.Error(t, err)
709726
assert.Nil(t, result, "FS should be nil so caller keeps original")
710727
require.Len(t, diags, 1)
711728
assert.Equal(t, hcl.DiagError, diags[0].Severity)
712-
assert.Contains(t, diags[0].Summary, "Duplicate block in primary")
729+
assert.Contains(t, diags[0].Summary, "Duplicate block")
713730
})
714731

715732
t.Run("DuplicatePrimaryLocalsNoError", func(t *testing.T) {
@@ -726,7 +743,7 @@ locals {
726743
a = 99
727744
}`)},
728745
}
729-
result, diags, err := mergeOverrides(fsys)
746+
result, diags, err := testMergeOverrides(t, fsys)
730747
require.NoError(t, err)
731748
assert.Empty(t, diags)
732749

@@ -752,7 +769,7 @@ terraform {
752769
resource "a" "b" { x = 1 }`)},
753770
"override.tf": &fstest.MapFile{Data: []byte(`resource "a" "b" { x = 2 }`)},
754771
}
755-
result, diags, err := mergeOverrides(fsys)
772+
result, diags, err := testMergeOverrides(t, fsys)
756773
require.NoError(t, err)
757774
assert.Empty(t, diags)
758775

@@ -773,7 +790,7 @@ func TestFilteredReadDir(t *testing.T) {
773790
"other.tf": &fstest.MapFile{Data: []byte(`resource "c" "d" { y = 1 }`)},
774791
"b_override.tf": &fstest.MapFile{Data: []byte(`resource "c" "d" { y = 2 }`)},
775792
}
776-
result, _, err := mergeOverrides(fsys)
793+
result, _, err := testMergeOverrides(t, fsys)
777794
require.NoError(t, err)
778795

779796
f, err := result.Open(".")

0 commit comments

Comments
 (0)