-
Notifications
You must be signed in to change notification settings - Fork 3
feat: ValidatePrebuilds to validate presets for prebuild use #194
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
bbc1114
1ff03fc
e65bdd7
176b0c5
aafd6e4
c6db0b6
13f6911
3400cff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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), | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
| }() | ||
|
Comment on lines
+13
to
+30
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just a safety for when we use |
||
|
|
||
| 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")), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to make sure I understand how preview and terraform-provider interact. IIUC, when updating a template via the UI and hitting build, preview runs first (static HCL analysis), then terraform plan/apply runs with terraform-provider validation. We already validate
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes this is correct. The static HCL analysis is also a "best guess" and I try to error on the permissive side when things cannot be determined. The terraform provider should always be a more restrictive validation.
Yes, and that should be the source of truth for validation.
Exactly. If the instance count is So this function is a check that says for all presets that will be used in prebuilds, are their values completely valid for a workspace build? (without any additional user input) |
||
| } | ||
| } | ||
|
|
||
| return p | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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) | ||||||||||||||||||
|
|
@@ -649,6 +671,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 { | ||||||||||||||||||
|
|
@@ -685,8 +710,18 @@ 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] | ||||||||||||||||||
| if !ok && tc.presetsFuncs != nil { | ||||||||||||||||||
| // TODO: Convert presetsFunc to presets | ||||||||||||||||||
Emyrk marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||
| continue | ||||||||||||||||||
| } | ||||||||||||||||||
| require.True(t, ok, "unknown preset %s", preset.Name) | ||||||||||||||||||
| check(t, preset) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // Assert variables | ||||||||||||||||||
|
|
@@ -700,6 +735,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", | ||||||||||||||||||
ssncferreira marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
| 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(), | ||||||||||||||||||
Emyrk marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||
| }, | ||||||||||||||||||
| }, | ||||||||||||||||||
| } { | ||||||||||||||||||
| 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.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) | ||||||||||||||||||
| 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 +974,81 @@ 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") | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+935
to
+938
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| 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) 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] | ||||||||||||||||||
| 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)) | ||||||||||||||||||
|
|
||||||||||||||||||
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.
did you run into a case where this can panic?
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 wish I had receipts. I did not find a location where presets panic'd, but we've hit panics in the past when converting
cty-> normal types.Since the preset code is not tested that much, I just feel more comfortable with this safeguard in place. A panic never raises well, and we hit it a few times in the early days of parameters.
This is just being defensive.