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] Migrate Maps embeddables to NP #63976

Merged
merged 3 commits into from
Apr 23, 2020
Merged

[Maps] Migrate Maps embeddables to NP #63976

merged 3 commits into from
Apr 23, 2020

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Apr 20, 2020 •
edited
Loading

Moves maps embeddables functionality completely to the New Platform. Removes legacy presence.

For Maps team testing: It should work the same as it always did, it's just now registered on the NP side. Sample data or custom dashboards are the best ways to test functionality

For SIEM/Uptime testing: Everything you're doing now should also still work the same. You're welcome to also check potentially via debugging breakpoint on your NP side of things to see if the Maps plugin embeddable is available as you'd expect (note: you'd also likely have to add maps as a dependency to your kibana.json)

@kindsun kindsun added Feature:New Platform [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 v7.8.0 labels Apr 20, 2020
@kindsun kindsun requested review from a team as code owners April 20, 2020 16:22
@elasticmachine
Copy link
Contributor

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

@kindsun kindsun marked this pull request as draft April 20, 2020 17:30
@kindsun kindsun mentioned this pull request Apr 20, 2020
Closed
11 tasks
@kindsun kindsun force-pushed the np-migrate-embeddables branch from cab61d2 to ebccdcf Compare April 22, 2020 17:49
@kindsun kindsun requested review from nreese, rylnd and shahzad31 April 22, 2020 18:05
@kindsun kindsun marked this pull request as ready for review April 22, 2020 18:05
@kindsun kindsun added the release_note:skip Skip the PR/issue when compiling release notes label Apr 22, 2020
@kindsun kindsun removed the request for review from a team April 22, 2020 18:21
nreese
nreese approved these changes Apr 22, 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.

lgtm with green ci
code review

@rylnd rylnd mentioned this pull request Apr 22, 2020
Merged
4 tasks
justinkambic
justinkambic approved these changes Apr 23, 2020
View reviewed changes
Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

I looked at the code; I don't see anything objectionable.

I pulled this down and ran it, and merged it into a test branch for my NP client migration, and was able to run my team's solution and use the map embeddable, so from Uptime's point of view I am good to go here.

LGTM WFG

@kindsun
Copy link
Contributor Author

kindsun commented Apr 23, 2020

Currently looking into a fix for jest tests leveraging maps embeddables in NP. There are a few failures, but they all look something like this:

[2020-04-22T20:19:53.748Z]  FAIL  legacy/plugins/canvas/common/lib/autocomplete.test.ts
[2020-04-22T20:19:53.748Z]   ● Test suite failed to run
[2020-04-22T20:19:53.748Z] 
[2020-04-22T20:19:53.748Z]     Cannot find module '!!file-loader!mapbox-gl/dist/mapbox-gl-csp-worker' from 'view.js'
[2020-04-22T20:19:53.748Z] 
[2020-04-22T20:19:53.748Z]     However, Jest was able to find:
[2020-04-22T20:19:53.748Z]     	'./view.js'

The issue appears isolated to jest tests importing maps via embeddables or otherwise but doesn't affect normal functionality. It might be possible to mock these imports for each of the tests, but more ideally we'd tackle this in a more global way. There may have been some background work done on jest test configuration to support #51675 where these inline file-loader imports were originally introduced. If that's the case, we'll need to replicate this work if possible for NP.

cc. @mistic @thomasneirynck

@mistic mistic mentioned this pull request Apr 23, 2020
Merged
@kindsun
Copy link
Contributor Author

kindsun commented Apr 23, 2020

@elasticmachine merge upstream

XavierM
XavierM approved these changes Apr 23, 2020
View reviewed changes
Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

LGTM
I was able to load the embeddable map with no problem on our legacy siem
image

@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 97c7127 into elastic:master Apr 23, 2020
@kindsun kindsun deleted the np-migrate-embeddables branch April 23, 2020 22:30
@kindsun kindsun mentioned this pull request Apr 23, 2020
Merged
kindsun pushed a commit to kindsun/kibana that referenced this pull request Apr 23, 2020
@elasticmachine
[Maps] Migrate Maps embeddables to NP (elastic#63976)
a5ad497 
* Migrate maps embeddables. Clean up legacy presence

* Fix type error

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 24, 2020
@gmmorris
Merge branch 'master' into alerting/no-explicit-any
7b89039 
* master: (70 commits)
  KQL removes leading zero and breaks query (elastic#62748)
  [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193)
  [ML] Changes transforms wizard UI text (elastic#64150)
  [Alerting] change server log action type .log to .server-log in README (elastic#64124)
  [Metrics UI] Design Refresh: Inventory View, Episode 1 (elastic#64026)
  chore(NA): reduce siem bundle size using babel-plugin-transfor… (elastic#63269)
  chore(NA): use core-js instead of babel-polyfill on canvas sha… (elastic#63486)
  skip flaky suite (elastic#61173)
  skip flaky suite (elastic#62497)
  Renamed ilm policy for event log so it is not prefixed with dot (elastic#64262)
  [eslint] no_restricted_paths config cleanup (elastic#63741)
  Add Oil Rig Icon from @elastic/maki (elastic#64364)
  [Maps] Migrate Maps embeddables to NP (elastic#63976)
  [Ingest] Data streams list page (elastic#64134)
  chore(NA): add file-loader into jest moduleNameMapper (elastic#64330)
  [DOCS] Added images to automating report generation (elastic#64333)
  [SIEM][CASE] Api Integration Tests: Configuration (elastic#63948)
  Expose ability to check if API Keys are enabled (elastic#63454)
  [DOCS] Fixes formatting in alerting doc (elastic#64338)
  [data.search.aggs]: Create agg types function for terms agg. (elastic#63541)
  ...
kindsun pushed a commit that referenced this pull request Apr 24, 2020
[7.x] [Maps] Migrate Maps embeddables to NP (#63976) (#64378)
9017685
@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

@justinkambic justinkambic justinkambic approved these changes

@nreese nreese nreese approved these changes

@XavierM XavierM XavierM approved these changes

@rylnd rylnd Awaiting requested review from rylnd

@shahzad31 shahzad31 Awaiting requested review from shahzad31

Assignees
No one assigned
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation Feature:New Platform 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 @XavierM @nreese @justinkambic