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

Move @kbn/es-query into data plugin - filters folder #49843

Merged
merged 2 commits into from
Nov 8, 2019

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Oct 31, 2019

Summary

Part of #42885

What was done in this PR:

  • move es-query/filters into new folder
  • new namespace was created for filters. Now all code related to filters located in esFilters namespace
  • tests were migrated from Mocha -> Jest
  • lib folder was removed, buildFilter methods were moved directly to filters

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@alexwizp alexwizp force-pushed the es-query-filters branch 5 times, most recently from 3083638 to ffc45e7 Compare November 4, 2019 16:56
@elastic elastic deleted a comment from elasticmachine Nov 4, 2019
@elastic elastic deleted a comment from elasticmachine Nov 4, 2019
@elastic elastic deleted a comment from elasticmachine Nov 4, 2019
@elastic elastic deleted a comment from elasticmachine Nov 4, 2019
@elastic elastic deleted a comment from elasticmachine Nov 4, 2019
@alexwizp alexwizp force-pushed the es-query-filters branch 2 times, most recently from dc3f00c to 207a8f1 Compare November 5, 2019 07:39
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

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

@alexwizp alexwizp marked this pull request as ready for review November 5, 2019 11:54
@alexwizp alexwizp requested a review from a team November 5, 2019 11:54
@alexwizp alexwizp requested review from a team as code owners November 5, 2019 11:54
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@lizozom lizozom left a 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.

  1. I see that esFilters is consumed from /siem/server. Does this mean esFilters should be a common util?
  2. I'd love someone else's opinion on the esFilters name (it's fine by me, but I'm biased)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@ppisljar ppisljar 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

@TinaHeiligers
Copy link
Contributor

Please do not merge before @Bargs has take a look.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@lizozom lizozom left a 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.

@Bargs
Copy link
Contributor

Bargs commented Nov 6, 2019

@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 es_query directory. Since this PR makes the code in the filters directory inaccessible from the package, how should I change my code to make it work after this PR is merged? Should I just put the field finding helpers in the utils/filters.js file for the time being, as was done for a few other helpers that needed to be used in both places (at least that's my understanding of that file from looking at the changes)?

If we can answer that one question, I'm cool with merging this PR 👍

@lizozom
Copy link
Contributor

lizozom commented Nov 6, 2019

@Bargs IMO you are right about putting them, temporarily, inside the utils folder.
Lets wait until @alexwizp is online tomorrow to confirm.

@alexwizp
Copy link
Contributor Author

alexwizp commented Nov 7, 2019

@Bargs yes, you can temporary put your methods into filters/utils folder. Also next week we are going to move kuery and es-query folders into new platform. If you can wait with your PR for 1 week it will be great.

@lizozom lizozom merged commit c3951e9 into elastic:master Nov 8, 2019
alexwizp added a commit to alexwizp/kibana that referenced this pull request Nov 8, 2019
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
alexwizp added a commit that referenced this pull request Nov 8, 2019
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
@Bargs Bargs mentioned this pull request Nov 14, 2019
1 task
@alexwizp alexwizp deleted the es-query-filters branch January 4, 2020 08:18
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.

6 participants