validations: add 'allOf' property validation - #61
saschagrunert wants to merge 1 commit into
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: saschagrunert The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
a7ed633 to
692b916
Compare
|
Rebased |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Apologies for the long delay on the review here - I haven't been able to carve out much time to review PRs here for some time.
Overall this looks pretty good. Just one change I'd like to see made to what is considered compatible changes.
692b916 to
f5143a2
Compare
k8s-ci-robot
added
size/XL
81b274f to
e38f529
Compare
| The `allOf` validation can be configured to allow removing subschemas as a compatible change when you are confident that relaxing validation will not affect API consumers: | ||
|
|
||
| - `removalPolicy` - controls whether removing allOf subschemas is considered compatible. Allowed values are `Allow` and `Disallow`. When set to `Allow`, the validation does not flag removals. The default is `Disallow` to ensure such changes are reviewed. |
There was a problem hiding this comment.
I must've missed this previously, be we want both addition and removal policy configuration knobs so end-users can distinguish between reader/writer semantics for their APIs based on the actions they care about for their API.
For example, if a user only cares about the readers of their API, removals are likely tolerable but additions are not. If they only care about the writers of their API, removals are not tolerable, but additions likely are. If they care about both, neither are tolerable.
There was a problem hiding this comment.
Thanks for the feedback! Added an additionPolicy configuration knob alongside removalPolicy, both defaulting to Disallow. This lets users tailor validations to their reader/writer semantics independently.
| func (ao *AllOf) compareExisting(old, updated []apiextensionsv1.JSONSchemaProps) error { | ||
| added := countNotFound(updated, old) | ||
| if added > 0 { | ||
| return fmt.Errorf("%w : %d subschema(s)", ErrAllOfConstraintAdded, added) | ||
| } | ||
|
|
||
| return ao.checkRemoved(countNotFound(old, updated), ErrAllOfConstraintRemoved) | ||
| } | ||
|
|
||
| func (ao *AllOf) checkRemoved(count int, sentinel error) error { | ||
| if count > 0 && ao.RemovalPolicy != AllOfRemovalPolicyAllow { | ||
| return fmt.Errorf("%w : %d subschema(s)", sentinel, count) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func countNotFound(check, against []apiextensionsv1.JSONSchemaProps) int { | ||
| count := 0 | ||
|
|
||
| for i := range check { | ||
| found := false | ||
|
|
||
| for j := range against { | ||
| if equality.Semantic.DeepEqual(&check[i], &against[j]) { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !found { | ||
| count++ | ||
| } | ||
| } | ||
|
|
||
| return count | ||
| } |
There was a problem hiding this comment.
In a more recent PR similar to this one, I saw the use of a string form of the apiextensionsv1.JSONSchemaProps object pushed into a sets.Set object for both the old and new properties. From there they used the Difference() method on the sets to identify any missing values from either set to determine adds/removals.
I suspect a similar behavior could be implemented here.
There was a problem hiding this comment.
Refactored to use sets.Set[string] with Difference() for schema diffing, matching the approach in #64. Also added normalizeAllOfSchema to strip non-structural fields before comparison.
aa4f8d0 to
8e382c6
Compare
k8s-ci-robot
added
the
needs-rebase
Add additionPolicy and removalPolicy config knobs for reader/writer semantics. Use sets.Set for schema diffing instead of manual loops. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
8e382c6 to
f1fd821
Compare
k8s-ci-robot
removed
the
needs-rebase
3 participants
What this PR does:
Adds a property validation for the
allOfJSON Schema keyword. It detects incompatible changes toallOfconstraints on CRD properties:allOfconstraint when there was none previouslyallOfconstraintallOfconstraintBoth additions and removals are treated as incompatible by default. An
additionPolicyandremovalPolicyconfiguration knob allow users to opt into treating additions or removals as compatible when appropriate for their reader/writer considerations.Uses
sets.Setwith stringified schemas for diffing, matching the approach in #64.Which issue this PR is related to:
Fixes #24