-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Optimize away repeated structural schema and CEL DeclType construction in CRD validation #110135
Optimize away repeated structural schema and CEL DeclType construction in CRD validation #110135
Conversation
8fecfcf
to
6c7560e
Compare
@liggitt would you be willing to review? This is one of the optimizations we had listed to do before taking CEL CRD validation to beta (@DangerOnTheRanger will be working on the other one-- eliminating repeated calls to calculate the min size of a schema). |
/sig api-machinery |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
f49b78c
to
a5ae4cb
Compare
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
Outdated
Show resolved
Hide resolved
a5ae4cb
to
8bf80d8
Compare
/retest |
1 similar comment
/retest |
staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation.go
Outdated
Show resolved
Hide resolved
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 didn't review the tests yet but wanted to do a sweep in the impl
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
Outdated
Show resolved
Hide resolved
09dcc39
to
fed572b
Compare
/retest |
1 similar comment
/retest |
660d441
to
799baef
Compare
staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
673a7e6
to
222e36a
Compare
/retest |
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/cel_validation.go
Show resolved
Hide resolved
222e36a
to
744083a
Compare
staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go
Outdated
Show resolved
Hide resolved
#110135 (comment) is the only outstanding comment I have. Go ahead and squash once that's done, then lgtm Thanks for all the work here, the expanded test coverage looks good |
744083a
to
81bd325
Compare
Fixed, squashed and rebased. Thanks for the review! |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpbetz, liggitt 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This optimizes away repeated structural schema and CEL "decls" construction, which was flagged as a potential blocker for beta for CRD Validation using CEL (kubernetes/enhancements#3266).
Which issue(s) this PR fixes:
Fixes: #106438
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Please use the following format for linking documentation: