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

Introduce Filter CloudEvents Feature #5424

Merged
merged 17 commits into from
Apr 10, 2024

Conversation

SpiritZhou
Copy link
Contributor

@SpiritZhou SpiritZhou commented Jan 23, 2024

Introduce filter CloudEvents feature

Checklist

  • I have verified that my change is according to the deprecations & breaking changes policy
  • Tests have been added
  • Changelog has been updated and is aligned with our changelog requirements
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #

Relates to #3533

@SpiritZhou SpiritZhou requested a review from a team as a code owner January 23, 2024 07:59
@tomkerkhove
Copy link
Member

Mind opening a doc PR?

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Can we add some unit tests as well?

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/eventemitter/eventemitter.go Outdated Show resolved Hide resolved
SpiritZhou and others added 2 commits February 18, 2024 10:08
Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
@tomkerkhove
Copy link
Member

LGTM, but I'd like to see these two improvements though to improve the user experience/usability:

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
@SpiritZhou
Copy link
Contributor Author

LGTM, but I'd like to see these two improvements though to improve the user experience/usability:

Improvements added.

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
@SpiritZhou
Copy link
Contributor Author

SpiritZhou commented Mar 4, 2024

/run-e2e internal
Update: You can check the progress here

@tomkerkhove
Copy link
Member

Would you mind fixing the conflicts please?

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
@SpiritZhou
Copy link
Contributor Author

Fixed.

@tomkerkhove
Copy link
Member

@zroubalik / @JorTurFer Do you mind reviewing? I am not enough of an expert on the codebase but think it's generally OK

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Looking good! Nice job!

apis/eventing/v1alpha1/cloudevent_types.go Show resolved Hide resolved
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Awesome job! Some comments inline

pkg/eventemitter/eventemitter.go Outdated Show resolved Hide resolved
pkg/eventemitter/eventemitter.go Outdated Show resolved Hide resolved
pkg/eventemitter/eventemitter.go Show resolved Hide resolved
pkg/eventemitter/eventfilter.go Outdated Show resolved Hide resolved
SpiritZhou and others added 2 commits March 27, 2024 14:33
Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
apis/eventing/v1alpha1/cloudeventsource_webhook.go Outdated Show resolved Hide resolved
pkg/eventemitter/eventemitter.go Show resolved Hide resolved
pkg/eventemitter/eventfilter.go Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

I've been thinking about this. Does it make sense supporting both at the same time? I mean, shouldn't include and exclude be mutually exclusive? Maybe we should support both, but being mutually exlusive, @tomkerkhove ?

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
@SpiritZhou
Copy link
Contributor Author

It will always let user confuse if include and exclude exist at the same time. It is better to have only one type in the specification.

@tomkerkhove
Copy link
Member

End-users should be able to use both, for example:

eventSubscription: #Optional. Submit included/excluded event types will filter events when emitting events. 
  includedEventTypes: #Optional. Only events in this section will be emitted.
  - keda.scaledobject.failed.v1
  excludedEventTypes: #Optional. Events in this section will not be emitted.       
  - keda.scaledobject.ready.v1

But this should not be allowed and blocked:

eventSubscription: #Optional. Submit included/excluded event types will filter events when emitting events. 
  includedEventTypes: #Optional. Only events in this section will be emitted.
  - keda.scaledobject.ready.v1
  excludedEventTypes: #Optional. Events in this section will not be emitted.       
  - keda.scaledobject.ready.v1

@JorTurFer
Copy link
Member

End-users should be able to use both, for example:

eventSubscription: #Optional. Submit included/excluded event types will filter events when emitting events.
includedEventTypes: #Optional. Only events in this section will be emitted.

  • keda.scaledobject.failed.v1
    excludedEventTypes: #Optional. Events in this section will not be emitted.
  • keda.scaledobject.ready.v1

Yeah, currently they can, but my doubt is if that makes sense. In the moment when you add include keda.scaledobject.failed.v1 you are automatically excluding keda.scaledobject.ready.v1 (because it's not the only included type). I mean, adding include or exclude, you are explicitly discarding the other group and supporting both at once can have more problems checking misconfigurations than the benefit we give to the users.

Maybe we can go adding them as mutually exclusive, we will maintain the flexibility for the users because they can use include or exclude, but we can improve the logic not supporting both at the same time. WDYT?

@tomkerkhove
Copy link
Member

Oh in that way, that makes sense indeed. Let's only allow one or the other indeed

Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
@tomkerkhove
Copy link
Member

tomkerkhove commented Apr 3, 2024

/run-e2e internal
Update: You can check the progress here

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

LGTM!

@JorTurFer
Copy link
Member

JorTurFer commented Apr 4, 2024

/run-e2e internal
Update: You can check the progress here

@JorTurFer
Copy link
Member

JorTurFer commented Apr 7, 2024

/run-e2e internal
Update: You can check the progress here

Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
@JorTurFer
Copy link
Member

/skip-e2e

@JorTurFer JorTurFer enabled auto-merge (squash) April 10, 2024 21:23
@JorTurFer JorTurFer merged commit c20c47e into kedacore:main Apr 10, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants