- Notifications
You must be signed in to change notification settings - Fork22
feat: Add legacy_variable_name to parameter#110
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 descriptions could be slightly clarified, but other than that I think this looks good! 👍🏻
docs/data-sources/parameter.md Outdated
| @@ -24,7 +24,8 @@ Use this data source to configure editable options for workspaces. | |||
| - `default` (String) A default value for the parameter. | |||
| - `description` (String) Describe what this parameter does. | |||
| - `icon` (String) A URL to an icon that will display in the dashboard. View built-in icons here: https://github.com/coder/coder/tree/main/site/static/icon. Use a built-in icon with `data.coder_workspace.me.access_url + "/icon/<path>"`. | |||
| - `legacy_variable` (String) The name of the Terraform variable used by legacy parameters. Coder will use it to lookup the parameter value. | |||
| - `legacy_variable` (String) Reference to the legacy parameter. Coder will use it to lookup the default value. | |||
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.
Maybe we could call thislegacy_variable_{,default_}value to clearly indicate the difference between name/default value?
PS. It might be a bit confusing to users to call thesevariable when the description mentions parameters. Perhaps reword the description to only mention variables and not parameters? (Same 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.
I rephrased the sentence to refer only to variables.
Maybe we could call this legacy_variable_{,default_}value to clearly indicate the difference between name/default value?
I'm hesitant as I'd rather see template authors referring to variables. I have a feeling thatlegacy_variable_{,default_}value may suggest inputting a number instead of${var.foobar}. Actually, it's unfortunate that we need a field for a variable name, but Terraform can't operate on metadata (resource name).
Besides that, this is only for migration purposes. Thoselegacy_* should be present only for one update cycle of a template. I think we're good...
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 agree with your reasoning for keeping the name as-is. 👍🏻
| "legacy_variable_name": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| RequiredWith: []string{"legacy_variable"}, |
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.
Great that you added this requirement! 🤩
Related:coder/coder#6368
This PR adds new property to
coder_parameter-legacy_variable_name.It's outcome of the researchhere.