-
Notifications
You must be signed in to change notification settings - Fork 13
validations: add pattern field validation
#44
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LoginovIlia The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @LoginovIlia! |
|
/cc @jpbetz |
|
Just a heads up that this is on my list to take a look at soon. If I don't get to it today, it will be on my list for first thing next week. Thanks for contributing! |
everettraven
left a 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.
Overall, this looks really good - thanks for taking the time to contribute!
I'm requesting changes here because of some recent conversations I've had with folks that brought to my attention the importance of taking into account how loosening validations may impact both readers and writers of an API.
Because crdify doesn't have nuanced knowledge of a situation, we should add a knob here so that users of crdify that do not want to allow removal of a pattern constraint can do so and we should default to that behavior (we always default to the most restrictive at the moment).
| Validates compatibility of changes to a property's pattern regular expression. Adding a pattern | ||
| that did not previously exist or modifying the expression can tighten validation, so these changes | ||
| are flagged for review. Removing a pattern is accepted because it broadens what values are allowed. |
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.
We may want to allow users to restrict removal of a pattern as well (and maybe we default to restricting this too).
Removing a pattern is a compatible change for writers, but is likely a breaking change to readers.
Older clients may no longer be able to handle the values here if they were built on the assumption that valid values for that field always adhere to that pattern.
What this PR does
Adds validation for the pattern field when a pattern is added or changed. Removing a pattern is still allowed
Rationale
Changing or adding a regex may block values that worked before, so we mark these changes. Removing a regex makes the input more open, so we do not mark it
Changes
pkg/validations/property/pattern.gowith unit testsdocs/validations.mdtest/patternaddedandtest/patternchanged