- Notifications
You must be signed in to change notification settings - Fork1.1k
feat: use preview to compute workspace tags from terraform#18720
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
Emyrk commentedJul 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed byGraphite. Learn more aboutstacking. |
6c1fdf5 to20d0254Compare15a6ca1 tof0dd768Compare6f3d388 tof4cd152Comparef0dd768 toa87678dCompare29a7cf6 to4d06a6cCompare| "github.com/coder/coder/v2/testutil" | ||
| ) | ||
| funcTest_DynamicWorkspaceTagDefaultsFromFile(t*testing.T) { |
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.
This test is copied fromtfparse_test.go.
The tfparse test can be removed when this becomes the new standard.
96c83c7 to8d594a7Comparefc1ad45 to11e0407Compare11e0407 tod1ed169Compare35d29b9 toea564d1CompareUh oh!
There was an error while loading.Please reload this page.
| return | ||
| } | ||
| vardynamicTemplatebool |
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.
Unfortunate, but orphan'd template versions can never use dynamic parameters 😢
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.
By 'orphaned' you mean !(active version)?
I wonder how this squares with the UI option you get to create a workspace from this template version when you create a new template version.
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, I mean when you first create a new template, the flow is:
- Create template version
- Create template using template version
So in step 1, the template version is orphaned. And it can remain that way.
Uh oh!
There was an error while loading.Please reload this page.
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.
Pull Request Overview
This PR replaces the tfparse-based workspace tag extraction with thepreview SDK for dynamic-parameter templates, adds caching in the workspace builder, and expands end-to-end and unit tests for tag parsing.
- Upgrade to
github.com/coder/previewv1.0.3 for tag extraction - Implement
dynamicTemplateVersionTags/classicTemplateVersionTagsin the API and cache tags in the workspace builder - Add
tags_internal_test.goand integration testTestDynamicWorkspaceTags, plus a zip‐FS helper
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Bumpgithub.com/coder/preview to v1.0.3-… |
| enterprise/coderd/testdata/.../main.tf | Add Terraform test case for workspace tags |
| enterprise/coderd/dynamicparameters_test.go | AddTestDynamicWorkspaceTags integration test |
| coderd/wsbuilder/wsbuilder.go | Cache and return dynamic workspace tags |
| coderd/templateversions.go | Route tag parsing throughpreview.Preview for dynamic templates |
| coderd/dynamicparameters/tags.go | AddCheckTags and diagnostics for invalid tag values |
| coderd/dynamicparameters/error.go | Rename error constructors and update error messaging |
| coderd/dynamicparameters/render.go | Refactor workspace owner resolution and add version support |
| coderd/dynamicparameters/resolver.go | Rename error calls to lowercase versions |
| coderd/coderdtest/dynamicparameters.go | ExposeVersion hook in test helper |
| archive/fs/zip.go | IntroduceFromZipReader helper for zip archives |
Comments suppressed due to low confidence (4)
coderd/templateversions.go:1765
- [nitpick] This new helper method lacks a GoDoc comment; consider adding a brief comment explaining its behavior and parameters.
func (api *API) dynamicTemplateVersionTags(ctx context.Context, rw http.ResponseWriter, orgID uuid.UUID, owner uuid.UUID, file database.File) (map[string]string, bool) {coderd/templateversions.go:1795
- The unsupported mimetype branch in
dynamicTemplateVersionTagsisn’t exercised by existing tests; consider adding a test to cover this error path.
default:archive/fs/zip.go:1
- The new
FromZipReaderhelper lacks unit tests; consider adding tests to validate correct in-memory FS creation from zip data.
package archivefscoderd/dynamicparameters/error.go:21
- [nitpick] The error message is now "Failed to parse provisioner tags", which may be confusing in a workspace-tag context; consider using "workspace tags" for consistency with other messages.
func tagValidationError(diags hcl.Diagnostics) *DiagnosticError {Uh oh!
There was an error while loading.Please reload this page.
ea564d1 to76ca1faCompare
johnstcn left a comment
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.
🐐
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| }, | ||
| expectTags:map[string]string{"cluster":"developers","platform":"kubernetes","region":"us"}, | ||
| expectedFailedTags:map[string]string{ | ||
| "az":"Tag value is not known, it likely refers to a variable that is not set or has no default.", |
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.
review: this also differs slightly from tfparse which returns an empty value, but the empty result is checked externally IIRC
| expectedFailedTags:map[string]string{ | ||
| "hostname":unknownTag, | ||
| }, |
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.
review: same result as tfparse, but the error is nicer here 👍
Uh oh!
There was an error while loading.Please reload this page.
| "az":"a", | ||
| }, | ||
| expectedFailedTags:map[string]string{ | ||
| "some_path":unknownTag, |
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.
review: it might be better to have an explicit "function not allowed" error here
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.
You can actually use the function, but it's the~ in the function call. The~ cannot be expanded inpreview, so that invocation of the function fails. I'll make a comment
| expectTags:map[string]string{ | ||
| "stringvar":"a", | ||
| "numvar":"1", | ||
| "boolvar":"true", | ||
| "stringparam":"a", | ||
| "numparam":"1", | ||
| "boolparam":"true", | ||
| "listparam":`["a", "b"]`,// OK because params are cast to strings | ||
| }, | ||
| expectedFailedTags:map[string]string{ | ||
| "listvar":invalidValueType, | ||
| "mapvar":invalidValueType, | ||
| }, |
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.
review: behavioural difference from tfparse, but this was discussed and agreed to be correct as the provider will complain about the type not being stringy enough
76ca1fa toff0cf69Comparea099a8a intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
What this does
If using dynamic parameters, workspace tags are extracted using
coder/preview. This occurs at template version import and workspace create.Templates that are not opted into dynamic parameters will use
tfparse.Future Work
When
previewis used as the source of truth for tags, we can removetfparse.Closescoder/internal#698