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

chore: update ce sdk version and codegen #3964

Merged
merged 2 commits into from
Jul 23, 2024
Merged

chore: update ce sdk version and codegen #3964

merged 2 commits into from
Jul 23, 2024

Conversation

Cali0707
Copy link
Member

@Cali0707 Cali0707 commented Jul 3, 2024

Updates the cloudevents sdk to support CESQL v1

Proposed Changes

  • Update the cloudevents sdk
  • Update codegen (this led to an unrelated format change)

Release Note

The CESQL filter dialect now supports the CESQL v1 specification (https://github.com/cloudevents/spec/releases/tag/cesql%2Fv1.0.0)

@Cali0707
chore: update ce sdk version and codegen
3b9e8b9 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/data-plane labels Jul 3, 2024
@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2024
@Cali0707
Copy link
Member Author

Cali0707 commented Jul 3, 2024

/cc @pierDipi @matzew @creydr

/hold until we have an equivalent PR to update the go SDK in eventing core

@knative-prow knative-prow bot requested review from creydr, matzew and pierDipi July 3, 2024 20:43
@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2024
@codecov Codecov
Copy link

codecov bot commented Jul 3, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.24%. Comparing base (d0df87a) to head (25f9fba).
Report is 22 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #3964   +/-   ##
=======================================
  Coverage   48.24%   48.24%           
=======================================
  Files         246      246           
  Lines       14500    14500           
=======================================
  Hits         6995     6995           
  Misses       6799     6799           
  Partials      706      706

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

matzew
matzew reviewed Jul 4, 2024
View reviewed changes
...a/dev/knative/eventing/kafka/broker/dispatcher/impl/filter/subscriptionsapi/CeSqlFilter.java Outdated
import io.cloudevents.sql.Expression;
import io.cloudevents.sql.Parser;
import io.cloudevents.sql.Type;
import io.cloudevents.sql.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

no no. no wildcards :-)

matzew
matzew reviewed Jul 4, 2024
View reviewed changes
...v/knative/eventing/kafka/broker/dispatcher/impl/filter/subscriptionsapi/CeSqlFilterTest.java
@@ -51,7 +51,7 @@ static Stream<Arguments> testCases() {
Arguments.of(event, "'TRUE'", true),
Arguments.of(event, "'FALSE'", false),
Arguments.of(event, "0", false),
Arguments.of(event, "1", false),
Arguments.of(event, "1", true),
Copy link
Contributor

Choose a reason for hiding this comment

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

wat?

Copy link
Contributor

Choose a reason for hiding this comment

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

why was that false before? 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh before integer to boolean casting was a cast error, which turns in a filter result of false. In v1, we defined integer to boolean casting so now it works :)

@Cali0707
cleanup: no longer using wildcard import
25f9fba 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707 Cali0707 requested a review from matzew July 4, 2024 13:15
@Cali0707
Copy link
Member Author

Cali0707 commented Jul 4, 2024

/test upgrade-tests

1 similar comment
@matzew
Copy link
Contributor

matzew commented Jul 16, 2024

/test upgrade-tests

matzew
matzew approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@matzew matzew left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2024
@knative-prow Knative Prow
Copy link

knative-prow bot commented Jul 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, matzew

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

@Cali0707
Copy link
Member Author

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2024
@Cali0707
Copy link
Member Author

/retest-required

1 similar comment
@matzew
Copy link
Contributor

matzew commented Jul 17, 2024

/retest-required

@matzew
Copy link
Contributor

matzew commented Jul 17, 2024

/retest

@Cali0707
Copy link
Member Author

/retest-required

@matzew
Copy link
Contributor

matzew commented Jul 18, 2024

/retest

1 similar comment
@matzew
Copy link
Contributor

matzew commented Jul 18, 2024

/retest

@matzew
Copy link
Contributor

matzew commented Jul 18, 2024

/retest

@Cali0707 Cali0707 mentioned this pull request Jul 18, 2024
Merged
@matzew
Copy link
Contributor

matzew commented Jul 23, 2024

/test upgrade-tests

@matzew
Copy link
Contributor

matzew commented Jul 23, 2024

/retest

@knative-prow knative-prow bot merged commit 63338be into knative-extensions:main Jul 23, 2024
33 checks passed
pierDipi
pierDipi reviewed Jul 23, 2024
View reviewed changes
data-plane/THIRD-PARTY.txt
Comment on lines +35 to 36
(Apache-2.0) Apache Commons Codec (commons-codec:commons-codec:1.16.1 - https://commons.apache.org/proper/commons-codec/)
(Apache-2.0) Apache Commons Codec (commons-codec:commons-codec:1.17.0 - https://commons.apache.org/proper/commons-codec/)
Copy link
Member

@pierDipi pierDipi Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

same library 2 different versions? 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh not sure how that happened. Any ideas?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierDipi pierDipi pierDipi left review comments

@matzew matzew matzew approved these changes

@aliok aliok Awaiting requested review from aliok

@lionelvillard lionelvillard Awaiting requested review from lionelvillard

@creydr creydr Awaiting requested review from creydr

Assignees

@matzew matzew

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/data-plane lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Cali0707 @matzew @pierDipi