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

feat: Introduce expr filters #1226

Merged
merged 7 commits into from
Jun 10, 2021
Merged

feat: Introduce expr filters #1226

merged 7 commits into from
Jun 10, 2021

Conversation

VaibhavPage
Copy link
Contributor

Checklist:

Stephan van Maris and others added 2 commits May 23, 2021 14:34
@VaibhavPage
chore: Fix make command (#1221)
c622530 
target "all" was replaced by "build" in #1203

Signed-off-by: Stephan van Maris <s.vanmaris@teamitg.com>
@VaibhavPage
feat: added expr filter logic and tests
3d821fd 
Signed-off-by: Vaibhav Page <vaibhav.page@gmail.com>
@VaibhavPage VaibhavPage force-pushed the feat/introduce-expr-filters branch from 19a2063 to 3d821fd Compare May 23, 2021 18:38
@VaibhavPage VaibhavPage marked this pull request as ready for review May 24, 2021 13:12
@VaibhavPage VaibhavPage requested a review from whynowy May 24, 2021 13:13
@VaibhavPage VaibhavPage linked an issue May 24, 2021 that may be closed by this pull request
Introduce expression filters for events #1224
Closed
@whynowy
Copy link
Member

whynowy commented May 24, 2021

Welcome back!

whynowy
whynowy reviewed May 25, 2021
View reviewed changes
Copy link
Member

@whynowy whynowy left a comment

Choose a reason for hiding this comment

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

Do you think this can replace the dataFilter, especially if it does not need fields?

examples/sensors/filter-with-expressions.yaml
filters:
# If event payload passes either one of the following filters, the event is considered a valid event.
exprs:
- expr: a == "b" || c == 10
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to do following without specifying the fields?

  exprs:
    - expr: a  == "b" || c== 10
    - expr: d.e == false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you'd need the fields because they represent the variables in the expression. e.g. govaluate won't understand field d.e

sensors/dependencies/filter.go Outdated
}
switch field.Type {
case v1alpha1.JSONTypeString:
parameters[field.Name] = result.Str
Copy link
Member

@whynowy whynowy May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can you try parameters[field.Name] = result.Value()? I feel like that will work, and then fields is not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that would work

@whynowy
Copy link
Member

whynowy commented May 25, 2021

You also need to clean up some outdated, codegen related binaries in {gopath}/bin/** to fix the codegen check error.

@VaibhavPage
Copy link
Contributor Author

Do you think this can replace the dataFilter, especially if it does not need fields?

Yep, I think we should deprecate dataFilters in a future release

@VaibhavPage
chore: codegen
cee392a 
Signed-off-by: Vaibhav Page <vaibhav.page@gmail.com>
@VaibhavPage VaibhavPage requested a review from whynowy May 25, 2021 11:43
@VaibhavPage
Copy link
Contributor Author

You also need to clean up some outdated, codegen related binaries in {gopath}/bin/** to fix the codegen check error.

What's this step does "Ensure nothing has changed" in the pipeline?

@whynowy
Copy link
Member

whynowy commented May 25, 2021

You also need to clean up some outdated, codegen related binaries in {gopath}/bin/** to fix the codegen check error.

What's this step does "Ensure nothing has changed" in the pipeline?

That uses a fresh env to run make codegen, and check if there's any file changes. Here it fails which means the codegen files on your local env are different from a fresh set up env generated.

@cbuckley01
Copy link
Contributor

Hey guys, thanks for adding this, we are eagerly awaiting to give this a whirl, anything I can do to help it get merged in?

@whynowy whynowy changed the title Feat/introduce expr filters feat: Introduce expr filters Jun 10, 2021
@whynowy whynowy merged commit 2f907b1 into master Jun 10, 2021
@cbuckley01
Copy link
Contributor

Thanks @whynowy !

@whynowy whynowy deleted the feat/introduce-expr-filters branch July 13, 2021 06:40
@Hunter-Thompson Hunter-Thompson mentioned this pull request Aug 11, 2021
Closed
juliev0 pushed a commit to juliev0/argo-events that referenced this pull request Mar 29, 2022
@VaibhavPage @whynowy
feat: Introduce expr filters (argoproj#1226)
dadbd61 
* chore: Fix make command (argoproj#1221)

target "all" was replaced by "build" in argoproj#1203

Signed-off-by: Stephan van Maris <s.vanmaris@teamitg.com>

* feat: added expr filter logic and tests

Signed-off-by: Vaibhav Page <vaibhav.page@gmail.com>

* chore: codegen

Signed-off-by: Vaibhav Page <vaibhav.page@gmail.com>

* chore: codegen

Signed-off-by: Vaibhav Page <vaibhav.page@gmail.com>

Co-authored-by: Stephan van Maris <s.vanmaris@e-village.nl>
Co-authored-by: Derek Wang <whynowy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whynowy whynowy Awaiting requested review from whynowy whynowy is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce expression filters for events
3 participants
@VaibhavPage @whynowy @cbuckley01