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 legacy/server/index_patterns -> src/plugins/data/server #46537

Merged
merged 23 commits into from
Nov 6, 2019

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Sep 24, 2019

Summary

Partially solves #44190.

  • Moved code from legacy/server/index_patterns to src/plugins/data/server.
  • Renamed IndexPatternsService to IndexPatternsFetcher. Because services are classes used by plugins that have setup(), start(), stop() methods. It can cause confusion.
  • Exposed IndexPatternsApiClient apis from IndexPatterns class. And used it in x-pack/beats_management.

Note.

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@sainthkh sainthkh marked this pull request as ready for review September 25, 2019 03:46
@sainthkh sainthkh requested a review from a team September 25, 2019 03:46
@sainthkh sainthkh requested a review from a team as a code owner September 25, 2019 03:46
@ppisljar ppisljar added release_note:skip Skip the PR/issue when compiling release notes review Team:AppArch v7.5.0 v8.0.0 labels Sep 25, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@ppisljar
Copy link
Member

jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sainthkh
Copy link
Contributor Author

@ppisljar As I wrote in the note, there's a bug in eslint. So, it won't pass until #46544 is merged. The failed test in firefox is a flaky test.

I've run the test on my local for 5 times, it all passed.

@sainthkh sainthkh force-pushed the move-index-patterns-server-pt2 branch from d635764 to 5315f22 Compare September 27, 2019 02:51
@ppisljar
Copy link
Member

@elasticmachine merge upstream

@streamich streamich self-assigned this Oct 3, 2019
@lizozom
Copy link
Contributor

lizozom commented Oct 28, 2019

@elasticmachine merge upstream

@lizozom
Copy link
Contributor

lizozom commented Oct 31, 2019

Jenkins test this

@lizozom
Copy link
Contributor

lizozom commented Oct 31, 2019

@sainthkh seems like there's another reference to IndexPatternsService in getKueryBarIndexPattern.ts that needs an update.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sainthkh sainthkh requested a review from a team as a code owner October 31, 2019 22:25
@sainthkh sainthkh requested a review from lizozom October 31, 2019 22:25
@lizozom
Copy link
Contributor

lizozom commented Nov 4, 2019

@elasticmachine merge upstream

@lizozom
Copy link
Contributor

lizozom commented Nov 5, 2019

Jenkins test this

@lizozom
Copy link
Contributor

lizozom commented Nov 5, 2019

@elasticmachine merge upstream

@lizozom
Copy link
Contributor

lizozom commented Nov 5, 2019

Jenkins, test this

@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 changes look good.

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.

@sainthkh I'm still getting an error when navigating to APM application:

image

Could you add a test to catch this failure please?

@sainthkh
Copy link
Contributor Author

sainthkh commented Nov 6, 2019

@lizozom I found out that the bug has something to do with indexPatternsFetcher (because APM team decided to use it), but it wasn't introduced in this PR.

After following the history, I found out that it was introduced in 46304f0. If you check out that commit and a10cf38 (the commit right below that), 404 error doesn't show on a10, but on 463.

I guess it should be a separate issue and PR.

@dgieselaar
Copy link
Member

@sainthkh I might be missing some context, but I cannot reproduce the 404 on master, if that is what you're suggesting. Can you?

@sainthkh
Copy link
Contributor Author

sainthkh commented Nov 6, 2019

Yes, I found 404 on master.

  1. Check out that commit. + yarn kbn bootstrap
  2. Start up kibana.
  3. Set up flight sample. This step might be unrelated.
  4. Turn on Chrome devtool console tab. Visit apm page.
  5. 404 error.

Env: ubuntu 18.04.

It's run from main kibana folder, not x-pack.

If you cannot reproduce it, please let me know. I'll check it out again tomorrow.

@lizozom
Copy link
Contributor

lizozom commented Nov 6, 2019

@dgieselaar @sainthkh I can confirm that this bug happens on master.

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

@dgieselaar
Copy link
Member

@lizozom strange, I've not been able to reproduce it on any of our environments. Do you have any settings that are out of the ordinary?

@lizozom
Copy link
Contributor

lizozom commented Nov 6, 2019

@dgieselaar nope just fresh out of the box setup.
DM me.

Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

404 is likely because of lack of APM indices. Not a concern. Verified the branch against clusters with APM indices and all seems fine.

@lizozom lizozom merged commit 3f130ba into elastic:master Nov 6, 2019
lizozom pushed a commit to lizozom/kibana that referenced this pull request Nov 7, 2019
…#46537)


Dismissed @elastic/kibana-app review of import changes.

ts-ignored @ts-ignore

* Moved files to plugins/data/server.

* Renamed IndexPatternsService to IndexPatternsFetcher and created new IndexPatternsService

* Set routerPath.

* Fixed type error.

* Changed beats_management _fields_for_wildcard request with data/public/legacy api.

* Fixed changed paths.

* Fixes crashes after merge.

* Updated path for clarity.

* Applied Plugin interface to service.

* Fixed test failure caused by non camel case local variable name.

* Fixed import to IndexPatternsService to IndexPatternsFetcher.
lizozom pushed a commit that referenced this pull request Nov 7, 2019
…#50081)

Dismissed @elastic/kibana-app review of import changes.

ts-ignored @ts-ignore

* Moved files to plugins/data/server.

* Renamed IndexPatternsService to IndexPatternsFetcher and created new IndexPatternsService

* Set routerPath.

* Fixed type error.

* Changed beats_management _fields_for_wildcard request with data/public/legacy api.

* Fixed changed paths.

* Fixes crashes after merge.

* Updated path for clarity.

* Applied Plugin interface to service.

* Fixed test failure caused by non camel case local variable name.

* Fixed import to IndexPatternsService to IndexPatternsFetcher.
@streamich streamich mentioned this pull request Nov 11, 2019
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes review v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants