-
Notifications
You must be signed in to change notification settings - Fork 886
Rich Parameter validation is skipped if min or max is 0. #7582
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
Comments
Thanks for linking the source code. It is a valid concern. As a short-term mitigation, we can modify the condition: It won't cover the edge case Min = Max = 0, but it seems to be rare. Do you think that it is worth applying it in your PR, @spikecurtis? Nevertheless, I agree that the proper fix requires changes in the persistence layer (nullable column). |
I agree that Min = Max = 0 isn't an important case, as the parameter then is just a fixed value. However, In practice there is almost always a min or max, e.g. max int32 or whatever, but it's kind of obnoxious to force template authors to provide both. I think we should just do the proper fix in a separate PR and not absorb it into the one I'm currently working on. |
👍 Fortunately, the fix should not have implications on the API compatibility ( ValidationMin int32 `json:"validation_min,omitempty"`
ValidationMax int32 `json:"validation_max,omitempty"` |
FYI: I'm working on fixing this issue.
The problem has its roots in the terraform-provider-coder, which assumes that both properties ( |
Resolved in coder/terraform-provider-coder#122 |
I'm afraid that I found another issue. When terraform is resolving the plan it ends up with zero-populated values, which is odd as Fragment of the .tfplan file: {
"address": "data.coder_parameter.compute_instances_7",
"mode": "data",
"type": "coder_parameter",
"name": "compute_instances_7",
"provider_name": "registry.terraform.io/coder/coder",
"schema_version": 0,
"values": {
"default": "3",
"description": "Let's set the expected number of instances.",
"display_name": null,
"icon": "/icon/rubymine.svg",
"id": "a7564661-442f-4ee6-a3d0-49fcdda08839",
"legacy_variable": null,
"legacy_variable_name": null,
"mutable": true,
"name": "Compute instances",
"option": null,
"optional": true,
"type": "number",
"validation": [
{
"error": "",
"max": 8,
"min": 0,
"monotonic": "increasing",
"regex": ""
}
],
"value": "3"
},
"sensitive_values": {
"validation": [
{}
]
}
} I'm investigating it. It doesn't make sense as BTW Is that what you meant @mafredri? |
If you set
validation.min
orvalidation.max
to 0 in a rich parameter, we skip validation.This makes it impossible to correctly enforce a parameter range where 0 is the min or max, e.g. 0-9.
Spotted during a code read:
coder/codersdk/richparameters.go
Line 112 in e951778
Unfortunately this needs to be resolved at the protobuf / data persistence layer, since we only store the min and max, and not whether either or both were explicitly set in the terraform.
cc @mtojek
The text was updated successfully, but these errors were encountered: