- Notifications
You must be signed in to change notification settings - Fork907
chore: disable parameter validatation for dynamic params for all transitions#17926
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
Validation drift prevents builds from being submitted, suchas deleting
coderd/workspacebuilds.go Outdated
| // Only defer to dynamic parameters if the experiment is enabled. | ||
| if api.Experiments.Enabled(codersdk.ExperimentDynamicParameters) && | ||
| createBuild.EnableDynamicParameters != nil { | ||
| // Explicit opt-in | ||
| builder = builder.DynamicParameters(*createBuild.EnableDynamicParameters) | ||
| } |
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 provide the explicit opt in as a way to force through transitions such asDELETE if there an issue. It would be unfortunate to have to change a template wide setting to fix a single workspace if we get into a strange state.
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 drop a WARN or something if the build requests dynamic parameters but the experiment is not enabled?
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.
Uh oh!
There was an error while loading.Please reload this page.
coderd/workspacebuilds.go Outdated
| // Only defer to dynamic parameters if the experiment is enabled. | ||
| if api.Experiments.Enabled(codersdk.ExperimentDynamicParameters) && | ||
| createBuild.EnableDynamicParameters != nil { | ||
| // Explicit opt-in | ||
| builder = builder.DynamicParameters(*createBuild.EnableDynamicParameters) | ||
| } |
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 drop a WARN or something if the build requests dynamic parameters but the experiment is not enabled?
| func ProvisionerVersionSupportsDynamicParameters(version string) bool { | ||
| major, minor, err := apiversion.Parse(version) | ||
| // If the api version is not valid or less than 1.6, we need to use the static parameters | ||
| useStaticParams := err != nil || major < 1 || (major == 1 && minor < 6) | ||
| return !useStaticParams | ||
| } |
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.
nit: this feels like something that might want to be inprovisionersdk
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.
Hmm. Really tough to know where to place it. I'm going to keep it with wsbuilder right now, since it only affects workspace builds.
I think I'll create a new package for dynamic params more generally as more code is added to support the feature, and I can move it there.
e76d58f 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
Dynamic params currently has relaxed parameter validation in coder/coder. This is because conditional parameters cannot be validated with the static parameters in the database.
We might want to tighten this up later based on the
coderprovider version in the terraform. As version >=2.4.0 all have the strict validation built into the provider. And the UI form relies on the preview engine for validation.Unit test
A unit test of validation drift is present. Without this dynamic params option in wsbuilder, workspace transactions fail to validate, and will never succeed.
This puts the workspace in a locked position where you cannot change its state.
The error is:
This would also break updates, auto-starts, auto-stop, deletes, etc.
Future work
Before we exit experimental, we should revisit if parameter validation is critical in coder/coder (for provider versions >2.4.0).
Validation in coder/coder is for raising errors early, rather than waiting for the terraform logs. For any action without the ability to change params, this does not really provide much help, as there is no way to resolve the issue.
We can always hook up dynamic params to more endpoints, the cost is just a bit of memory loading in all the files.
#17942
Note
This currently only applies to deployments that enable the experiment. Historical versions enforce the previous validation always.
Templates can additionally be opt-in or opt-out of dynamic params. Meaning a deployment with the experiment could have some templates testing the new params, and some on the old.