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.

Sign up for GitHub

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

Jump to bottom

[Maps] Move layers to np maps #61877

Merged
merged 19 commits into from
Apr 6, 2020
Merged

[Maps] Move layers to np maps #61877

merged 19 commits into from
Apr 6, 2020

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Mar 30, 2020

Follow-up to #59585. This PR moves layers to NP. For the most part, it really is just physically moving these files and updating refs to the files

@kindsun kindsun added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.8.0 labels Mar 30, 2020
@kindsun kindsun requested review from a team as code owners March 30, 2020 18:32
@elasticmachine
Copy link
Contributor

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

thomasneirynck
thomasneirynck reviewed Mar 30, 2020
View reviewed changes
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

Thanks for moving! 💦

In general, I think we should strive for as clean a move as possible. It's easier to follow along when the dependencies are legacy->np instead of the other way around. NP is more&more becoming the source-of-truth (especially for utils/configs), so it's iffy referencing from np to legacy when it's not strictly necessary.

  1. There's some unnecessary dependencies from NP to legacy. Instead of referencing legacy, these should reference the original NP-equivalent.
  • `constants
  • kibana_services
  • i18n_getters
  1. While it may be "technically" allowed, I would already start moving legacy module dependencies wholesale, especially the fairly dummy config and utility ones. I don't think there's a particular reason to keep them hanging around in legacy, when we can just move them wholesale right now.
  • legacy/maps/common/descriptor_types: (it pretty much defines the layers/* data-model).
  • elasticsearch_geo_utils
  • connected_components/map/mb_utils (this one actually has a todo anyway).
  1. I'd move over some of the stray utility-functions to NP already (the other utilities can stay in legacy, but just lift out the simple ones).
  • isRetina from meta.

I think I caught most of them. There's a few react-components, and utilities relying on chrome which we can keep in legacy in the short-term.

x-pack/plugins/maps/public/layers/sources/ems_unavailable_message.js Outdated Show resolved Hide resolved
@kindsun
Copy link
Contributor Author

kindsun commented Mar 30, 2020

Thanks @thomasneirynck. There are several paths that got updated to pull from legacy. I expected this, but usually these get flagged in subsequent linting. It's not at all clear to me why that's not happening here. To resolve these issues and to address the remainder of your comments, I'll need to import a few layers-adjacent files. I'll re-request review when I've completed this work!

snide
snide approved these changes Mar 30, 2020
View reviewed changes
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

SCSS files moved only. No need for design review. Did not test functionality of PR.

@thomasneirynck thomasneirynck self-requested a review April 1, 2020 16:14
@kindsun kindsun added the WIP Work in progress label Apr 1, 2020
@kindsun kindsun removed the WIP Work in progress label Apr 3, 2020
@kindsun
Copy link
Contributor Author

kindsun commented Apr 3, 2020

@thomasneirynck @nreese This should be ready for another pass. For the most part, this PR still amounts largely to just moving the layers files to NP, but with a couple of updates:

  • I increased the number of services initialized by NP kibana_services and decreased the number handled by legacy kibana_services since many of our service needs are on the NP side now. This will continue until there are really no kibana_services on the legacy side
  • I added in several files that support layers and aren't really used by legacy. I tried to keep this minimal and really just focused on files layers needs.

nreese
nreese approved these changes Apr 6, 2020
View reviewed changes
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.

Great to see this much code get migrated to NP!

lgtm
code review

thomasneirynck
thomasneirynck approved these changes Apr 6, 2020
View reviewed changes
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

lgtm when green. np getting there...

x-pack/plugins/maps/public/kibana_services.js
@@ -3,7 +3,89 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { esFilters, search } from '../../../../src/plugins/data/public';
Copy link
Contributor

Choose a reason for hiding this comment

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

great to see so many functions moved over

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@kindsun kindsun merged commit f80925a into elastic:master Apr 6, 2020
@kindsun kindsun deleted the np-move-layers-to-np-maps branch April 6, 2020 20:01
@kindsun kindsun mentioned this pull request Apr 6, 2020
Merged
kindsun pushed a commit to kindsun/kibana that referenced this pull request Apr 6, 2020
[Maps] Move layers to np maps (elastic#61877)
27b6557 
* Move layers to new location

* Update layer path refs

* Update np kibana services to cover all required services

* Init np kibana services in legacy plugin. Port init functions to np

* Path updates, supporting file moves, general clean up

* More moves of related files and clean-up of legacy refs

* Path updates. Typescript warning fixes

* Update test paths

* Clean up unused kibana services usage in legacy

* Remove unused http ref

* Test fixes and clean up

* Remove unused snapshots

* Add np service init to embeddables too

* Move validate color picker to NP
kindsun pushed a commit that referenced this pull request Apr 6, 2020
[7.x] [Maps] Move layers to np maps (#61877) (#62697)
5c1c094 
* Move layers to new location

* Update layer path refs

* Update np kibana services to cover all required services

* Init np kibana services in legacy plugin. Port init functions to np

* Path updates, supporting file moves, general clean up

* More moves of related files and clean-up of legacy refs

* Path updates. Typescript warning fixes

* Update test paths

* Clean up unused kibana services usage in legacy

* Remove unused http ref

* Test fixes and clean up

* Remove unused snapshots

* Add np service init to embeddables too

* Move validate color picker to NP
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 7, 2020
@gmmorris
Merge branch 'master' into alerting/tls-disable
a328e16 
* master: (36 commits)
  [data.search.aggs] Remove service getters from agg types (elastic#61628)
  fixing APM internationalization (elastic#62757)
  fix: 🐛 correctly create error on no_matching_indices (elastic#61257)
  [Lens] Remove all legacy imports (elastic#62596)
  Add label for ace editor (elastic#62588)
  [ML] Show better file structure finder explanations (elastic#62316)
  Fix old pathes in eslintrc (elastic#62580)
  [Uptime] Improve Telemetry test (elastic#62428)
  [SIEM] Adds sort rules Cypress test (elastic#62700)
  [Uptime]Abstracted 'access:uptime-read' tag into a wrapper for… (elastic#62576)
  fixing bug (elastic#62577)
  [Maps] Allow updating requestType for ESGeoGridSource (elastic#62365)
  [Maps] do not show circle border when symbol size is zero (elastic#62644)
  [Maps] Always show current zoom level (elastic#62684)
  bc5 siem rules merge (elastic#62679)
  Revert "[Monitoring] Cluster state watch to Kibana alerting (elastic#61685)"
  Fix visual tests (elastic#62660)
  [Telemetry] update crypto packages (elastic#62469)
  [DOCS] Removed references to left (elastic#60807)
  [Maps] Move layers to np maps (elastic#61877)
  ...
@kindsun kindsun mentioned this pull request Apr 15, 2020
Closed
11 tasks
@thomasneirynck thomasneirynck mentioned this pull request May 29, 2020
Closed
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snide snide snide approved these changes

@nreese nreese nreese approved these changes

@thomasneirynck thomasneirynck thomasneirynck approved these changes

Assignees
No one assigned
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.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kindsun @elasticmachine @kibanamachine @snide @nreese @thomasneirynck