- Notifications
You must be signed in to change notification settings - Fork921
chore: refactor dynamic parameters into dedicated package#18420
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
37bfb5c to758cd26Compare| // TestDynamicParameterTemplate uses a template with some dynamic elements, and | ||
| // tests the parameters, values, etc are all as expected. | ||
| funcTestDynamicParameterTemplate(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 actually kinda dope. I like the resulting syntax
| @@ -81,292 +68,49 @@ func (api *API) templateVersionDynamicParametersWebsocket(rw http.ResponseWriter | |||
| })(rw,r) | |||
| } | |||
| //nolint:revive // listen is a control flag | |||
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.
Why is a control flag acceptable or desired here if the linter discourages it? Perhaps we should include that justification in the 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.
Added 👍
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 understood as much as you've described in the new comment. My question is why this needs to be a "control flag" in this way instead of being extracted from the context inside this endpoint. They're functionally equivalent as far as I can tell. I'm not saying one approach is better.
I'm asking because I'm sure the linting rule was added for a reason and//nolint can be a smell unless it's well justified.
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.
As I read it, you are right. I could refactor this to remove the control flag now that thedynamicparameters package exists.
I'm going to keep it for now, and I'm going to make an issue to revisit this.
#18478
It is on the parameters board, so I can fix this up in my next maintenance cycle 👍
Uh oh!
There was an error while loading.Please reload this page.
coderd/dynamicparameters/static.go Outdated
| } | ||
| varprotoOptions []*sdkproto.RichParameterOption | ||
| _=json.Unmarshal(it.Options,&protoOptions)// Not going to make this fatal |
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.
If we're going to comment this, can we documentwhy we ignore this error, so that future contributors know when it may or may not be useful to reassess this? Is it simply because we're pretty sure this will be valid json since it comes from the DB? I'd like to avoid Chesterton's Fence.
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.
Actually, I can throw this error on the parameter now as a diagnostic. I'm going to toss the error in there. 👍
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.
Added it as a diagnostic on the param 👍
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 refactors dynamic parameter rendering out ofcoderd/parameters.go into a newcoderd/dynamicparameters package, unifies static and dynamic parameter handling under a single interface, and adds test utilities and fixtures for exercising dynamic parameters.
- Extract dynamic parameter logic into
dynamicparameterspackage withLoader/Renderer - Update API handlers in
coderd/parameters.goto use the new package - Add comprehensive tests and helpers for dynamic parameter rendering, plus a Terraform test fixture
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| enterprise/coderd/testdata/parameters/dynamic/main.tf | New Terraform fixture demonstrating dynamic parameters |
| enterprise/coderd/parameters_test.go | Tests updated to useSynchronousStream helper |
| enterprise/coderd/dynamicparameters_test.go | End-to-end tests for dynamic parameters via WebSocket/API |
| coderd/util/slice/slice.go | NewConvert utility to map slices |
| coderd/parameters_test.go | Refactored setup to use unifiedDynamicParameterTemplate |
| coderd/parameters.go | Switched todynamicparameters.Loader for rendering logic |
| coderd/dynamicparameters/static.go | Static renderer implementation for legacy parameter flow |
| coderd/dynamicparameters/render.go | Dynamic renderer andRenderer interface implementation |
| coderd/coderdtest/stream.go | AddedSynchronousStream test helper |
| coderd/coderdtest/dynamicparameters.go | High-level helper for template/dynamic parameter tests |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Emyrk commentedJun 20, 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. |
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'm new to the dynamic parameter space, but I'm pretty confident I understand how this all fits together.
9b5d499 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Refactor dynamic parameters into dedicated package
This PR extracts dynamic parameter rendering logic from coderd/parameters.go into a new coderd/dynamicparameters package. Partly for organization and maintainability, but primarily to be reused in
wsbuilderto be leveraged as validation.Key changes:
previewThe refactoring maintains backward compatibility while creating a cleaner separation of concerns for parameter handling logic. The next step is to use this logic in wsbuilder for validation purposes.