move management registry to new platform #53020

mattkime · 2019-12-13T18:27:58Z

Summary

tldr; Move management registry to new platform to provide for transition to new api.

Steps

Create management plugin
Pass in capabilities dependency
Change all references from import { management } from 'ui/management'; to npStart.plugins.management.legacy.
Implement mocks - karma and jest
Passed on providing types as the legacy api is being deprecated very soon
I18n - minor changes to maintain existing translations (all two of them)
Change management field component reference to direct reference, no ui/management

mattkime · 2019-12-13T23:06:51Z

src/plugins/management/public/mocks/index.ts

-  it('provides ManagementSection', () => {
-    expect(management).to.be.a(ManagementSection);
-  });
+const createStartContract = () => ({


strange diff, not a file move.

mattkime · 2019-12-14T14:25:57Z

@elasticmachine merge upstream

elasticmachine · 2019-12-14T14:25:59Z

merge conflict between base and head

elasticmachine · 2019-12-15T04:28:32Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

lizozom

Tested and works well.
Added a couple comments about type usage

lizozom · 2019-12-16T10:56:59Z

src/legacy/core_plugins/telemetry/public/components/telemetry_form.js

@@ -31,7 +31,7 @@ import {
 } from '@elastic/eui';
 import { PRIVACY_STATEMENT_URL } from '../../common/constants';
 import { OptInExampleFlyout } from './opt_in_details_component';
-import { Field } from 'ui/management';
+import { Field } from '../../../kibana/public/management/sections/settings/components/field/field';


I would consider renaming this in a subsequent PR to something more management specific.

I don't even think this is management specific TBH. I think Field is the component used to render each of the advanced settings options.

Originally we had discussed having a dedicated settings plugin for dealing with the advanced settings UI, but maybe we decided to move it into management instead? I can't remember what the latest thought was on that.

If we do keep it in management though, I 100% agree we should rename. Maybe add a "settings" app / service inside the management plugin, and call it AdvancedSettingsField or something, IDK.

lizozom · 2019-12-16T11:06:44Z

src/plugins/management/public/plugin.ts

+
+import { CoreSetup, CoreStart, Plugin } from 'kibana/public';
+import { ManagementStart } from './types';
+// @ts-ignorets-ignore


Double ts ignore

lizozom · 2019-12-16T11:10:06Z

src/plugins/management/public/types.ts

+ */
+
+export interface ManagementStart {
+  legacy: unknown;


Why not give this type any and avoid adding multiple ts-ignores?

lizozom · 2019-12-16T11:12:17Z

x-pack/legacy/plugins/index_management/public/legacy.ts

 import { createUiStatsReporter } from '../../../../../src/legacy/core_plugins/ui_metric/public';

 export interface LegacyStart {
  management: {
-    getSection: typeof management.getSection;
+    // @ts-ignore
+    getSection: typeof npStart.plugins.management.legacy.getSection;


his applies to all places where typeof npStart... is used

src/plugins/management/public/legacy/section.js

peteharverson

Tested ML and Transforms edits, and LGTM. Just wondering if the ts-ignores on npStart.plugins.management.legacy and management.sections.getSection can be avoided?

peteharverson · 2019-12-16T11:41:05Z

x-pack/legacy/plugins/ml/public/application/management/index.ts

-    url: `#${JOBS_LIST_PATH}`,
-  });
+  // @ts-ignore
+  npStart.plugins.management.legacy


Is this ts-ignore because legacy has been given unknown type in ManagementStart?

will take a look

They were, thanks for the nudge

mattkime · 2019-12-16T13:45:27Z

@elasticmachine merge upstream

jportner

Security changes LGTM

mattkime · 2019-12-16T16:37:55Z

@elasticmachine merge upstream

mattkime · 2019-12-16T22:24:31Z

src/legacy/ui/public/management/components/sidebar_nav.test.ts

@@ -77,6 +77,7 @@ const managementSections = [

 describe('Management', () => {
  it('filters and filters and maps section objects into SidebarNav items', () => {
+    // @ts-ignore


This was giving me some trouble so I decided to ignore it. I'm happy to revisit with some help. Should be addressed in my next PR since there will be a layer of abstraction to merge two different section formats (new and old platforms)

lukeelmers

Reviewed code and tested everything locally. Everything seems to work as expected. I also tested adding a new management section from a NP plugin, and that works too (so I suppose technically this could unblock folks from moving to the new platform today if they really wanted to... although we know this API will be removed before 8.0 still)

I think we might be able to avoid making this change "breaking" altogether by re-exporting the NP contract from the legacy ui/management. If that works, it could simplify the footprint of this PR greatly and put less pressure on any 3rd party devs who may be registering management sections in their own plugins.

src/legacy/ui/public/management/index.js

src/plugins/management/public/plugin.ts

src/plugins/management/public/legacy/section.js

mattkime · 2019-12-17T05:03:09Z

@lukeelmers

I think we might be able to avoid making this change "breaking" altogether by re-exporting the NP contract from the legacy ui/management. If that works, it could simplify the footprint of this PR greatly and put less pressure on any 3rd party devs who may be registering management sections in their own plugins.

import { npStart } from 'ui/new_platform`;
export const legacyManagement = npStart.plugins.management.legacy;

I attempted this and ran into difficulty. I think I ran into lifecycle / compilation problems. I'm seeing trouble navigating between kibana apps. I created a scratch PR to play with to verify - #53253

Edit: Reviewing this. I may have misattributed a problem I saw previously.

lizozom

Diff is much more simple now!
I didn't retest after the recent changes, but overall LGTM

elasticmachine · 2019-12-17T18:13:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 75e4616

History

💔 Build #16003 failed 9e837c3
💔 Build #15927 failed a1ea7d9f50903739aa6e32d42b4202d5ad1b13c3
💔 Build #15887 failed f1dad5179b391290ae5ffce2010fc8668fd4c1f0
💚 Build #15805 succeeded aa40e7c
💔 Build #15771 failed 19de27a

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

lukeelmers

Haven't re-tested since the latest updates were pushed, but code all LGTM!

lukeelmers · 2019-12-17T18:51:59Z

src/legacy/core_plugins/telemetry/public/components/telemetry_form.js

@@ -31,7 +31,7 @@ import {
 } from '@elastic/eui';
 import { PRIVACY_STATEMENT_URL } from '../../common/constants';
 import { OptInExampleFlyout } from './opt_in_details_component';
-import { Field } from 'ui/management';
+import { Field } from '../../../kibana/public/management/sections/settings/components/field/field';


I don't even think this is management specific TBH. I think Field is the component used to render each of the advanced settings options.

Originally we had discussed having a dedicated settings plugin for dealing with the advanced settings UI, but maybe we decided to move it into management instead? I can't remember what the latest thought was on that.

If we do keep it in management though, I 100% agree we should rename. Maybe add a "settings" app / service inside the management plugin, and call it AdvancedSettingsField or something, IDK.

* move management registry to new platform

mattkime added 3 commits December 13, 2019 12:25

move management registry to new platform

ce52843

fix typescript, fix a jest test or two

6d5b360

add mocks

e656b0a

mattkime commented Dec 13, 2019

View reviewed changes

mattkime added 4 commits December 13, 2019 20:56

i18n fix

d9e84dc

mock ui/new_platform in a number tests

fce62f0

fix mocks for management api

3e3eac2

fix telemetry test

31305e0

mattkime added 2 commits December 14, 2019 13:16

Merge branch 'master' into move_management_registry_to_new_platfom

925e694

fix test

1abbe0a

mattkime added the Team:AppArch label Dec 15, 2019

mattkime added Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v8.0.0 v7.6.0 labels Dec 15, 2019

mattkime marked this pull request as ready for review December 15, 2019 05:58

mattkime requested review from a team as code owners December 15, 2019 05:58

lizozom reviewed Dec 16, 2019

View reviewed changes

peteharverson approved these changes Dec 16, 2019

View reviewed changes

jportner approved these changes Dec 16, 2019

View reviewed changes

mattkime commented Dec 16, 2019

View reviewed changes

lukeelmers reviewed Dec 17, 2019

View reviewed changes

src/legacy/ui/public/management/index.js Show resolved Hide resolved

src/plugins/management/public/plugin.ts Show resolved Hide resolved

src/plugins/management/public/legacy/section.js Show resolved Hide resolved

mattkime force-pushed the move_management_registry_to_new_platfom branch from a60d252 to 9e837c3 Compare December 17, 2019 07:26

mattkime added 4 commits December 17, 2019 07:51

eslint fix

1f38a12

Merge branch 'master' into move_management_registry_scratch_2

d33c924

fix karma tests

92a336c

ts fixes

75e4616

mattkime force-pushed the move_management_registry_to_new_platfom branch from 9e837c3 to 75e4616 Compare December 17, 2019 16:53

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

lizozom approved these changes Dec 17, 2019

View reviewed changes

lukeelmers approved these changes Dec 17, 2019

View reviewed changes

mattkime merged commit 2d36b21 into elastic:master Dec 17, 2019

legrego mentioned this pull request Jan 6, 2020

[7.x] Reorganizing spaces client-side plugin (#53644) #54044

Merged

mattkime mentioned this pull request Jan 6, 2020

[7.x] move management registry to new platform (#53020) #54058

Merged

mattkime added a commit to mattkime/kibana that referenced this pull request Jan 6, 2020

move management registry to new platform (elastic#53020)

ea2f1aa

* move management registry to new platform

mattkime added a commit that referenced this pull request Jan 6, 2020

move management registry to new platform (#53020) (#54058)

7913df4

* move management registry to new platform

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move management registry to new platform #53020

move management registry to new platform #53020

mattkime commented Dec 13, 2019 •

edited

Loading

mattkime Dec 13, 2019

mattkime commented Dec 14, 2019

elasticmachine commented Dec 14, 2019

elasticmachine commented Dec 15, 2019

lizozom left a comment

lizozom Dec 16, 2019

lukeelmers Dec 17, 2019

lizozom Dec 16, 2019

lizozom Dec 16, 2019

lizozom Dec 16, 2019 •

edited

Loading

peteharverson left a comment

peteharverson Dec 16, 2019

mattkime Dec 16, 2019

mattkime Dec 16, 2019

mattkime commented Dec 16, 2019

jportner left a comment •

edited

Loading

mattkime commented Dec 16, 2019

mattkime Dec 16, 2019

lukeelmers left a comment

mattkime commented Dec 17, 2019 •

edited

Loading

lizozom left a comment •

edited

Loading

elasticmachine commented Dec 17, 2019

lukeelmers left a comment

lukeelmers Dec 17, 2019

move management registry to new platform #53020

move management registry to new platform #53020

Conversation

mattkime commented Dec 13, 2019 • edited Loading

Summary

Choose a reason for hiding this comment

mattkime commented Dec 14, 2019

elasticmachine commented Dec 14, 2019

elasticmachine commented Dec 15, 2019

lizozom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizozom Dec 16, 2019 • edited Loading

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkime commented Dec 16, 2019

jportner left a comment • edited Loading

Choose a reason for hiding this comment

mattkime commented Dec 16, 2019

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

mattkime commented Dec 17, 2019 • edited Loading

lizozom left a comment • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Dec 17, 2019

💚 Build Succeeded

History

lukeelmers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkime commented Dec 13, 2019 •

edited

Loading

lizozom Dec 16, 2019 •

edited

Loading

jportner left a comment •

edited

Loading

mattkime commented Dec 17, 2019 •

edited

Loading

lizozom left a comment •

edited

Loading