feat: Expose managed variables via API #6134
Conversation
coderd/database/migrations/000098_template_version_variables.up.sql
Outdated
Show resolved
Hide resolved
coderd/database/migrations/000098_template_version_variables.up.sql
Outdated
Show resolved
Hide resolved
| template_version_id uuid not null references template_versions (id) on delete cascade, | ||
| name text not null, | ||
| description text not null, | ||
| type text not null, |
There was a problem hiding this comment.
I saw this was listed as an enum in the API, should we turn it into an enum in the DB as well?
There was a problem hiding this comment.
To be honest, we don't intend to run any conditional logic based on the type, so I'm not convinced. Terraform template ensures the type validation here, so only TF var types are welcome. Also, we don't do this validation on rich parameters, so I would leave it consistent with them.
| type TemplateVersionVariable struct { | ||
| Name string `json:"name"` | ||
| Description string `json:"description"` | ||
| Type string `json:"type" enums:"string,number,bool"` |
There was a problem hiding this comment.
I didn't see that these enum values are enforced anywhere, should they be? (I posted a suggestion in the migration to use database enum.)
| if err != nil { | ||
| return nil, xerrors.Errorf("parse variable %q default: %w", variable.Name, err) | ||
| } | ||
| defaultData = string(defaultDataRaw) |
There was a problem hiding this comment.
I'm not sure what variable.Default looks like, so wondering if the idea with json marshaling here is to turn a string value like hello into "hello"? Or maybe the purpose is entirely different 😄
There was a problem hiding this comment.
According to Terraform, the variable can have simple types like strings... but also lists, arrays, etc. hence marshaling. Anyway, the idea is not new but borrowed from convertVariableToParameter.
| @@ -175,7 +192,8 @@ message Parse { | |||
| string directory = 1; | |||
| } | |||
| message Complete { | |||
| repeated ParameterSchema parameter_schemas = 2; | |||
| repeated ParameterSchema parameter_schemas = 1; | |||
There was a problem hiding this comment.
Is it safe/backwards compatible to change number here?
There was a problem hiding this comment.
Ok, let's reorder these just in case. BTW for some reason, parameter_schemas was the 2nd item, but no 1st field?
| @@ -175,6 +192,7 @@ message Parse { | |||
| string directory = 1; | |||
| } | |||
| message Complete { | |||
| repeated TemplateVariable template_variables = 1; | |||
| repeated ParameterSchema parameter_schemas = 2; | |||
There was a problem hiding this comment.
@kylecarbs is this safe or should we leave 1 empty and add template_variables as 3?
There was a problem hiding this comment.
I think it should be fine.
Customers aren't running provisioner daemons externally at the moment, so this incompatibility should be alright.
Related: #5980
This PR introduces the concept of template version variables, that are pulled from an imported template, and stored in the database table. If the imported variable is marked as required, a user is expected to pass the value, otherwise, the import job fails fast.
To use managed template variables, the user is required to define an extra flag in the template:
feature_use_managed_variables/variables