-
Notifications
You must be signed in to change notification settings - Fork 948
feat: include template variables in dynamic parameter rendering #18819
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
Conversation
e1915e7
to
a8bdd04
Compare
// Defaulting the empty type to "string" | ||
// TODO: This does not match the terraform behavior, however it is too late | ||
// at this point in the code to determine this, as the database type stores all values | ||
// as strings. The code needs to be fixed in the `Parse` step of the provisioner. | ||
// That step should determine the type of the variable correctly and store it in the database. | ||
case "string", "": | ||
ctyVals[v.Name] = cty.StringVal(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.
This is not ideal, will resolve in another PR
// TODO: This Parse is incomplete. It uses tfparse instead of terraform. | ||
// The inputs are incomplete, as values such as the user context, parameters, | ||
// etc are all important to the parsing process. This should be replaced with | ||
// preview and have all inputs. |
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 need to fix this to remove the need for tfparse
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughThe changes introduce support for passing and handling user-supplied template variable values in dynamic parameter flows, aligning behavior with classic flows. This includes updates to data structures, dynamic parameter rendering, test coverage, and workspace builder logic. Additional refactoring and minor improvements are made to related tests and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant DB
participant DynamicRenderer
User->>API: Submit template version with variable values
API->>DB: Store template and variables
API->>DynamicRenderer: Prepare dynamic renderer (with variables)
DynamicRenderer->>DB: Load template variable values (if not provided)
DynamicRenderer->>DynamicRenderer: Convert variables to TFVars
DynamicRenderer->>API: Return rendered parameters (using TFVars)
API->>User: Respond with rendered parameters
Estimated code review effort3 (~45 minutes) Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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 implements support for template variables in dynamic parameter rendering, allowing templates to access variable values during the dynamic parameter resolution process. Previously, template variables were not available when rendering dynamic parameters, limiting the flexibility of parameter definitions.
- Adds template variable support to the dynamic parameter rendering system
- Updates the preview library dependency to enable template variable functionality
- Includes comprehensive test coverage for both "classic" and "dynamic" parameter flows
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
provisioner/terraform/parse.go |
Adds TODO comment about incomplete Parse function implementation |
go.mod |
Updates preview library dependency to newer version |
enterprise/coderd/workspaces_test.go |
Refactors test to support both dynamic and classic parameter flows |
coderd/wsbuilder/wsbuilder.go |
Integrates template variables into dynamic parameter renderer |
coderd/testdata/parameters/variables/main.tf |
Adds test terraform configuration for variable testing |
coderd/templateversions.go |
Passes template variables to dynamic tag rendering |
coderd/parameters_test.go |
Adds test case for template variables in dynamic parameters |
coderd/dynamicparameters/variablevalues.go |
Implements variable value conversion to cty.Value format |
coderd/dynamicparameters/render.go |
Integrates template variable values into dynamic renderer |
coderd/coderdtest/dynamicparameters.go |
Updates test helper to support template variables |
Comments suppressed due to low confidence (1)
enterprise/coderd/workspaces_test.go:3178
- The change from
-time.Second*10
to-time.Second
reduces the time tolerance significantly. This change appears unrelated to the PR's purpose and could make the test flaky if there are timing variations.
require.WithinRange(t, *workspace.DormantAt, time.Now().Add(-time.Second), time.Now())
// TODO: Does this break if the type is not a string? | ||
tfVarValues := make(map[string]cty.Value) | ||
for _, variable := range templateVariables { | ||
tfVarValues[variable.Name] = cty.StringVal(variable.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.
AFAICT VariableValue.Value
will always be a string?
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 is, which I just found strange. This is working for now, I will test complex behavior as I edit the source of this (the parse function)
Closes #18671
Template variables now loaded into dynamic parameters. Future work is #18819 (comment) & #18819 (comment). Both are related
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores