-
Notifications
You must be signed in to change notification settings - Fork 205
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
Allow marker comments to extend validations for subschemas #469
Conversation
bf5bc2e
to
626a771
Compare
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.
I feel like there are quite a few implicit changes here, would you be able to separate them out into another PR/commit to explain the relevance? The changes move the codebase in a good direction but I'm a bit confused about the interdependency between them and the PR description goal.
OTOH
- a set of fields being transformed into ptrs
- deterministic output
- validations such as minItems/etc should only be attached corresponding types
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.
Looks good. A few very minor comments.
4160662
to
db03e74
Compare
I split the deterministic output change into a separate commit (no test changes, since theres only 1 possible element so it is more of a future-proofing change) I also removed the MinProperties/MaxProperties behavior change, so diff is much smaller. ready for another pass |
pattern := "" | ||
if c.Pattern != nil { | ||
pattern = *c.Pattern | ||
} |
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.
question: What's the purpose of changing this set of fields to pointers?
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.
It allows us to disambiguate empty from unset. It doesnt affect anything right now but I thought that while I was copying the fields over it'd be better to have this ability.
/hold @jpbetz While trying to use this I've ran into the problem that CEL expressions inside |
ef73428
to
b7afea5
Compare
b7afea5
to
8d2dde0
Compare
Ive tested this extensively as part of kubernetes/kubernetes#123163 To use CEL within these still requires kubernetes/kubernetes#124381 but I think this is ready to merge /cc @jpbetz |
lgtm from me, I'll leave lgtm with @jpbetz if he wants another pass |
pkg/generators/openapi_test.go
Outdated
// +k8s:validation:maxProperties=10 | ||
// +k8s:validation:minProperties=1 | ||
// +k8s:validation:exclusiveMinimum | ||
// +k8s:validation:exclusiveMaximum |
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.
Sorry if this is obvious, but why are we removing these tags from this test input?
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.
Before this PR enforcement of invalid marker tags on structs was skipped:
So tags like exclusiveMinimum
and exclusiveMaximum
now throw errors for structs. minProperties
and maxProperties
are allowed on structs, so their removal was likely overzealous. I'll add it back
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.
added back
One question about the removed test cases then this looks ready to merge. |
…er comments adds powerful feature to marker comments to be able to add validations attributed to subschemas. Does not support removing validations from subschemas
8d2dde0
to
47d23e8
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexzielenski, jpbetz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This extends current marker syntax to support extending the subschemas of a struct inside
items
,properties
, oradditionalProperties
. This is useful in cases like declarative validation for native types where we have many shared types whose validations change at point-of-use, so it does not make sense to annotate the type. And using an alias is too disruptive.Example:
In the above example we can override subschemas at multiple different levels, with type checking supported.
This yields schema:
The value validations are placed in an allOf inside the object they were attached to and override subproperties. New structure is not allowed to be placed in these tags, only validations. We validate that any named properties are also in the structure of the Go type.