Visualizations Plugin -> New Platform Ready #44644

sainthkh · 2019-09-03T10:43:31Z

Summary

Fixes #44306.

Setup NP-ready folder for visualizations plugin in core_plugins/visualizations/

Create core_plugins/visualizations/public/np_ready/ folder.
- Move all files into np_ready folder.
kibana.json manifest
index.ts
plugin.ts
legacy.ts
mocks.ts
Pass in legacy dependencies through __LEGACY object.
Implement .types.registerVisualization() instead of .types.VisTypesRegistryProvider.register() in setup contract.

Parent issue: #44121

Dev Docs

plugins/visualizations => src/legacy/core_plugins/visualizations/public/np_ready/public/

elasticmachine · 2019-09-03T10:43:33Z

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?

mattkime · 2019-09-03T18:20:05Z

retest

elasticmachine · 2019-09-03T19:49:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mattkime · 2019-09-03T18:05:50Z

src/legacy/core_plugins/kibana/public/visualize/wizard/new_vis_modal.tsx

@@ -24,7 +24,7 @@ import { i18n } from '@kbn/i18n';

 import chrome from 'ui/chrome';
 import { VisType } from 'ui/vis';
-import { VisTypeAlias } from 'plugins/visualizations';
+import { VisTypeAlias } from 'src/legacy/core_plugins/visualizations/public/np_ready/public';


I think this would make more sense as a relative path. There are a couple of other references like this in this PR.

I believe np_ready can go directly into visualizations

As #44306 said to create np_ready folder, I did so. But I also thought that it wasn't necessary because visualizations plugin doesn't have any legacy code right now.

I'll revert it back to the first non-np_raedy version.

After investigating a bit, I found out that np_ready cannot be removed.

As visualizations lives in legacy/core_plugins it needs to have package.json and index.ts files for legacy plugin initialization.

When we remove them, we should merge legacy index.ts and np index.ts.

It seems that np_ready is necessary evil for now.

p.s. I've changed all paths to relative in the below.

mattkime · 2019-09-03T20:29:57Z

Hello @sainthkh - it looks like you've picked up an issue that was part of our release planning process. Thank you! I'm looking forward to working with you to get this PR into our codebase.

That said, because this was part of our planning process its also considered critical path for our 7.5 release. We should talk to make sure that our interest and commitment levels align so both sides are happy with how things proceed. Drop me an email at matthew.kime@elastic.co and we'll talk through the details of working together.

sainthkh · 2019-09-03T21:06:18Z

@mattkime I sent you an email.

…from setup().

…acy.ts

sainthkh · 2019-09-04T01:04:04Z

Done. Ready for test and review.

elasticmachine · 2019-09-04T02:10:53Z

Pinging @elastic/kibana-app-arch

mattkime · 2019-09-04T02:27:34Z

LGTM - checking on whether this needs release notes. if so I'll write them.

ppisljar · 2019-09-04T10:58:24Z

jenkins, test this

elasticmachine · 2019-09-04T12:20:45Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Created np_ready/public and moved files into it. * Added kibana.json * Added plugin.ts and moved plugin class into it. * Added legacy.ts and moved ui imports to legacy.ts. * Added mock.ts * Created registerVisualization() and removed VisTypesRegistryProvider from setup(). * Fixed imports from plugins/visualizations. * Removed visualizations from index.ts and call setup directly from legacy.ts

lukeelmers

I know this has already merged, but I had a few comments that I think we should address -- I will make a follow-up PR with some adjustments.

lukeelmers · 2019-09-04T14:19:50Z

src/legacy/core_plugins/kibana/public/visualize/listing/visualize_listing.js

@@ -30,11 +30,13 @@ import { SavedObjectsClientProvider } from 'ui/saved_objects';
 import { VisualizeListingTable } from './visualize_listing_table';
 import { NewVisModal } from '../wizard/new_vis_modal';
 import { VisualizeConstants } from '../visualize_constants';
-import { visualizations } from 'plugins/visualizations';
+import { setup } from '../../../../visualizations/public/np_ready/public/legacy';


I think we should be exporting this contract from the top level public instead, e.g. visualizations/public/legacy

np_ready directory is an implementation detail of the visualizations plugin and consumers of this shim shouldn't need to be aware of whether the directory exists or not.

also (nit) for clarity I think it's nice to do import { setup as visualizations } or setup as visualizationsSetup or similar as we have done elsewhere... would be good to make this consistent across the PR

* Created np_ready/public and moved files into it. * Added kibana.json * Added plugin.ts and moved plugin class into it. * Added legacy.ts and moved ui imports to legacy.ts. * Added mock.ts * Created registerVisualization() and removed VisTypesRegistryProvider from setup(). * Fixed imports from plugins/visualizations. * Removed visualizations from index.ts and call setup directly from legacy.ts

lukeelmers · 2019-09-04T23:54:03Z

Made this follow-up PR with some additional feedback, which also incorporates the changes I was working on in #43730: #44839

(@sainthkh feel free to review that one too if you'd like to discuss further!)

…port paths.

mattkime requested changes Sep 3, 2019

View reviewed changes

sainthkh added 9 commits September 4, 2019 10:02

Created np_ready/public and moved files into it.

66daf9f

Added kibana.json

fe6f975

Added plugin.ts and moved plugin class into it.

f280cea

Added legacy.ts and moved ui imports to legacy.ts.

9f536ad

Removed unused variables in legacy.ts.

f4ced5a

Added mock.ts

4f7ce8e

Created registerVisualization() and removed VisTypesRegistryProvider …

1c9440a

…from setup().

Fixed imports from plugins/visualizations.

e9d8d6e

Removed visualizations from index.ts and call setup directly from leg…

2d879d9

…acy.ts

sainthkh force-pushed the visualization-np-ready branch from a00bf58 to 2d879d9 Compare September 4, 2019 01:03

sainthkh marked this pull request as ready for review September 4, 2019 01:03

mattkime added v7.5.0 Team:AppArch labels Sep 4, 2019

mattkime changed the title ~~Visualization np ready~~ Visualizations Plugin -> New Platform Ready Sep 4, 2019

mattkime approved these changes Sep 4, 2019

View reviewed changes

ppisljar added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 labels Sep 4, 2019

mattkime added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:skip Skip the PR/issue when compiling release notes labels Sep 4, 2019

mattkime merged commit b14d17d into elastic:master Sep 4, 2019

mattkime mentioned this pull request Sep 4, 2019

[7.x] Visualizations Plugin -> New Platform Ready (#44644) #44780

Merged

lukeelmers reviewed Sep 4, 2019

View reviewed changes

wylieconlon mentioned this pull request Sep 4, 2019

[lens] Fix breakage from app-arch movements #44720

Merged

lukeelmers mentioned this pull request Sep 4, 2019

Prep visualizations plugin for NP migration. #44839

Merged

streamich mentioned this pull request Sep 5, 2019

Visualizations 👉 New Platform #44121

Closed

13 tasks

lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Sep 9, 2019

Revert updates from elastic#44644 that changed visualizations shim im…

afd7f56

…port paths.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Visualizations Plugin -> New Platform Ready #44644

Visualizations Plugin -> New Platform Ready #44644

sainthkh commented Sep 3, 2019 •

edited by mattkime

Loading

elasticmachine commented Sep 3, 2019

mattkime commented Sep 3, 2019

elasticmachine commented Sep 3, 2019

mattkime Sep 3, 2019

sainthkh Sep 3, 2019

sainthkh Sep 3, 2019

mattkime commented Sep 3, 2019

sainthkh commented Sep 3, 2019

sainthkh commented Sep 4, 2019

elasticmachine commented Sep 4, 2019

mattkime commented Sep 4, 2019

ppisljar commented Sep 4, 2019

elasticmachine commented Sep 4, 2019

lukeelmers left a comment

lukeelmers Sep 4, 2019

lukeelmers commented Sep 4, 2019 •

edited

Loading

Visualizations Plugin -> New Platform Ready #44644

Visualizations Plugin -> New Platform Ready #44644

Conversation

sainthkh commented Sep 3, 2019 • edited by mattkime Loading

Summary

Dev Docs

plugins/visualizations => src/legacy/core_plugins/visualizations/public/np_ready/public/

elasticmachine commented Sep 3, 2019

mattkime commented Sep 3, 2019

elasticmachine commented Sep 3, 2019

💔 Build Failed

mattkime Sep 3, 2019

Choose a reason for hiding this comment

sainthkh Sep 3, 2019

Choose a reason for hiding this comment

sainthkh Sep 3, 2019

Choose a reason for hiding this comment

mattkime commented Sep 3, 2019

sainthkh commented Sep 3, 2019

sainthkh commented Sep 4, 2019

elasticmachine commented Sep 4, 2019

mattkime commented Sep 4, 2019

ppisljar commented Sep 4, 2019

elasticmachine commented Sep 4, 2019

💚 Build Succeeded

lukeelmers left a comment

Choose a reason for hiding this comment

lukeelmers Sep 4, 2019

Choose a reason for hiding this comment

lukeelmers commented Sep 4, 2019 • edited Loading

sainthkh commented Sep 3, 2019 •

edited by mattkime

Loading

lukeelmers commented Sep 4, 2019 •

edited

Loading