-
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
feature: Allow validation comment tags to be merged through aliases #449
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexzielenski 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 |
/cc @Jefftree |
87712fc
to
bc192d4
Compare
I think it more straightforward to convert the commentTags into a spec.Schema and generate from that. Especially as we begin to add more "virtual" markers to the struct rather than direct spec.SchemaProps
Allows more advanced inheritences to be specified for when base Go struct types are shared. A transparent alias can be used to easily apply extra validations
bc192d4
to
3ddbc3f
Compare
This has the same idea as why we have a separate "description" field that we override for referenced types. I like the merging order, I'm trying to understand the schema implications a bit more. How does the OpenAPI look like for a type alias field? Does the validation field for an upstream type T propagate down to the alias? I understand that's the end behavior but what does the schema for that look like? |
Putting validation on an alias type helps a lot here. Given how hard it is to expose the validation data in OpenAPI, I'd be okay with "server side only" declarative validation for these cases, at least until we have more time to look into ways to either merge or inject validation into shared types in OpenAPI while still keeping the changes non-disruptive to OpenAPI generated clients. EDIT: I could imagine a couple possible ways to implement "server side only" declarative validation. One would be to use the aliases like proposed in this PR, but don't change OpenAPI. Another might be to have a declarative way to call out to hand written validation as a stop-gap that would allow us to make incremental forward progress on a migration. |
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/test-infra 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 |
The Kubernetes project currently lacks enough active 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 rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
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. |
We have a number of shared Go types used in k8s API. It would be useful if we could use aliases to override validation of a base type without creating a new type (so it is transparent to Go).
One example is NodeSelectorTerm:
https://github.com/kubernetes/kubernetes/blob/2ce04fc04bf2cbbbacf2f184fd9ebd4e99d65430/pkg/apis/core/types.go#L2830-L2847
There are separate validations for
MatchExpressions
vsMatchField
both with typeNodeSelectorRequirement
despite the type being the same.https://github.com/kubernetes/kubernetes/blob/7972f0309ce8bad3292f3291718361367b2e58fe/pkg/apis/core/validation/validation.go#L4384-L4396
It wouldn't be good to write complex CEL
.all
quantification expressions due to the fact that the error messages wouldnt be reported in the correct locations.This PR makes it so we can define separate types overlaying a common type through aliases, and our validation comment tags still function. The tags specified closest to the the field take precedence, and we follow the type and aliases thereafter.