-
Notifications
You must be signed in to change notification settings - Fork 899
chore: add form_type parameter argument to db #17920
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
Conversation
// - Add `form_type` field to parameters | ||
const ( | ||
CurrentMajor = 1 | ||
CurrentMinor = 6 |
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.
v1.6 has not been released yet. So joining the 1.6 version train
coderd/workspaces_test.go
Outdated
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'd like to see a test here for the kind of build inputs that could trigger the new error path in codersdk added below.
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.
The previous code was stricter (not allowing multi-select).
So the new code allows more inputs, rather than removing.
But I will hit the new error path 👍
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.
Added TestWorkspaceWithMultiSelectFailure
The enum types are defined here: https://github.com/coder/terraform-provider-coder/blob/main/provider/formtype.go#L41-L53 You can see the list in our docs here: https://coder.com/docs/admin/templates/extending-templates/parameters#available-form-input-types
All previous parameters that were valid, are still valid. This change adds the validation case for
Since we source this value from terraform (tfstate), and the terraform validates it. I do not see why we need to have additional strictness in So I prefer not to enforce an enum, and allow some version dsync between the provider & coder. Solving that version desync is not something I want to tackle right now. We allow it today.
Now if the form type is data "coder_parameter" "color" {
name = "color"
type = "list(string)"
form_type = "multi-select"
default = `["red","blue"]`
option {
name = "Red"
value = "red"
}
option {
name = "Blue"
value = "blue"
}
option {
name = "Green"
value = "green"
}
} It affects more than deleting, but I hit this because of dynamic parameters. Dynamic parameters skip the The deleting bug was triggered if you start with dynamic parameters, and switch back to the classic parameter flow (it's opt in, opt out with the experiment enabled). Switching back had the new multi-select params, without the correct validation. |
This reasoning doesn't make sense to me. If we add a new form type to the Coder Terraform provider, and a template author uses it in a template with an old version of Coderd that doesn't support that new form type, we should not allow that template to be successfully imported. Otherwise, how could we possibly do the right thing when it comes time to actually render and validate the form? The bug you are hitting with This is exactly the kind of thing that using API versions on the provisionerd protocol is designed to handle. If
I'm concerned about this ability to opt in to dynamic parameters and then opt back out with the same template version. As we add new capabilities to dynamic parameters are we really signing ourselves up to maintain absolute parity via the classic parameter flow? |
Based on @spikecurtis's suggestions, I will make this an enum. I am not going to add backwards support for mulit-select, so if no form value comes in, I will not let it pass the This strictness will be intentional to support the new features. |
codersdk/templateversions.go
Outdated
@@ -59,6 +59,7 @@ type TemplateVersionParameter struct { | |||
Description string `json:"description"` | |||
DescriptionPlaintext string `json:"description_plaintext"` | |||
Type string `json:"type" enums:"string,number,bool,list(string)"` | |||
FormType string `json:"form_type"` |
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.
don't we at least want an enums:
tag?
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.
Ah, TIL enums
is a swagger doc, not a validation thing. This is never consumed as input (an api request). I'll add it for the swagger docs.
form_type
is a new parameter field in the terraform provider. Bring that field into coder/coder.Validation for
multi-select
has also been added.Partially closes #17892