Add json.marshal and json.unmarshal cel functions#1033
Add json.marshal and json.unmarshal cel functions#1033k8s-ci-robot merged 2 commits intokubernetes-sigs:mainfrom
Conversation
|
Welcome @NicholasBlaskey! |
|
Hi @NicholasBlaskey. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
c64d39a to
f20a252
Compare
|
/ok-to-test |
a-hilaly
left a comment
There was a problem hiding this comment.
Thank you Nick - i left one nit comment below
There was a problem hiding this comment.
I agree that the cel package deserves a better organization. Can we make this change in a seperate PR?
There was a problem hiding this comment.
yes please, this should be focused to the functions only.
There was a problem hiding this comment.
Yep can do
I moved these functions into a new package to resolve a circular import error I was getting
|
/retest |
a-hilaly
left a comment
There was a problem hiding this comment.
Can you please update the CEL library docs? https://kro.run/docs/concepts/rgd/cel-expressions#available-cel-libraries
2e486d5 to
d4e98f5
Compare
updated |
a-hilaly
left a comment
There was a problem hiding this comment.
Thank you @NicholasBlaskey !
/approve
|
@jakobmoellerdev @matthchr shall we pin this for 0.9.0? or are we good with a 0.8.6? personally i'm fine with either |
There was a problem hiding this comment.
yes please, this should be focused to the functions only.
|
|
||
| func (l *jsonLibrary) CompileOptions() []cel.EnvOption { | ||
| return []cel.EnvOption{ | ||
| cel.Function("json.unmarshal", |
There was a problem hiding this comment.
this should be versioned internally to v1alpha1
There was a problem hiding this comment.
Could you clarify what you mean for versioned internally here?
I see some other libraries have integer versioning like this but not seeing any use a string version like v1alpha1. The random library also doesn't seem to have any versioning currently.
https://github.com/google/cel-go/blob/v0.27.0/ext/lists.go#L213
There was a problem hiding this comment.
mabye take a look at https://github.com/google/cel-go/blob/v0.27.0/ext/lists.go#L175-L189 or https://github.com/google/cel-go/blob/v0.27.0/ext/sets.go#L88-L97. Its basically about introducing a version so we can control it later. i dont really mind if its v1alpha1 or an integer tbh.
There was a problem hiding this comment.
added support for setting a version
currently the version integer is unused but can be referenced for future updates to the library
| } | ||
|
|
||
| func unmarshalJSON(jsonString ref.Val) ref.Val { | ||
| if jsonString.Type() != types.StringType { |
There was a problem hiding this comment.
I think you could use the native default type adapter here to get a string.
|
I would favor a pin but won't block on 0.8.6 |
d4e98f5 to
6de5410
Compare
jakobmoellerdev
left a comment
There was a problem hiding this comment.
/lgtm
/hold
holding until 0.9.0 or if we have another approve ack this for next patch
|
On another note: https://github.com/google/cel-go/pull/1261/changes introduced a JSON Value compatibility. Do you think this is relevant here? |
doesn't seem relevant for For As far as I can tell they should be equivalent but don't have a strong opinion either way, open to changing it after #1047 merges |
|
@NicholasBlaskey can you please rebase this one? |
cde882c to
91eebbe
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, NicholasBlaskey The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
Add a
jsoncel library to allow easier integration from / to json in RGDsinitially has
json.marshaljson.unmarshalTested with example.
Also tested error case of
which causes a reconciliation failure