Skip to content

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

Merged
merged 61 commits into from
May 29, 2025
Merged

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented May 19, 2025

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

Comment on lines +28 to 31
// - Add `form_type` field to parameters
const (
CurrentMajor = 1
CurrentMinor = 6
Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

@Emyrk Emyrk May 19, 2025

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 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added TestWorkspaceWithMultiSelectFailure

@Emyrk Emyrk changed the title chore: bring form_type parameter argument into db chore: add form_type parameter argument to db May 19, 2025
@Emyrk Emyrk requested a review from johnstcn May 19, 2025 20:13
@spikecurtis
Copy link
Contributor

@Emyrk can you talk about back compatibility and why you're not making this "type" field an enum (in the database and in the proto), or point to a design doc if one exists and I just missed it.

Also, I don't understand how form_type relates to #17892 which is about deleting workspaces.

@Emyrk
Copy link
Member Author

Emyrk commented May 20, 2025

@Emyrk can you talk about back compatibility and why you're not making this "type" field an enum (in the database and in the proto), or point to a design doc if one exists and I just missed it.

Also, I don't understand how form_type relates to #17892 which is about deleting workspaces.

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

can you talk about back compatibility

All previous parameters that were valid, are still valid. This change adds the validation case for multi-select.

why you're not making this "type" field an enum (in the database and in the proto)

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 coder/coder. If we were to change the terraform to add, remove, or edit a form type, then previous versions of coder would break on the new terraform provider. As the old enum would be stale.

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.

Also, I don't understand how form_type relates to #17892 which is about deleting workspaces.

multi-select has different validation then is currently supported. This is what a multi-select looks like. Before this PR, all list(string) validation assumed the string literal value ["red","blue"] would exist in the options.

Now if the form type is multi-select, we know the string literal "red" and "blue" have to each exist in the options.

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 coder/coder validation of their parameters atm. We defer to the terraform to validate.

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.

@spikecurtis
Copy link
Contributor

If we were to change the terraform to add, remove, or edit a form type, then previous versions of coder would break on the new terraform provider. As the old enum would be stale.

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.

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 multi-select is a direct example of this problem: the provider allows a value that a back-level Coderd does not understand and will incorrectly validate.

This is exactly the kind of thing that using API versions on the provisionerd protocol is designed to handle. If multi-select was first supported in v1.6 of the protocol, and it turns out your Coderd only supports v1.5, then you have some indication that it needs to be handled, either by rejecting multi-selects, or outright refusing to connect to Coderd at all (it would be very strange and rare for provisionerd to be at a higher version than coderd, so erroring out entirely is reasonable IMO).

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.

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?

@Emyrk
Copy link
Member Author

Emyrk commented May 21, 2025

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 multi-select validation.

This strictness will be intentional to support the new features.

@@ -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"`
Copy link
Contributor

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?

Copy link
Member Author

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.

@Emyrk Emyrk requested a review from spikecurtis May 28, 2025 16:17
@Emyrk Emyrk merged commit 8387dd2 into main May 29, 2025
39 checks passed
@Emyrk Emyrk deleted the stevenmasley/param_form_type branch May 29, 2025 13:55
@github-actions github-actions bot locked and limited conversation to collaborators May 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: deleting a workspace using dynamic params fails due to params
3 participants