- Notifications
You must be signed in to change notification settings - Fork917
chore(docs): update documentation for coder_workspace_tags#15620
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.
Left a few commnents but looks good to me.
| - You will need to have an [external provisioner](https://coder.com/docs/admin/provisioners#external-provisioners) with the correct tagset running in order to import this template. | ||
| - When specifying values for the `coder_workspace_tags` data source, you are restricted to using a subset of Terraform's capabilities. | ||
| - You must specify default values for all data sources and variables referenced by the `coder_workspace_tags` data source. |
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.
Are we exposing an explicit error if this is not the case to inform the user on what they can do to fix it. Think about it in context of#15447 .
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.
See below comment, but TL;DR yes.
| You mayonly specify the following as inputs for `coder_workspace_tags`: | ||
| | | Example | | ||
| | :----------------- | :-------------------------------------------- | | ||
| | Static values | `"developers"` | | ||
| | Template variables | `var.az` | | ||
| | Coder parameters | `data.coder_parameter.runtime_selector.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.
But the value of thecoder_parameter can itself come from any otherlocal,resource, ordata. Right?
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.
No, the default must be specified in thecoder_parameter data source. I'll make this more explicit.
| All template variables and `coder_parameter` data sources **must** provide a | ||
| default value. Failure to do so will result in an error. |
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.
Can we be sure that the error is explicit and inform the user on how to fix it?
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.
There are some UI improvements to be made, but here are the main messages:
- Missing
tagsattribute ondata.coder_workspace_tags:https://github.com/coder/coder/blob/main/provisioner/terraform/tfparse/tfparse_test.go#L85 - Missing a default value for a tag:https://github.com/coder/coder/blob/main/provisioner/terraform/tfparse/tfparse_test.go#L187
- Disallowed data source for
coder_workspace_tagsvalue:https://github.com/coder/coder/blob/main/provisioner/terraform/tfparse/tfparse_test.go#L290 - Disallowed resource source for
coder_workspace_tagsvalue:https://github.com/coder/coder/blob/main/provisioner/terraform/tfparse/tfparse_test.go#L324 (This one needs to be improved) - Disallow function call in
coder_workspace_tagsvalue:https://github.com/coder/coder/blob/main/provisioner/terraform/tfparse/tfparse_test.go#L356
I'd rather not over-index on documenting package-level errors in documentation, but I can document these if you think it's necessary?
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.
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.
It should end up in the API response returned by the backend, so all the FE needs to do is display it to the user 👍
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 think we should create an issue in coder/internal to track adding terraform errors to API response.
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, but this might already be in the scope of#15447
Your call though!
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.
Yes ✅
Uh oh!
There was an error while loading.Please reload this page.
e550365 intomainUh oh!
There was an error while loading.Please reload this page.
Adds documentation relating to#15087 and#15427
See also:#15578