From bbc1114f5b774e5a56e4dc5776555a1a6a8024c3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Dec 2025 16:35:51 -0600 Subject: [PATCH 1/5] feat: ValidatePresets to validate presets for prebuild use --- extract/preset.go | 34 ++++++++++- preview.go | 42 ++++++++++++- preview_test.go | 114 ++++++++++++++++++++++++++++++++++++ testdata/presetfail/main.tf | 51 ++++++++++++++++ types/preset.go | 5 ++ 5 files changed, 242 insertions(+), 4 deletions(-) create mode 100644 testdata/presetfail/main.tf diff --git a/extract/preset.go b/extract/preset.go index 01c9310..b6cf4f0 100644 --- a/extract/preset.go +++ b/extract/preset.go @@ -1,12 +1,34 @@ package extract import ( + "fmt" + "github.com/aquasecurity/trivy/pkg/iac/terraform" - "github.com/coder/preview/types" "github.com/hashicorp/hcl/v2" + + "github.com/coder/preview/types" ) -func PresetFromBlock(block *terraform.Block) types.Preset { +func PresetFromBlock(block *terraform.Block) (tfPreset types.Preset) { + defer func() { + // Extra safety mechanism to ensure that if a panic occurs, we do not break + // everything else. + if r := recover(); r != nil { + tfPreset = types.Preset{ + PresetData: types.PresetData{ + Name: block.Label(), + }, + Diagnostics: types.Diagnostics{ + { + Severity: hcl.DiagError, + Summary: "Panic occurred in extracting preset. This should not happen, please report this to Coder.", + Detail: fmt.Sprintf("panic in preset extract: %+v", r), + }, + }, + } + } + }() + p := types.Preset{ PresetData: types.PresetData{ Parameters: make(map[string]string), @@ -41,5 +63,13 @@ func PresetFromBlock(block *terraform.Block) types.Preset { p.Default = defaultAttr.Value().True() } + prebuildBlock := block.GetBlock("prebuilds") + if prebuildBlock != nil { + p.Prebuild = &types.PrebuildData{ + // Invalid values will be set to 0 + Instances: int(optionalInteger(prebuildBlock, "instances")), + } + } + return p } diff --git a/preview.go b/preview.go index ea4154a..bf538d1 100644 --- a/preview.go +++ b/preview.go @@ -68,6 +68,42 @@ func (o Output) MarshalJSON() ([]byte, error) { }) } +// ValidatePresets will iterate over each preset, validate the inputs as a set, +// and attach any diagnostics to the preset if there are issues. +func ValidatePresets(ctx context.Context, input Input, preValid []types.Preset, dir fs.FS) { + for i := range preValid { + pre := &preValid[i] + if pre.Prebuild == nil || pre.Prebuild.Instances <= 0 { + // No prebuilds, so presets do not need to be valid without user input + continue + } + + input.ParameterValues = pre.Parameters + + output, diagnostics := Preview(ctx, input, dir) + if diagnostics.HasErrors() { + pre.Diagnostics = append(pre.Diagnostics, diagnostics...) + } + + if output == nil { + continue + } + + for _, param := range output.Parameters { + if hcl.Diagnostics(param.Diagnostics).HasErrors() { + for _, paramDiag := range param.Diagnostics { + if paramDiag.Severity != hcl.DiagError { + continue // Only care about errors here + } + orig := paramDiag.Summary + paramDiag.Summary = fmt.Sprintf("Parameter %s: %s", param.Name, orig) + pre.Diagnostics = append(pre.Diagnostics, paramDiag) + } + } + } + } +} + func Preview(ctx context.Context, input Input, dir fs.FS) (output *Output, diagnostics hcl.Diagnostics) { // The trivy package works with `github.com/zclconf/go-cty`. This package is // similar to `reflect` in its usage. This package can panic if types are @@ -180,7 +216,9 @@ func Preview(ctx context.Context, input Input, dir fs.FS) (output *Output, diagn diags := make(hcl.Diagnostics, 0) rp, rpDiags := parameters(modules) - presets := presets(modules, rp) + // preValidPresets are extracted as written in terraform. Each individual + // param value is checked, however the preset as a whole might not valid. + preValidPresets := presets(modules, rp) tags, tagDiags := workspaceTags(modules, p.Files()) vars := variables(modules) @@ -191,7 +229,7 @@ func Preview(ctx context.Context, input Input, dir fs.FS) (output *Output, diagn ModuleOutput: outputs, Parameters: rp, WorkspaceTags: tags, - Presets: presets, + Presets: preValidPresets, Files: p.Files(), Variables: vars, }, diags.Extend(rpDiags).Extend(tagDiags) diff --git a/preview_test.go b/preview_test.go index c75cf6a..b312369 100644 --- a/preview_test.go +++ b/preview_test.go @@ -700,6 +700,55 @@ func Test_Extract(t *testing.T) { } } +func TestPresetValidation(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + dir string + input preview.Input + presetAssert map[string]assertPreset + }{ + { + name: "preset failure", + dir: "presetfail", + input: preview.Input{}, + presetAssert: map[string]assertPreset{ + "invalid_parameters": aPreWithDiags(). + errorDiagnostics("Parameter no_default: Required parameter not provided"), + "valid_preset": aPre(). + value("has_default", "changed"). + value("no_default", "custom value"). + noDiagnostics(), + "prebuild_instance_zero": aPre(), + "not_prebuild": aPre(), + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + dirFs := os.DirFS(filepath.Join("testdata", tc.dir)) + output, diags := preview.Preview(context.Background(), tc.input, dirFs) + if diags.HasErrors() { + t.Logf("diags: %s", diags) + } + require.False(t, diags.HasErrors()) + require.Len(t, diags, 0) + + preview.ValidatePresets(context.Background(), tc.input, output.Presets, dirFs) + for _, preset := range output.Presets { + check, ok := tc.presetAssert[preset.Name] + require.True(t, ok, "unknown preset %s", preset.Name) + check(t, preset) + delete(tc.presetAssert, preset.Name) + } + + require.Len(t, tc.presetAssert, 0, "some presets were not found") + }) + } +} + type assertVariable func(t *testing.T, variable types.Variable) func av() assertVariable { @@ -890,6 +939,71 @@ func (a assertVariable) extend(f assertVariable) assertVariable { } } +type assertPreset func(t *testing.T, preset types.Preset) + +func aPre() assertPreset { + return func(t *testing.T, parameter types.Preset) { + t.Helper() + assert.Empty(t, parameter.Diagnostics, "parameter should have no diagnostics") + } +} + +func aPreWithDiags() assertPreset { + return func(t *testing.T, parameter types.Preset) {} +} + +func (a assertPreset) def(def bool) assertPreset { + return a.extend(func(t *testing.T, preset types.Preset) { + require.Equal(t, def, preset.Default) + }) +} + +func (a assertPreset) value(key, value string) assertPreset { + return a.extend(func(t *testing.T, preset types.Preset) { + v, ok := preset.Parameters[key] + require.Truef(t, ok, "preset parameter %q existence check", key) + assert.Equalf(t, value, v, "preset parameter %q value equality check", key) + }) +} + +func (a assertPreset) errorDiagnostics(patterns ...string) assertPreset { + return a.diagnostics(hcl.DiagError, patterns...) +} + +func (a assertPreset) warnDiagnostics(patterns ...string) assertPreset { + return a.diagnostics(hcl.DiagWarning, patterns...) +} + +func (a assertPreset) diagnostics(sev hcl.DiagnosticSeverity, patterns ...string) assertPreset { + shadow := patterns + return a.extend(func(t *testing.T, preset types.Preset) { + t.Helper() + + assertDiags(t, sev, preset.Diagnostics, shadow...) + }) +} + +func (a assertPreset) noDiagnostics() assertPreset { + return a.extend(func(t *testing.T, preset types.Preset) { + t.Helper() + + assert.Empty(t, preset.Diagnostics, "parameter should have no diagnostics") + }) +} + +//nolint:revive +func (a assertPreset) extend(f assertPreset) assertPreset { + if a == nil { + a = func(t *testing.T, v types.Preset) {} + } + + return func(t *testing.T, v types.Preset) { + t.Helper() + (a)(t, v) + f(t, v) + } +} + func assertDiags(t *testing.T, sev hcl.DiagnosticSeverity, diags types.Diagnostics, patterns ...string) { t.Helper() checks := make([]string, len(patterns)) diff --git a/testdata/presetfail/main.tf b/testdata/presetfail/main.tf new file mode 100644 index 0000000..81c2e31 --- /dev/null +++ b/testdata/presetfail/main.tf @@ -0,0 +1,51 @@ +terraform { + required_providers { + coder = { + source = "coder/coder" + version = "2.8.0" + } + } +} + +data "coder_parameter" "no_default" { + name = "no_default" +} + +data "coder_parameter" "has_default" { + name = "has_default" + default = "hello world" +} + + +data "coder_workspace_preset" "invalid_parameters" { + name = "invalid_parameters" + + prebuilds { + instances = 1 + } +} + +data "coder_workspace_preset" "valid_preset" { + name = "valid_preset" + + parameters = { + "no_default" = "custom value" + "has_default" = "changed" + } + prebuilds { + instances = 1 + } +} + +data "coder_workspace_preset" "prebuild_instance_zero" { + name = "prebuild_instance_zero" + + prebuilds { + // No instances + instances = 0 + } +} + +data "coder_workspace_preset" "not_prebuild" { + name = "not_prebuild" +} \ No newline at end of file diff --git a/types/preset.go b/types/preset.go index da05e41..91ee890 100644 --- a/types/preset.go +++ b/types/preset.go @@ -11,8 +11,13 @@ type Preset struct { Diagnostics Diagnostics `json:"diagnostics"` } +type PrebuildData struct { + Instances int `json:"instances"` +} + type PresetData struct { Name string `json:"name"` Parameters map[string]string `json:"parameters"` Default bool `json:"default"` + Prebuild *PrebuildData } From 1ff03fc184eb956e6905cc78f4d93c999544c12b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Dec 2025 16:40:46 -0600 Subject: [PATCH 2/5] add comments --- preview.go | 17 +++++++++++++---- preview_test.go | 5 ++++- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/preview.go b/preview.go index bf538d1..8ec4aa3 100644 --- a/preview.go +++ b/preview.go @@ -68,9 +68,13 @@ func (o Output) MarshalJSON() ([]byte, error) { }) } -// ValidatePresets will iterate over each preset, validate the inputs as a set, -// and attach any diagnostics to the preset if there are issues. -func ValidatePresets(ctx context.Context, input Input, preValid []types.Preset, dir fs.FS) { +// ValidatePrebuilds will iterate over each preset, validate the inputs as a set, +// and attach any diagnostics to the preset if there are issues. This must be done +// because prebuilds have to be valid without user input. +// +// This will only validate presets that have prebuilds configured and have no existing +// error diagnostics. +func ValidatePrebuilds(ctx context.Context, input Input, preValid []types.Preset, dir fs.FS) []types.Preset { for i := range preValid { pre := &preValid[i] if pre.Prebuild == nil || pre.Prebuild.Instances <= 0 { @@ -78,8 +82,12 @@ func ValidatePresets(ctx context.Context, input Input, preValid []types.Preset, continue } - input.ParameterValues = pre.Parameters + if hcl.Diagnostics(pre.Diagnostics).HasErrors() { + // Piling on diagnostics is not helpful, if an error exists, leave it at that. + continue + } + input.ParameterValues = pre.Parameters output, diagnostics := Preview(ctx, input, dir) if diagnostics.HasErrors() { pre.Diagnostics = append(pre.Diagnostics, diagnostics...) @@ -102,6 +110,7 @@ func ValidatePresets(ctx context.Context, input Input, preValid []types.Preset, } } } + return preValid } func Preview(ctx context.Context, input Input, dir fs.FS) (output *Output, diagnostics hcl.Diagnostics) { diff --git a/preview_test.go b/preview_test.go index b312369..6b11a62 100644 --- a/preview_test.go +++ b/preview_test.go @@ -649,6 +649,9 @@ func Test_Extract(t *testing.T) { } require.False(t, diags.HasErrors()) + // Validate prebuilds too + preview.ValidatePrebuilds(context.Background(), tc.input, output.Presets, dirFs) + if len(tc.warnings) > 0 { for _, w := range tc.warnings { idx := slices.IndexFunc(diags, func(diagnostic *hcl.Diagnostic) bool { @@ -736,7 +739,7 @@ func TestPresetValidation(t *testing.T) { require.False(t, diags.HasErrors()) require.Len(t, diags, 0) - preview.ValidatePresets(context.Background(), tc.input, output.Presets, dirFs) + preview.ValidatePrebuilds(context.Background(), tc.input, output.Presets, dirFs) for _, preset := range output.Presets { check, ok := tc.presetAssert[preset.Name] require.True(t, ok, "unknown preset %s", preset.Name) From e65bdd7c10e26c53832befa5fecca3dd67381f55 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Dec 2025 16:48:47 -0600 Subject: [PATCH 3/5] add happy path test --- preview_test.go | 56 +++++++++++++++++++++++++++++++------ testdata/preset/main.tf | 50 +++++++++++++++++++++++++++++++++ testdata/presetfail/skipe2e | 1 + 3 files changed, 98 insertions(+), 9 deletions(-) create mode 100644 testdata/preset/main.tf create mode 100644 testdata/presetfail/skipe2e diff --git a/preview_test.go b/preview_test.go index 6b11a62..39a164f 100644 --- a/preview_test.go +++ b/preview_test.go @@ -41,12 +41,13 @@ func Test_Extract(t *testing.T) { failPreview bool input preview.Input - expTags map[string]string - unknownTags []string - params map[string]assertParam - variables map[string]assertVariable - presets func(t *testing.T, presets []types.Preset) - warnings []*regexp.Regexp + expTags map[string]string + unknownTags []string + params map[string]assertParam + variables map[string]assertVariable + presetsFuncs func(t *testing.T, presets []types.Preset) + presets map[string]assertPreset + warnings []*regexp.Regexp }{ { name: "bad param values", @@ -266,6 +267,27 @@ func Test_Extract(t *testing.T) { errorDiagnostics("Required"), }, }, + { + name: "valid prebuild", + dir: "preset", + expTags: map[string]string{}, + input: preview.Input{}, + unknownTags: []string{}, + params: map[string]assertParam{ + "number": ap(), + "has_default": ap(), + }, + presets: map[string]assertPreset{ + "valid_preset": aPre(). + value("number", "9"). + value("has_default", "changed"). + prebuildCount(3), + "prebuild_instance_zero": aPre(). + prebuildCount(0), + "not_prebuild": aPre(). + prebuildCount(0), + }, + }, { name: "invalid presets", dir: "invalidpresets", @@ -276,7 +298,7 @@ func Test_Extract(t *testing.T) { "valid_parameter_name": ap(). optVals("valid_option_value"), }, - presets: func(t *testing.T, presets []types.Preset) { + presetsFuncs: func(t *testing.T, presets []types.Preset) { presetMap := map[string]func(t *testing.T, preset types.Preset){ "empty_parameters": func(t *testing.T, preset types.Preset) { require.Len(t, preset.Diagnostics, 0) @@ -688,8 +710,14 @@ func Test_Extract(t *testing.T) { } // Assert presets - if tc.presets != nil { - tc.presets(t, output.Presets) + if tc.presetsFuncs != nil { + tc.presetsFuncs(t, output.Presets) + } + + for _, preset := range output.Presets { + check, ok := tc.presets[preset.Name] + require.True(t, ok, "unknown preset %s", preset.Name) + check(t, preset) } // Assert variables @@ -961,6 +989,16 @@ func (a assertPreset) def(def bool) assertPreset { }) } +func (a assertPreset) prebuildCount(exp int) assertPreset { + return a.extend(func(t *testing.T, preset types.Preset) { + if exp == 0 && preset.Prebuild == nil { + return + } + require.NotNilf(t, preset.Prebuild, "prebuild should not be nil, expected %d instances", exp) + require.Equal(t, exp, preset.Prebuild.Instances) + }) +} + func (a assertPreset) value(key, value string) assertPreset { return a.extend(func(t *testing.T, preset types.Preset) { v, ok := preset.Parameters[key] diff --git a/testdata/preset/main.tf b/testdata/preset/main.tf new file mode 100644 index 0000000..a8d6091 --- /dev/null +++ b/testdata/preset/main.tf @@ -0,0 +1,50 @@ +terraform { + required_providers { + coder = { + source = "coder/coder" + version = "2.8.0" + } + } +} + +data "coder_parameter" "number" { + name = "number" + default = "7" + type = "number" + + validation { + min = 4 + max = 10 + } +} + +data "coder_parameter" "has_default" { + name = "has_default" + default = "hello world" +} + + +data "coder_workspace_preset" "valid_preset" { + name = "valid_preset" + + parameters = { + "number" = "9" + "has_default" = "changed" + } + prebuilds { + instances = 3 + } +} + +data "coder_workspace_preset" "prebuild_instance_zero" { + name = "prebuild_instance_zero" + + prebuilds { + // No instances + instances = 0 + } +} + +data "coder_workspace_preset" "not_prebuild" { + name = "not_prebuild" +} \ No newline at end of file diff --git a/testdata/presetfail/skipe2e b/testdata/presetfail/skipe2e new file mode 100644 index 0000000..3ce3bec --- /dev/null +++ b/testdata/presetfail/skipe2e @@ -0,0 +1 @@ +parameters are invalid without input \ No newline at end of file From 176b0c58b6f428d176c04c46302833a8785d4275 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Dec 2025 16:52:45 -0600 Subject: [PATCH 4/5] fix test --- preview.go | 8 ++++++-- preview_test.go | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/preview.go b/preview.go index 8ec4aa3..2a411bd 100644 --- a/preview.go +++ b/preview.go @@ -74,7 +74,7 @@ func (o Output) MarshalJSON() ([]byte, error) { // // This will only validate presets that have prebuilds configured and have no existing // error diagnostics. -func ValidatePrebuilds(ctx context.Context, input Input, preValid []types.Preset, dir fs.FS) []types.Preset { +func ValidatePrebuilds(ctx context.Context, input Input, preValid []types.Preset, dir fs.FS) { for i := range preValid { pre := &preValid[i] if pre.Prebuild == nil || pre.Prebuild.Instances <= 0 { @@ -87,16 +87,21 @@ func ValidatePrebuilds(ctx context.Context, input Input, preValid []types.Preset continue } + // Diagnostics are added to the existing preset. input.ParameterValues = pre.Parameters output, diagnostics := Preview(ctx, input, dir) if diagnostics.HasErrors() { pre.Diagnostics = append(pre.Diagnostics, diagnostics...) + // Do not pile on more diagnostics for individual params, it already failed + continue } if output == nil { continue } + // If any parameter is invalid, then the preset is invalid. + // A value must be specified for this failing parameter. for _, param := range output.Parameters { if hcl.Diagnostics(param.Diagnostics).HasErrors() { for _, paramDiag := range param.Diagnostics { @@ -110,7 +115,6 @@ func ValidatePrebuilds(ctx context.Context, input Input, preValid []types.Preset } } } - return preValid } func Preview(ctx context.Context, input Input, dir fs.FS) (output *Output, diagnostics hcl.Diagnostics) { diff --git a/preview_test.go b/preview_test.go index 39a164f..de7bc3d 100644 --- a/preview_test.go +++ b/preview_test.go @@ -716,6 +716,10 @@ func Test_Extract(t *testing.T) { for _, preset := range output.Presets { check, ok := tc.presets[preset.Name] + if !ok && tc.presetsFuncs != nil { + // TODO: Convert presetsFunc to presets + continue + } require.True(t, ok, "unknown preset %s", preset.Name) check(t, preset) } From aafd6e4939f5b303e64a0c252370fa120d016e93 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 12 Dec 2025 07:28:05 -0600 Subject: [PATCH 5/5] update asserts to new pattern --- preview_test.go | 61 +++++---------------------------- testdata/invalidpresets/main.tf | 11 +++--- 2 files changed, 14 insertions(+), 58 deletions(-) diff --git a/preview_test.go b/preview_test.go index de7bc3d..32526c6 100644 --- a/preview_test.go +++ b/preview_test.go @@ -298,50 +298,14 @@ func Test_Extract(t *testing.T) { "valid_parameter_name": ap(). optVals("valid_option_value"), }, - presetsFuncs: func(t *testing.T, presets []types.Preset) { - presetMap := map[string]func(t *testing.T, preset types.Preset){ - "empty_parameters": func(t *testing.T, preset types.Preset) { - require.Len(t, preset.Diagnostics, 0) - }, - "no_parameters": func(t *testing.T, preset types.Preset) { - require.Len(t, preset.Diagnostics, 0) - }, - "invalid_parameter_name": func(t *testing.T, preset types.Preset) { - require.Len(t, preset.Diagnostics, 1) - require.Equal(t, preset.Diagnostics[0].Summary, "Undefined Parameter") - require.Equal(t, preset.Diagnostics[0].Detail, "Preset parameter \"invalid_parameter_name\" is not defined by the template.") - }, - "invalid_parameter_value": func(t *testing.T, preset types.Preset) { - require.Len(t, preset.Diagnostics, 1) - require.Equal(t, preset.Diagnostics[0].Summary, "Value must be a valid option") - require.Equal(t, preset.Diagnostics[0].Detail, "the value \"invalid_value\" must be defined as one of options") - }, - "valid_preset": func(t *testing.T, preset types.Preset) { - require.Len(t, preset.Diagnostics, 0) - require.Equal(t, preset.Parameters, map[string]string{ - "valid_parameter_name": "valid_option_value", - }) - }, - } - - for _, preset := range presets { - if fn, ok := presetMap[preset.Name]; ok { - fn(t, preset) - } - } - - var defaultPresetsWithError int - for _, preset := range presets { - if preset.Name == "default_preset" || preset.Name == "another_default_preset" { - for _, diag := range preset.Diagnostics { - if diag.Summary == "Multiple default presets" { - defaultPresetsWithError++ - break - } - } - } - } - require.Equal(t, 1, defaultPresetsWithError, "exactly one default preset should have the multiple defaults error") + presets: map[string]assertPreset{ + "empty_parameters": aPre(), + "no_parameters": aPre(), + "invalid_parameter_name": aPreWithDiags().errorDiagnostics("Preset parameter \"invalid_parameter_name\" is not defined by the template."), + "invalid_parameter_value": aPreWithDiags().errorDiagnostics("the value \"invalid_value\" must be defined as one of options"), + "valid_preset": aPre().value("valid_parameter_name", "valid_option_value"), + "another_default_preset": aPre().def(true), + "default_preset": aPreWithDiags().errorDiagnostics("Only one preset can be marked as default. \"another_default_preset\" is already marked as default"), }, }, { @@ -709,17 +673,8 @@ func Test_Extract(t *testing.T) { check(t, param) } - // Assert presets - if tc.presetsFuncs != nil { - tc.presetsFuncs(t, output.Presets) - } - for _, preset := range output.Presets { check, ok := tc.presets[preset.Name] - if !ok && tc.presetsFuncs != nil { - // TODO: Convert presetsFunc to presets - continue - } require.True(t, ok, "unknown preset %s", preset.Name) check(t, preset) } diff --git a/testdata/invalidpresets/main.tf b/testdata/invalidpresets/main.tf index d9abd56..a7afc28 100644 --- a/testdata/invalidpresets/main.tf +++ b/testdata/invalidpresets/main.tf @@ -46,18 +46,19 @@ data "coder_workspace_preset" "valid_preset" { } } -data "coder_workspace_preset" "default_preset" { - name = "default_preset" +data "coder_workspace_preset" "another_default_preset" { + name = "another_default_preset" parameters = { "valid_parameter_name" = "valid_option_value" } default = true } -data "coder_workspace_preset" "another_default_preset" { - name = "another_default_preset" +data "coder_workspace_preset" "default_preset" { + name = "default_preset" parameters = { "valid_parameter_name" = "valid_option_value" } default = true -} \ No newline at end of file +} +