Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bump to CEL v0.10.0 and update tests and usage #108576

Merged
merged 2 commits into from
Mar 8, 2022
Merged

Bump to CEL v0.10.0 and update tests and usage #108576

merged 2 commits into from
Mar 8, 2022

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Mar 8, 2022 •
edited

CEL v0.10.0 contains quite a few CEL changes we care about:

This PR specifically includes:

  1. the bump:
hack/pin-dependency.sh  github.com/google/cel-go v0.10.0

  1. A bunch of tests around numeric comparisons (to show how the support works. Note that we cannot compare ints to floats w/o a cast), and int-or-string, which is much more flexible now (although we don't have a union type yet).

  2. A change to list/map literals inline in CEL expressions restricting them to only contain elements/keys/values of the same type (i.e. `[1, false, 3.0, "a"] is not allowed), this a undesirable from a type checking perspective so we're just not allowing it.

/sig api-machinery
/cc @cici37 @liggitt @DangerOnTheRanger @TristonianJones

NONE

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/dependency Issues or PRs related to dependency changes labels Mar 8, 2022
@jpbetz
Copy link
Contributor Author

jpbetz commented Mar 8, 2022

/triage accepted
/priority important-soon

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 8, 2022
liggitt
liggitt reviewed Mar 8, 2022
View reviewed changes
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A restriction to lists literals inline in CEL expressions restricting them to only contain elements of the same type (i.e. `[1, false, 3.0, "a"] is not allowed).

any tests needed here for that? does that just apply to literal values or also to literal lists formed from variable values ([self.foo, self.bar, self.baz])?

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go Show resolved Hide resolved
staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go Show resolved Hide resolved
@jpbetz
Copy link
Contributor Author

jpbetz commented Mar 8, 2022

Thanks @liggitt! Feedback applied.

@liggitt
Copy link
Member

liggitt commented Mar 8, 2022

lgtm, go ahead and squash in the review commit

@jpbetz
Copy link
Contributor Author

jpbetz commented Mar 8, 2022

squashed

@liggitt
Copy link
Member

liggitt commented Mar 8, 2022

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2022
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2022
@jpbetz
Copy link
Contributor Author

jpbetz commented Mar 8, 2022

/retest

@pacoxu
Copy link
Member

pacoxu commented Mar 8, 2022

/kind feature
Does this need a release note?

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 8, 2022
@k8s-ci-robot k8s-ci-robot merged commit c964ef8 into kubernetes:master Mar 8, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Mar 8, 2022
This was referenced Mar 22, 2022
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@cici37 cici37 Awaiting requested review from cici37

@DangerOnTheRanger DangerOnTheRanger Awaiting requested review from DangerOnTheRanger

Assignees

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dependency Issues or PRs related to dependency changes cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.24
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jpbetz @liggitt @k8s-ci-robot @pacoxu