feat: ValidatePrebuilds to validate presets for prebuild use #194
Conversation
Emyrk
changed the title
cstyan
left a comment
cstyan
left a comment
There was a problem hiding this comment.
approving to unblock, just had some minor comments/questions
|
|
||
| func PresetFromBlock(block *terraform.Block) types.Preset { | ||
| func PresetFromBlock(block *terraform.Block) (tfPreset types.Preset) { | ||
| defer func() { |
There was a problem hiding this comment.
did you run into a case where this can panic?
There was a problem hiding this comment.
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.
| 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), | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
This is just a safety for when we use cty. It helps prevent production breaks from 1 unexpected template value.
|
|
||
| func PresetFromBlock(block *terraform.Block) types.Preset { | ||
| func PresetFromBlock(block *terraform.Block) (tfPreset types.Preset) { | ||
| defer func() { |
There was a problem hiding this comment.
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.
ssncferreira
left a comment
ssncferreira
left a comment
There was a problem hiding this comment.
LGTM, just a couple of comments to better understand these changes
| 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.
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 prebuilds.instances in terraform-provider. Is extracting it here in preview just for reading the value to decide whether to run ValidatePrebuilds(), or is there additional validation happening that terraform-provider doesn't cover?
There was a problem hiding this comment.
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.
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.
We already validate prebuilds.instances in terraform-provider.
Yes, and that should be the source of truth for validation.
Is extracting it here in preview just for reading the value to decide whether to run ValidatePrebuilds()
Exactly. If the instance count is 0, then the preset values are allowed to be a partially complete form. If the instance count is >0, then the preset values must be complete (meaning all required form values are filled in).
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)
ssncferreira
left a comment
ssncferreira
left a comment
There was a problem hiding this comment.
LGTM 💪 just a couple of non-blocking comments.
Thank you for addressing the comments ❤️
| // Imports as a protection against invalid presets. A template can still be | ||
| // usable if it's presets are invalid. It just cannot be used for prebuilds. |
There was a problem hiding this comment.
A template can still be usable if it's presets are invalid. It just cannot be used for prebuilds.
I'm a bit confused with this sentence, is this to say that it is still possible to create regular (non-prebuilt) workspaces? And only in case a preset is invalid, it cannot be used for creating prebuilt workspaces?
There was a problem hiding this comment.
I'm a bit confused with this sentence, is this to say that it is still possible to create regular (non-prebuilt) workspaces?
Yes. A preset is allowed to be invalid because it can be partial. You could imagine a template with 5 required params. If the preset selects 3 values, then it's presets are invalid by themselves. It requires the presets + some human values in the form. And things can be dynamic, so a value that was not set can change the preset validation logic.
For prebuilds, there is no user prompt. So it is an invalid prebuild.
There was a problem hiding this comment.
Makes sense 👍 nit: updating the comment, maybe something like:
A preset doesn't need to specify all required parameters, as users can provide the remaining values when creating a workspace. However, since prebuild creation has no user input, presets used for prebuilds must provide all required parameter values.
Co-authored-by: Susana Ferreira <[email protected]>
3 participants
Presets were being created with prebuild counts, even if the preset was unable to be used for a prebuild. This creates a function that can validate prebuilds.
Related: coder/coder#21237
Goal is to prevent creating templates with prebuilds that will just spam failures.