-
Notifications
You must be signed in to change notification settings - Fork 14
(feature): Add nullable property validation
#35
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: stas17 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 @stas17! |
Signed-off-by: Stas Smirnov <[email protected]>
5553b03 to
62d22ef
Compare
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.
Thanks @stas17 for taking this work on and creating this PR!
Overall this looks pretty good to me - just a couple comments.
| Validates compatibility of changes to a property nullable constraint. While most changes to the | ||
| nullable constraint of a property are _generally_ safe, it is important to note that changing | ||
| the semantics of a field _is_ a breaking change as it breaks expectations clients/users | ||
| have made about what configuring the property does. |
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.
This description makes it sound like there are a lot of potential changes that can be made to the nullable constraint of a property, where a majority of those are likely non-breaking, which doesn't seem to be true here.
As far as I know, a property is either nullable or it is not nullable. The change from non-nullable -> nullable is probably OK as it is less restrictive (maybe @JoelSpeed has some thoughts here?), but nullable -> non-nullable is a more restrictive change and is breaking (any clients that expect to be able to set the value of the property to null no longer can).
| // When set to Allow, changing from not-nullable to nullable will not be flagged. | ||
| // When set to Disallow, changing from not-nullable to nullable will be flagged. | ||
| // Defaults to Disallow. | ||
| ToNullablePolicy ToNullablePolicy `json:"toNullablePolicy,omitempty"` |
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.
What do you think of making this something like nullableChangePolicy with the only allowed values being None and AllowChangeToNullable?
I think this might get us a bit more descriptive options than Allow and Disallow that can be used here.
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
Issue: #28
What this PR does
This PR adds validation for tracking changes to nullable fields in CRDs. The validation checks for two scenarios:
nullable: trueproperty)nullable: trueproperty or setting it tofalse)Rationale
Similar to type changes, modifying the nullable property of a field is a potentially breaking change for existing users of the CRD. This change can affect validating webhooks, controllers, and other components that depend on the CRD schema.
Changes
nullablevalidation inpkg/validations/property/nullable.gopkg/validations/property/nullable_test.gotest/nullablefieldadded- verifies transition from non-nullable to nullable fieldtest/nullablefieldremoved- verifies transition from nullable to non-nullable fieldConfiguration
The validation supports a
toNullablePolicy:Disallow(default) - any change to nullable will be flagged as an errorAllow- permits making fields nullableExample configuration: