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

[Maps] Migrate Maps server to NP #66510

Merged
merged 27 commits into from
May 21, 2020
Merged

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented May 14, 2020

Pulling this out as a separate PR from #65079 since it has a pretty large footprint on its own. Migrates the remaining pieces on the Maps server side to NP. Includes:

  • Telemetry
  • Server-side routing
  • Basic server-side query/params validation
  • Sample data registration
  • Tutorial registration
  • Return config from client and server setup
  • Remove getInjectedVarFunc references and connect map config
  • Reduce legacy plugin init to a minimal presence required for plugin registration
  • Many other connecting pieces

There are a few aspects of Maps in NP worth future passes that I noticed during this migration. I'll generate issues for several I've listed below and any others I think of. Offhand, this might include:

  • Now that we're leveraging the new server-side router, take further advantage of router validation
  • Organize server/plugin.ts. Maybe move sample data and tutorial registration to a separate file
  • Audit config handling (related)

@kindsun kindsun added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 Feature:NP Migration v7.9.0 labels May 14, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@kindsun kindsun added the release_note:skip Skip the PR/issue when compiling release notes label May 14, 2020
@kindsun kindsun marked this pull request as ready for review May 14, 2020 22:14
@kindsun kindsun requested a review from a team as a code owner May 14, 2020 22:14
@nreese
Copy link
Contributor

nreese commented May 14, 2020

I am getting the error "log [16:32:45.317] [fatal][root] { ValidationError: child "xpack" fails because [child "maps" fails because ["showMapsInspectorAdapter" is not allowed]]" with xpack.maps.showMapsInspectorAdapter: true. How do you set xpack.maps.showMapsInspectorAdapter now?

@kindsun
Copy link
Contributor Author

kindsun commented May 14, 2020

I am getting the error "log [16:32:45.317] [fatal][root] { ValidationError: child "xpack" fails because [child "maps" fails because ["showMapsInspectorAdapter" is not allowed]]" with xpack.maps.showMapsInspectorAdapter: true. How do you set xpack.maps.showMapsInspectorAdapter now?

Something's broken here, I see it's failing tests as well. Will look into it! Fixed. We weren't able to complete remove legacy config validation so long as we have some code in legacy. We need some validation as a placeholder while there's a legacy presence for tests.

@kindsun kindsun marked this pull request as draft May 15, 2020 21:55
@kindsun kindsun marked this pull request as ready for review May 19, 2020 16:45
@kindsun
Copy link
Contributor Author

kindsun commented May 19, 2020

@thomasneirynck @nreese This is ready for another pass when you have a chance. A few things to note since this was last reviewed:

  • @nreese requested updates to config naming have been made
  • Even though we're handling configuration serving and validation in NP, so long as we have any presence in legacy, some sort of basic config validation must be present for tests. I've updated x-pack/legacy/plugins/maps/index.js with the recommended placeholder configuration for tests
  • Pass new maps config from legacy for service init
  • Any TS-specific warnings in legacy client code have been ignored since these files will all be going away anyway in [Maps] Migrate maps client router to react #65079
  • Updated how config is obtained for compat with karma tests which load the app in a different way where you can't rely on np plugins having been created for legacy import.
  • Added some basic route validation for query and params on server routing

@kindsun kindsun requested a review from nreese May 19, 2020 17:55
@kindsun kindsun requested a review from nreese May 20, 2020 15:38
x-pack/plugins/maps/public/index.ts Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/plugin.ts Show resolved Hide resolved
x-pack/plugins/maps/public/kibana_services.d.ts Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/kibana_services.d.ts Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/kibana_services.d.ts Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/kibana_services.d.ts Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/plugin.ts Show resolved Hide resolved
x-pack/plugins/maps/server/index.ts Show resolved Hide resolved
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.

Just one comment remaining about removing some ts-ignore

lgtm
code review, tested in chrome

x-pack/plugins/maps/public/index.ts Outdated Show resolved Hide resolved
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants