- Notifications
You must be signed in to change notification settings - Fork928
feat: allow number options with monotonic validation#12726
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
Signed-off-by: Danny Kopping <danny@coder.com>
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.
Initially we planned to extend validation in both the browser & CLI UIs
I don't remember now if CLI supports validation of monotonicity. As long as it usescodersdk.ValidateWorkspaceBuildParameters
, it should be supported. Maybe drop another CLI test to verify the behavior?
, but we agreed to defer this until#7099 is implemented which would centralise all the logic.
Since validation might work well on the CLI, I'm wondering if it isn't a low hanging fruit to come up with a temporary fix for the frontend. I'm unsure how frontend deals with monotonicity right now.
Uh oh!
There was an error while loading.Please reload this page.
dannykopping commentedMar 22, 2024 • 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.
The CLI doesn't seem to have any client-side validation for options that I can see. The
Frontend validation can work but it's not possible to display the error inline with the |
Signed-off-by: Danny Kopping <danny@coder.com>
AFAIR There were some problems with testing Anyway, if CLI and UI depend on backend validation here, then user will be warned about value conflict. If you want to try, you can modify |
Will do, thanks! |
Signed-off-by: Danny Kopping <danny@coder.com>
@mtojek I added a test inaae5463, PTAL? I also updated the provider now thatcoder/terraform-provider-coder#202 is merged. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
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.
One nit pick to clarify (restart or update) but otherwise it is 👍 .
Uh oh!
There was an error while loading.Please reload this page.
Signed-off-by: Danny Kopping <danny@coder.com>
Uh oh!
There was an error while loading.Please reload this page.
Fixes#11579
NOTE:
terraform-provider-coder
was updated to facilitate this change, and your template will requirev0.19.0 for this feature to work. You can runterraform init -upgrade
in your template directory. If you have a version constraint set, ensure it points to this version.This PR just does some light refactoring / cleanup; the real work was done incoder/terraform-provider-coder#202.
Initially we planned to extend validation in both the browser & CLI UIs, but we agreed to defer this until#7099 is implemented which would centralise all the logic. Right now the UIs don't validate the monotonicity before submit, but there is still backend validation of course.
Here's an example of how to use the new functionality:
Examples: