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

[data.search] Move SearchSource to common directory. #77823

Merged
merged 7 commits into from
Sep 18, 2020

Conversation

lukeelmers
Copy link
Member

@lukeelmers lukeelmers commented Sep 17, 2020

Part of #65069

This is the second-to-last PR in migrating SearchSource to the server. To keep things simple, all this PR does is relocate the various files required by SearchSource from the public directory to the common directory, and update imports accordingly.

  • contents of data/public/search/search_source moved to common
  • broken imports updated, including a few test files in the maps and visualizations plugins
  • fetch and legacy directories nested inside of search_source since they aren't used externally
  • default search strategy unit test refactored to prevent common importing code from public (see review notes)

The next/final PR will actually handle exposing the service on the server.

No functional changes have been introduced here; just moving code around.

@lukeelmers lukeelmers force-pushed the fix/ssource-to-common branch from 78c3c14 to 88dec35 Compare September 17, 2020 22:08
describe('defaultSearchStrategy', function () {
describe('search', function () {
describe('defaultSearchStrategy', () => {
describe('search', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

In this test file, I needed to move a few things around because it was testing callMsearch, which lives in public. So I instead mocked callMsearch and simply tested that the function was called with the correct args.

Then I moved the existing logic asserting that http.post was called correctly into a separate unit test which lives alongside callMsearch.

Comment on lines +20 to +35
import { Observable } from 'rxjs';
import { IEsSearchRequest, IEsSearchResponse, ISearchOptions } from '../../common/search';

export type ISearch = (
request: IKibanaSearchRequest,
options?: ISearchOptions
) => Observable<IKibanaSearchResponse>;

export type ISearchGeneric = <
SearchStrategyRequest extends IEsSearchRequest = IEsSearchRequest,
SearchStrategyResponse extends IEsSearchResponse = IEsSearchResponse
>(
request: SearchStrategyRequest,
options?: ISearchOptions
) => Observable<SearchStrategyResponse>;

Copy link
Member Author

Choose a reason for hiding this comment

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

For the purposes of this PR, these were the only types from public/search/types which needed to be relocated to common.

@lukeelmers lukeelmers self-assigned this Sep 17, 2020
@lukeelmers lukeelmers added Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:AppArch v7.10.0 v8.0.0 labels Sep 17, 2020
@lukeelmers lukeelmers marked this pull request as ready for review September 17, 2020 22:16
@lukeelmers lukeelmers requested review from a team as code owners September 17, 2020 22:16
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

maps changes LGTM
code review

@lukeelmers
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

changes lgtm

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
data 567 +3 564

page load bundle size

id value diff baseline
data 1.5MB +2.7KB 1.5MB

distributable file count

id value diff baseline
default 45928 +19 45909
oss 26670 +19 26651
total +38

History

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

@lukeelmers lukeelmers merged commit 0f493fa into elastic:master Sep 18, 2020
@lukeelmers lukeelmers deleted the fix/ssource-to-common branch September 18, 2020 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants