Management - New platform api #52579

mattkime · 2019-12-09T23:03:09Z

Summary

Addresses #51540

Implements Kibana platform management api along side the legacy management api, as documented here - #43631

but with the following modifications

Management Section getAvailable has been removed. It depends upon the capabilities api but this is an incomplete solution for determining whether a given management app should be displayed. ManagementApp will provide enable and disable methods which plugins can use as appropriate. getEnabledApps replaces it, which filters on enabled and sorts on order.
The mount function for ManagementApp will provide a setBreadcrumbs function available via the params argument. It provides core.chrome.setBreadcrumbs functionality but provides the top level breadcrumb.

Implementation note - Each management section is registered as a top level app inside a ui chrome (the management sidebar) rather than a single management app with extensions.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes [i18n support]~~(https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

Dev Docs

Management API for Kibana Platform

Management API for Kibana Platform implemented. Demonstration code available at test/plugin_functional/plugins/management_test_plugin/public/plugin.tsx

…ement app registry to new platform side

kibanamachine · 2019-12-21T05:06:07Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 80d4f46

History

💔 Build #16844 failed 7b1499f
💔 Build #16784 failed 50fea18
💔 Build #16609 failed 6ef6072
💔 Build #16331 failed 9da91b7
💔 Build #16327 failed b2d1a92

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

legrego · 2020-01-07T13:10:31Z

src/plugins/management/public/management_service.ts

+      euiIconType: 'logoElasticsearch',
+    });
+
+    return {


Will the NP Management API offer an equivalent to the LP deregister function? Security uses this today to hide the security section if Security becomes disabled, either via license change, or ES config change. /cc @azasypkin

ManagementApp provides .disable() and .enable() for similar functionality.

legrego

Spaces changes LGTM

ryankeairns

LGTM for design. The hard-coded pixel values have been removed; use of _index.scss files looks good.

streamich

LGTM. Ran on Mac/Chrome.

Added few questions and optional comments.

Also, if section does not have an icon it seems to be shifted left:

It is a minor thing, but since icon is optional, maybe we want to left-align the sections without icons with the rest.

streamich · 2020-01-08T09:07:01Z

src/plugins/management/public/components/management_sidebar_nav.tsx

@@ -0,0 +1,204 @@
+/*


In this /components folder would be nice to have each component in its own folder.

streamich · 2020-01-08T09:10:20Z

src/plugins/management/public/management_service.ts

+  constructor() {
+    this.sections = [];
+  }


Seems constructor is not doing anything.

streamich · 2020-01-08T09:15:08Z

src/plugins/management/public/management_service.ts

+  private sharedInterface = {
+    getSection: this.getSection.bind(this),
+    getSectionsEnabled: this.getSectionsEnabled.bind(this),
+    getAllSections: this.getAllSections.bind(this),
+  };


Maybe these functions could be defined here directly.

private sharedInterface = { getSection: (sectionId: ManagementSection['id']) => this.sections.find(section => section.id === sectionId), getSectionsEnabled: () => this.sections .filter(section => section.getAppsEnabled().length > 0) .sort((a, b) => a.order - b.order), getAllSections: () => this.sections, };

streamich · 2020-01-08T09:16:07Z

src/plugins/management/public/management_service.ts

+    getAllSections: this.getAllSections.bind(this),
+  };
+
+  public setup = (


Curious, why is setup defined as instance member instead of a method?

Suggested change

public setup = (

public setup(

streamich · 2020-01-08T09:16:22Z

src/plugins/management/public/management_service.ts

+    };
+  };
+
+  public start = (navigateToApp: CoreStart['application']['navigateToApp']) => ({


Suggested change

public start = (navigateToApp: CoreStart['application']['navigateToApp']) => ({

public start(navigateToApp: CoreStart['application']['navigateToApp']) => ({

streamich · 2020-01-08T09:38:20Z

src/plugins/management/public/components/management_sidebar_nav.tsx

+          mobileTitle={this.renderMobileTitle()}
+          isOpenOnMobile={this.state.isSideNavOpenOnMobile}
+          toggleOpenOnMobile={this.toggleOpenOnMobile}
+          // @ts-ignore


This comment seems not necessary.

streamich · 2020-01-08T09:39:54Z

src/plugins/management/public/components/management_sidebar_nav.tsx

+
+const sectionVisible = (section: LegacySection | LegacyApp) => !section.disabled && section.visible;
+
+export const sideNavItems = (sections: ManagementSection[], selectedId: string) =>


Suggested change

export const sideNavItems = (sections: ManagementSection[], selectedId: string) =>

const sideNavItems = (sections: ManagementSection[], selectedId: string) =>

streamich · 2020-01-08T09:45:04Z

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

@@ -22,7 +22,7 @@ import { IndexedArray } from '../../../../legacy/ui/public/indexed_array';

 const listeners = [];


Curious, why listerners are defined at module scope instead of being a member of LegacyManagementSection instance.

No particular reason. I'd prefer to leave it alone since this pr doesn't modify that file and the legacy code will be removed soon.

streamich · 2020-01-08T09:59:31Z

src/plugins/management/public/types.ts

+}
+
+export type ManagementSectionMount = (
+  params: ManagementAppMountParams


From the RFC my understanding was that Management app would use core's context containers, i.e. this function would have initial context argument:

Suggested change

params: ManagementAppMountParams

context, params: ManagementAppMountParams

Here is the RFC example.

And it would be implement like in this example.

But since the RFC context containers were deprecated and platform team said they will replace them by the Binder Service. Not sure what the status is on the binder service, and if there was a discussion to use the binder service here instead. Is it planned to be used here?

Currently I understand the plugins using Management app should instead use core.getStartServices() method to receive the start core and plugins?

Perhaps we should have a conversation with @joshdover about this. I've been confused about the purpose of context containers and I'm therefore uncertain of anything we're discussing in relationship to them.

Let's discuss via slack or zoom since I think we should even up on needs / expectations before talking interfaces.

spoke with @streamich and his questions are mainly regarding what the platform team is providing rather than this implementation.

streamich · 2020-01-08T10:05:23Z

src/plugins/management/public/management_app.tsx

+            getSections={getSections}
+            selectedId={id}
+            legacySections={getLegacyManagementSections().items}
+            mountedCallback={async element => {


Suggested change

mountedCallback={async element => {

onMount={async element => {

Also, maybe instead of handling mount/unmount here, the <ManagementChrome> component could receive the mount function and handle the mount/unmount internally?

<ManagementChrome mount={() => mount({ basePath, element, setBreadcrumbs, })} />

I've left this alone as my attempt at this didn't simplify things. I kind of like keeping ManagementChrome dumb, at least its a very simple separation of concerns.

lizozom · 2020-01-08T10:38:58Z

@streamich about the icon suggestion - I do think that it would be better to have a fallback icon for sections without an icon. @elastic/kibana-design

lizozom

The test plugin is a great addition!

It would have been nice if this PR used ManagementChrome in core_plugins already, but I assume this can be done in a follow up PR.

I also support most of @streamich comments.

Otherwise, tested on Ubuntu \ Chrome and LGTM.

mattkime · 2020-01-08T20:21:42Z

@streamich regarding alignment without an icon - that strikes me as an EUI issue.

cchaos · 2020-01-08T20:38:33Z

regarding alignment without an icon - that strikes me as an EUI issue.

It's not an EUI issue, as we do allow the list to have no icons and therefore needs to be fully left aligned.

I do think that it would be better to have a fallback icon for sections without an icon.

Simply use the empty icon type and the section will align properly without showing a default/random icon.

* implement management new platform api

* master: (23 commits) [Vis: Default editor] Reactify the timelion editor (elastic#52990) [Discover] fix histogram min interval (elastic#53979) [Telemetry] [Monitoring] Only retry fetching usage once monito… (elastic#54309) [docs][APM] Add runtime index config documentation (elastic#53907) [SIEM] Detection engine timeline (elastic#53783) Filter scripted fields preview field list to source fields (elastic#53826) Management - New platform api (elastic#52579) Reset region and Account when switching inventory (elastic#54287) [SIEM] [Case] Case workflow api schema (elastic#51535) Code coverage setup on CI (elastic#49003) [ML] DF Analytics Results: adds link to docs (elastic#54189) Update schemas boolean, byteSize, and duration to coerce strings (elastic#54177) [Metrics UI] Pass relevant shouldAllowEdit capabilities into SettingsPage (elastic#49781) [Canvas] Fixes bugs with autoplay and refresh (elastic#53149) [ML] DF Analytics Classification: ensure confusion matrix can be fetched (elastic#53629) Fix Vega react eslint errors (elastic#54259) Remove non existing codeowners (elastic#54274) use correct type (elastic#54244) [Dashboard] Removing 100% as dshDashboardViewport height (elastic#54263) add `examples/` to no-restricted-path config (elastic#54252) ...

kibanamachine · 2020-01-13T01:30:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 7a48b8b

History

💚 Build #18859 succeeded 7a48b8b
💚 Build #18809 succeeded d3d7a7b
💚 Build #18566 succeeded c795447
💚 Build #18383 succeeded 6cbe2da
💔 Build #18294 failed b76ed14

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

mattkime added 28 commits December 9, 2019 17:02

management new platform api - work in progress

249467d

sidebar pulling items from both registries

0082362

fix import

d24bd6d

restore data-test-subj

9d3ef21

remove getAvailable from setup - capabiltiies unavailable

43ffa97

sidebar nav in new and old management apps, need to wire up old manag…

c47ce1e

…ement app registry to new platform side

Merge branch 'master' into management_new_platform_api

b5825a1

Merge branch 'master' into management_new_platform_api

a1a2b63

Merge branch 'master' into management_new_platform_api

e4cb014

Merge branch 'master' into management_new_platform_api

b42cbae

partial work - loading both legacy and np management apps

b2d1a92

add components dir, forgot with last commit

9da91b7

i18n fix

04b33aa

Merge branch 'master' into management_new_platform_api

60da3ae

minor fixes, don't allow double registration of management section

6ef6072

management app rendering implemented

f8ddba1

type cleanup

54cc094

refactor - create management app class, provide enable/disable methods

50fea18

Merge branch 'master' into management_new_platform_api

9d8457b

remove capabilities api integration

bf31abd

remove unneeded types

fc9b1e3

implement breadcrumbs for management apps

360eb34

legacyManagementAdapter => LegacyManagementAdapter

cf30a01

initial navigateToApp functionality

7b1499f

support for icon url, still need to import scss content

d7bbafd

support for icon url, still need to import scss content

dfb2eb3

Merge branch 'master' into management_new_platform_api

f1bde42

css fix

80d4f46

elastic deleted a comment from elasticmachine Dec 21, 2019

mattkime added 4 commits January 6, 2020 21:15

Merge branch 'master' into management_new_platform_api

fd6bed1

remove test code

e67a115

fix ts, add test plugin

22c721c

css reference cleanup

b76ed14

mattkime requested a review from a team as a code owner January 7, 2020 05:16

legrego reviewed Jan 7, 2020

View reviewed changes

mattkime added 2 commits January 7, 2020 07:46

Merge branch 'master' into management_new_platform_api

e4f1181

snapshot update

6cbe2da

legrego approved these changes Jan 7, 2020

View reviewed changes

ryankeairns approved these changes Jan 7, 2020

View reviewed changes

streamich self-requested a review January 7, 2020 16:45

mattkime added 2 commits January 7, 2020 15:46

add plugin functional test

cd83753

add sidebar nav test

c795447

streamich approved these changes Jan 8, 2020

View reviewed changes

lizozom approved these changes Jan 8, 2020

View reviewed changes

components to own directories, type improvements

d3d7a7b

provide empty icon if none provided

7a48b8b

mattkime requested a review from a team as a code owner January 8, 2020 21:55

mattkime merged commit 9282f19 into elastic:master Jan 8, 2020

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

Management - New platform api (elastic#52579)

fab02d4

* implement management new platform api

mattkime mentioned this pull request Jan 9, 2020

[7.x] Management - New platform api (#52579) #54321

Merged

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

Management - New platform api (#52579) (#54321)

7479e2b

* implement management new platform api

mattkime added the release_note:enhancement label Jan 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Management - New platform api #52579

Management - New platform api #52579

mattkime commented Dec 9, 2019 •

edited

Loading

kibanamachine commented Dec 21, 2019

legrego Jan 7, 2020

mattkime Jan 7, 2020

legrego left a comment

ryankeairns left a comment

streamich left a comment

streamich Jan 8, 2020

streamich Jan 8, 2020

streamich Jan 8, 2020

streamich Jan 8, 2020

streamich Jan 8, 2020

streamich Jan 8, 2020

streamich Jan 8, 2020

streamich Jan 8, 2020

mattkime Jan 8, 2020

streamich Jan 8, 2020 •

edited

Loading

mattkime Jan 8, 2020

mattkime Jan 8, 2020

streamich Jan 8, 2020

streamich Jan 8, 2020

mattkime Jan 8, 2020

lizozom commented Jan 8, 2020

lizozom left a comment

mattkime commented Jan 8, 2020

cchaos commented Jan 8, 2020

kibanamachine commented Jan 13, 2020

	public start = (navigateToApp: CoreStart['application']['navigateToApp']) => ({
	public start(navigateToApp: CoreStart['application']['navigateToApp']) => ({


		const sectionVisible = (section: LegacySection \| LegacyApp) => !section.disabled && section.visible;

		export const sideNavItems = (sections: ManagementSection[], selectedId: string) =>

	export const sideNavItems = (sections: ManagementSection[], selectedId: string) =>
	const sideNavItems = (sections: ManagementSection[], selectedId: string) =>

		@@ -22,7 +22,7 @@ import { IndexedArray } from '../../../../legacy/ui/public/indexed_array';

		const listeners = [];

	params: ManagementAppMountParams
	context, params: ManagementAppMountParams

	mountedCallback={async element => {
	onMount={async element => {

Management - New platform api #52579

Management - New platform api #52579

Conversation

mattkime commented Dec 9, 2019 • edited Loading

Summary

Checklist

For maintainers

Dev Docs

Management API for Kibana Platform

kibanamachine commented Dec 21, 2019

💔 Build Failed

History

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego left a comment

Choose a reason for hiding this comment

ryankeairns left a comment

Choose a reason for hiding this comment

streamich 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

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

streamich Jan 8, 2020 • edited Loading

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

Choose a reason for hiding this comment

lizozom commented Jan 8, 2020

lizozom left a comment

Choose a reason for hiding this comment

mattkime commented Jan 8, 2020

cchaos commented Jan 8, 2020

kibanamachine commented Jan 13, 2020

💚 Build Succeeded

History

mattkime commented Dec 9, 2019 •

edited

Loading

streamich Jan 8, 2020 •

edited

Loading