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

Fix 37422 #64215

Merged
merged 7 commits into from
May 4, 2020
Merged

Fix 37422 #64215

merged 7 commits into from
May 4, 2020

Conversation

streamich
Copy link
Contributor

@streamich streamich commented Apr 22, 2020 •
edited
Loading

Summary

Closes #37422

Dev Docs

A breaking change was introduced to filter expression function. If you used type argument of filter function you now must use filterType instead.

Old code:

filter type={...} | ...

New code:

filter filterType={...} | ...

The type field is used internally by expression interpreter to discriminate between different values it passes between functions. filter function was the only function that exposed this field to users, after this change all expression values will consistently use type to determine a type of expression value.

@streamich streamich requested review from a team as code owners April 22, 2020 17:00
@botelastic botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Apr 22, 2020
@streamich streamich added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. review Team:AppArch Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.8.0 v8.0.0 labels Apr 22, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas (Team:Canvas)

lukeelmers
lukeelmers reviewed Apr 27, 2020
View reviewed changes
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Code LGTM so far! Glad you also took this opportunity to rename the Filter interface to something more specific.

I think we will probably need a release_note:breaking & dev docs writeup on this, unless the Canvas team feels otherwise.

@streamich
Copy link
Contributor Author

@elasticmachine merge upstream

@streamich streamich added release_note:breaking and removed release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. labels Apr 27, 2020
crob611
crob611 approved these changes Apr 29, 2020
View reviewed changes
Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Looks good 👍

So the breaking issue will be if there are any 3rd party developers who are creating filters incorrectly, they will not function as expected?

Agree that there should be some Dev Documentation around this. Not sure where we document breaking changes to our plugin apis though?

@streamich
Copy link
Contributor Author

@elasticmachine merge upstream

@streamich streamich added v7.9.0 and removed v7.8.0 labels May 4, 2020
@clintandrewhall
Copy link
Contributor

Glad to see this fix! We definitely need to add a breaking-change release note, perhaps with a link to the issue, so anyone who has a plugin break understands why.

@streamich streamich requested a review from lukeelmers May 4, 2020 16:31
@streamich
Copy link
Contributor Author

@clintandrewhall ah, glad to see you back!

lukeelmers
lukeelmers approved these changes May 4, 2020
View reviewed changes
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

All good from my perspective 👍

@streamich streamich added v7.8.0 and removed v7.9.0 labels May 4, 2020
@streamich streamich merged commit 496f492 into elastic:master May 4, 2020
@streamich streamich mentioned this pull request May 4, 2020
Merged
@streamich streamich added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label May 4, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

streamich added a commit that referenced this pull request May 4, 2020
@streamich @elasticmachine
Fix 37422 (#64215) (#65111)
8664ccc 
* refactor: 💡 rename Filter -> ExpressionValueFilter

* refactor: 💡 use new filter type in Canvas

* fix: 🐛 fix tests after refactor

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukeelmers lukeelmers lukeelmers approved these changes

@crob611 crob611 crob611 approved these changes

@clintandrewhall clintandrewhall Awaiting requested review from clintandrewhall

@ppisljar ppisljar Awaiting requested review from ppisljar

Assignees
No one assigned
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Canvas] Filter is not using the type property correctly; fix in 8.0
6 participants
@streamich @elasticmachine @clintandrewhall @kibanamachine @crob611 @lukeelmers