-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Move @kbn/es-query into data plugin - filters folder #49843
Conversation
3083638
to
ffc45e7
Compare
dc3f00c
to
207a8f1
Compare
💚 Build Succeeded
|
207a8f1
to
ed4e645
Compare
Pinging @elastic/kibana-app-arch (Team:AppArch) |
💚 Build Succeeded
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks mostly great. Haven't tested yet.
- I see that esFilters is consumed from /siem/server. Does this mean esFilters should be a common util?
- I'd love someone else's opinion on the esFilters name (it's fine by me, but I'm biased)
x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/types.ts
Outdated
Show resolved
Hide resolved
ed4e645
to
508585c
Compare
💚 Build Succeeded
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code LGTM
Please do not merge before @Bargs has take a look. |
508585c
to
d60b7a3
Compare
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and tested.
Lets wait for @Bargs to approve as well and go ahead and merge this.
@alexwizp @lizozom Changes look good but I do have one question in regards to the nested filter PR I have open. In that PR I added helper functions to each filter type's TS file for getting the target field from the filter (example). I then use those helpers inside a new file in the If we can answer that one question, I'm cool with merging this PR 👍 |
@Bargs yes, you can temporary put your methods into filters/utils folder. Also next week we are going to move |
Dismissing reviews from ml and canvas as this is only an import change. * Move @kbn/es-query into data plugin - filters folder * fix PR comments
Summary
Part of #42885
What was done in this PR:
es-query/filters
into new folderesFilters
namespaceMocha -> Jest
buildFilter
methods were moved directly to filtersChecklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers