- Notifications
You must be signed in to change notification settings - Fork925
chore: improve dynamic parameter validation errors#18501
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
Conversation
Emyrk commentedJun 23, 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. |
0e3dc00 toa51570aCompare0923f4d to817afe8Compare817afe8 to21556dbComparec1d05a0 to129235bCompare21556db to5f30d20Compare129235b to6a5ee9eCompare5f30d20 to40063cdCompare0523a2f tof4e5548Compare686b2f7 to6849a80Comparef4e5548 to0523a2fCompare6849a80 toa5f35ddComparea5f35dd to3c2904cCompareUh oh!
There was an error while loading.Please reload this page.
| Diagnostics hcl.Diagnostics | ||
| Parametermap[string]hcl.Diagnostics |
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.
suggestion: unexport and return a clone via method?
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 one should really mutate the diagnostics. But I can do that 👍
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 am going to leave it for now. If I make a method to export it, I would either need 1 method to return both diag sets. Or 2 methods. I think the usage is less intuitive in those cases.
func (e*ResolveError)Diagnostics()(hcl.Diagnostics,map[string]hcl.Diagnostics)// Or// This name is not ideal, since Diagnostics does not include the parameter ones :thinking:func (e*ResolveError)Diagnostics()(hcl.Diagnostics)func (e*ResolveError)ParameterDiagnostics()(map[string]hcl.Diagnostics)
coderd/httpapi/httperror/wsbuild.go Outdated
| Validations:nil, | ||
| } | ||
| forname,diag:=rangeparameterErr.Parameter { |
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.
suggestion: sort map keys for deterministic output?
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.
Good idea 👍
5ed0c7a 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
The current
BuildErrorresponse fromwsbuilderdoes not support rich errors from validation. Changed this to use theValidationsblock of codersdk responses to return all errors for invalid parameters.Example http response
{"message":"Unable to validate parameters","validations": [ {"field":"big_number","detail":"Invalid parameter value according to 'validation' block; Number must be between 500 and 1000" }, {"field":"colors","detail":"Values must be a valid option; Value\"[\\\"potatoe\\\"]\" is not a valid option, values\"potatoe\" are missing from the options" }, {"field":"intro","detail":"Invalid parameter value according to 'validation' block; All messages must start with 'Hello' (value\"Goodbye!\" does not match\"^Hello\")" } ]}Cli error