Rename new trigger filters sql field to cesql #6148

devguyio · 2022-02-15T12:14:44Z

Proposed Changes

The original implementation of the SubscriptionsAPI Trigger filters #5715 used the dialect name sql . The feedback from users was that this is a confusing name and the cesql name is more in alignment with the Cloud Events SQL Expression Language spec.

🧹 Rename new trigger filters sql field to cesql

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

- 🧹  The `sql` field of [the new trigger filters](https://knative.dev/development/eventing/experimental-features/new-trigger-filters/) experimental feature is now called `cesql`.

Docs

knative/docs#4737

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

knative-prow-robot · 2022-02-15T12:14:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devguyio

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [devguyio]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

devguyio · 2022-02-15T12:15:35Z

Going to update the docs
/hold

matzew · 2022-02-15T12:18:09Z

pkg/apis/eventing/v1/trigger_types.go

+	// CESQL is a CloudEvents CESQL expression that will be evaluated to true or false against each CloudEvent.
 	//
 	// +optional
-	SQL string `json:"sql,omitempty"`


I like this proposal, and since we touch an alpha stage experimental feature, this seems right to do.

Makes it more explicit, rather than just a generic sql field on the API 👍

codecov · 2022-02-15T12:21:20Z

Codecov Report

Merging #6148 (f022bec) into main (6b9499f) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main    #6148      +/-   ##
==========================================
- Coverage   82.09%   82.06%   -0.03%     
==========================================
  Files         231      231              
  Lines        7765     7769       +4     
==========================================
+ Hits         6375     6376       +1     
- Misses        940      942       +2     
- Partials      450      451       +1

Impacted Files	Coverage Δ
pkg/apis/eventing/v1/trigger_types.go	`100.00% <ø> (ø)`
pkg/broker/filter/filter_handler.go	`72.44% <33.33%> (ø)`
pkg/apis/eventing/v1/trigger_validation.go	`84.66% <100.00%> (-1.64%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b9499f...f022bec. Read the comment docs.

devguyio · 2022-02-15T12:27:45Z

Release note and docs added
/unhold

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

pierDipi

LGTM

pkg/apis/eventing/v1/trigger_types.go

n3wscott · 2022-02-15T16:02:49Z

pkg/apis/eventing/v1/trigger_types.go

 	//
 	// +optional
-	SQL string `json:"sql,omitempty"`
+	CESQL string `json:"cesql,omitempty"`


This feels like a really confusing rename. Why not change Suffix to CESuffix and Exact to CEExact? Because those are all worse names and this filter is in the context of cloudevents filters. This is a docs problem, not an API naming problem.

The feedback we've had so far is related to the strong database related connotation that comes with sql. What I read from the feedback is that calling it cesql makes it instantly obvious that this is a custom expression language related to CloudEvents.

I agree with @devguyio - renaming to CESQL will help to avoid lot of user confusion and may help with google search in future ...

I also think it's beneficial to change the name

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

devguyio · 2022-02-15T17:30:50Z

/retest

matzew · 2022-02-16T05:25:49Z

/lgtm
/hold

feel free to unhold ...

odacremolbap

nit: this comment might need updating

eventing/test/experimental/features/new_trigger_filters/filters.go

Line 37 in d8692ee

    
           // The feature set tests four filter dialects: exact, prefix, suffix and sql (aka CloudEvents SQL).

pkg/apis/eventing/v1/trigger_validation.go

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

devguyio · 2022-02-16T10:21:47Z

/retest-required

odacremolbap · 2022-02-16T16:54:59Z

/lgtm

matzew · 2022-02-17T05:47:20Z

/unhold

matzew · 2022-02-17T05:48:27Z

/retest

devguyio · 2022-02-17T08:08:19Z

/retest

devguyio · 2022-02-17T10:39:11Z

It still looks like a flake to me, can't see any correlation to the changes introduced
/retest

devguyio · 2022-02-17T15:16:05Z

/retest

matzew · 2022-02-22T09:46:51Z

/retest

knative-prow-robot · 2022-02-22T10:42:32Z

@devguyio: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-knative-eventing-reconciler-tests	`f022bec`	link	true	`/test pull-knative-eventing-reconciler-tests`

Full PR test history. Your PR dashboard.

Details

Instructions 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/test-infra repository. I understand the commands that are listed here.

Rename new trigger filters SQL field to CESQL

487f688

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

knative-prow-robot added area/test-and-release Test infrastructure, tests or release size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 15, 2022

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2022

matzew reviewed Feb 15, 2022

View reviewed changes

devguyio mentioned this pull request Feb 15, 2022

Rename sql to cesql knative/docs#4737

Merged

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2022

run hack/update-codegen.sh

ffb29af

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 15, 2022

pierDipi reviewed Feb 15, 2022

View reviewed changes

n3wscott reviewed Feb 15, 2022

View reviewed changes

pkg/apis/eventing/v1/trigger_types.go Outdated Show resolved Hide resolved

n3wscott reviewed Feb 15, 2022

View reviewed changes

fix typo in docs

d8692ee

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2022

knative-prow-robot assigned matzew Feb 16, 2022

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2022

odacremolbap reviewed Feb 16, 2022

View reviewed changes

pkg/apis/eventing/v1/trigger_validation.go Outdated Show resolved Hide resolved

fix validation cesql viaField

9119e20

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2022

fix comment

208e9fe

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 16, 2022

fix trigger validation tests

f022bec

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

knative-prow-robot assigned odacremolbap Feb 16, 2022

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2022

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 17, 2022

knative-prow-robot merged commit 74cebc5 into knative:main Feb 26, 2022

Rename new trigger filters sql field to cesql #6148

Rename new trigger filters sql field to cesql #6148

Uh oh!

Conversation

devguyio commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Pre-review Checklist

Uh oh!

knative-prow-robot commented Feb 15, 2022

Uh oh!

devguyio commented Feb 15, 2022

Uh oh!

matzew Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

devguyio commented Feb 15, 2022

Uh oh!

pierDipi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

n3wscott Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

devguyio Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

aslom Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

matzew Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

devguyio commented Feb 15, 2022

Uh oh!

matzew commented Feb 16, 2022

Uh oh!

odacremolbap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devguyio commented Feb 16, 2022

Uh oh!

odacremolbap commented Feb 16, 2022

Uh oh!

matzew commented Feb 17, 2022

Uh oh!

matzew commented Feb 17, 2022

Uh oh!

devguyio commented Feb 17, 2022

Uh oh!

devguyio commented Feb 17, 2022

Uh oh!

devguyio commented Feb 17, 2022

Uh oh!

matzew commented Feb 22, 2022

Uh oh!

knative-prow-robot commented Feb 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

devguyio commented Feb 15, 2022 •

edited

Loading

codecov bot commented Feb 15, 2022 •

edited

Loading