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.

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

Support JSON double values being reencoded in the CEL interceptor #470

Merged
merged 1 commit into from
Mar 6, 2020

Conversation

bigkevmcd
Copy link
Member

@bigkevmcd bigkevmcd commented Mar 5, 2020

Changes

When the JSON value 2 is decoded, it is a Double, so arithmetic on it returns doubles.

This ensures that the double value is correctly reencoded for JSON.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Release Notes

Fix reencoding of numeric values coming from JSON bodies.

@tekton-robot tekton-robot requested review from dibyom and dlorenc March 5, 2020 18:30
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 5, 2020
@bigkevmcd bigkevmcd force-pushed the fix-cel-increments branch from d7a37ea to f3a2bf7 Compare March 5, 2020 18:34
@bigkevmcd
Copy link
Member Author

I had a look at using a json.Decoder with UseNumber() and the resulting json.Number values are not supported by CEL (at least for now).

@bigkevmcd bigkevmcd force-pushed the fix-cel-increments branch from f3a2bf7 to 03a2c9a Compare March 5, 2020 19:37
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

nice, thank you!

Copy link
Contributor

@khrm khrm left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2020
@bigkevmcd bigkevmcd force-pushed the fix-cel-increments branch from 03a2c9a to 91892d5 Compare March 6, 2020 07:18
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 6, 2020
@bigkevmcd bigkevmcd force-pushed the fix-cel-increments branch from 91892d5 to bd4c958 Compare March 6, 2020 07:56
@bigkevmcd bigkevmcd changed the title Support JSON double values being reencoded. Support JSON double values being reencoded in the CEL interceptor Mar 6, 2020
@bigkevmcd bigkevmcd force-pushed the fix-cel-increments branch from bd4c958 to 708ffe2 Compare March 6, 2020 08:04
Copy link
Contributor

@khrm khrm left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2020
When the JSON value 2 is decoded, it is a Double, so arithmetic on it returns doubles.

It can be cast into an integer value which make it slightly easier to
read.

With further illustrative tests and documentation.
@bigkevmcd bigkevmcd force-pushed the fix-cel-increments branch from 708ffe2 to 30a5eb1 Compare March 6, 2020 08:08
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2020
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2020
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, khrm, vdemeester

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2020
@tekton-robot tekton-robot merged commit 8103c05 into tektoncd:master Mar 6, 2020
@bigkevmcd bigkevmcd deleted the fix-cel-increments branch March 10, 2020 10:27
nikhil-thomas added a commit to nikhil-thomas/triggers that referenced this pull request Jul 5, 2021
Add a check for an existing sync PR in the midstream repo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants