[Index management] Client-side NP ready #57295

sebelga · 2020-02-11T10:10:19Z

This PR adds the client-side migration of index management to the new platform.

As there are many changes and 3 dependent apps, I added both of you @cjcenizal and @alisonelizabeth to review this PR and make sure all the extensions keep on working.

The 3 app that have extensions are:

rollup
ILM
Cross cluster replication

ILM summary tab extension

ILM actions menu extension

ILM filters extensions

CCR badge extension

elasticmachine · 2020-02-11T10:15:46Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

alisonelizabeth · 2020-02-12T19:13:15Z

...ck/legacy/plugins/index_management/__jest__/client_integration/helpers/setup_environment.tsx

+import { setExtensionsService } from '../../../public/application/store/selectors';
+
+/* eslint-disable @kbn/eslint/no-restricted-paths */
+import { notificationServiceMock } from '../../../../../../../src/core/public/notifications/notifications_service.mock';


nit: all core mocks should be available via src/core/public/mocks

alisonelizabeth · 2020-02-12T20:49:32Z

...ck/legacy/plugins/index_management/__jest__/client_integration/helpers/setup_environment.tsx

+  httpService.setup(mockHttpClient);
+  breadcrumbService.setup(() => undefined);
+  documentationService.setup(docLinksServiceMock.createStartContract());
+  notificationService.setup(notificationServiceMock.createStartContract());


should this be notificationServiceMock.createSetupContract()?

They are both the same, but for consistency let's use the createSetupContract () 👍

…x-management-public-2

sebelga · 2020-02-13T05:54:08Z

Thanks for the review @alisonelizabeth ! I uncommented the tests and they pass now. Mistery of life! 😄 I address your feedback and made a comment about not using axios.

cjcenizal

Great work @sebelga! I tested ILM, Rollups, CCR, and UI metric telemetry and everything worked as expected. Here's a screenshot verifying CCR follower indices had the correct badge:

And rollup indices:

I do feel pretty strongly that we're adding friction to the DX by renaming init to setup in our services, but I don't think it's worth blocking on because it would be simple to change later.

cjcenizal · 2020-02-13T05:26:48Z

...nt/public/np_ready/application/sections/policy_table/components/policy_table/policy_table.js

-import { BASE_PATH } from '../../../../../../../common/constants';
-import { UIM_EDIT_CLICK } from '../../../../constants';
+import { getIndexListUri } from '../../../../../../../../index_management/public/application/services/navigation';
+import { BASE_PATH, UIM_EDIT_CLICK } from '../../../../../../../common/constants';


Why did UIM_EDIT_CLICK get moved to common? If these constants aren't being consumed by both public and server code, then I think they belong in public.

This is probably from the merge conflict resolution. There are no UIM_EDIT_CLICK in the common folder so importing it is probably undefined.. Will revert that change. 👍

cjcenizal · 2020-02-13T05:33:13Z

x-pack/legacy/plugins/index_management/public/application/services/api.ts

+import { IndexMgmtMetricsType } from '../../types';
+
+// Temporary hack to provide the uiMetricService instance to this file.
+// TODO: Refactor and export an ApiService instance through the app dependencies context


When do you plan to address this TODO?

cjcenizal · 2020-02-13T05:34:23Z

x-pack/legacy/plugins/index_management/public/application/store/selectors/index.js

-import { getToggleExtensions } from '../../../index_management_extensions';
+
+// Temporary hack to provide the extensionsService instance to this file.
+// TODO: Refactor and export all the app selectors through the app dependencies context


I'm happy to see this note here because it did surprise me to see this function available from this module. When do you plan to address this TODO?

cjcenizal · 2020-02-13T05:41:48Z

...ns/index_management/public/application/sections/home/index_list/detail_panel/detail_panel.js

+import { ShowJson } from './show_json';
+import { Summary } from './summary';
+import { EditSettingsJson } from './edit_settings_json';
+import { useServices } from '../../../../app_context';


Nit: Personally I find grouping imports by how distant they are from the file makes it easier to reason about relationships between the imports and the file importing them. For example, I know ShowJson, Summary, and EditSettingsJson are local to this file so I expect to be able to refactor them more easily because they probably aren't meant for external consumptions. So I'd put this import on line 32.

I also like that structure of ordering importing and I've already updated the imports order whenever I bump into them. I've seen in places in our code absolute path coming after relative ones.

cjcenizal · 2020-02-13T05:52:02Z

x-pack/legacy/plugins/index_management/public/application/services/breadcrumbs.ts

-  public init(chrome: ChromeStart, managementBreadcrumb: any): void {
-    this.chrome = chrome;
-    this.breadcrumbs.management = [managementBreadcrumb];
+  public setup(setBreadcrumbsHandler: SetBreadcrumbs): void {


What is the benefit of using the name setup in these services? I'm finding it confusing because one core aspect of the NP mental model is that of the lifecycle, defined by setup, start, and stop. These terms are associated with plugins and how they work, but as far as I can tell these services have no knowledge of the plugin lifecycle. Is there an instance in which we'd need to start or stop a service?

If not, then I think we should avoid using lifecycle terms. This will avoid overloading them, allowing us to grep for calls to setup( with confidence that any hits will be related to the NP lifecycle.

So this change is to align on terms and simplify the mental model. Those setup function are meant to be called.... at plugin setup() lifecycle time. And this method does that: set up the service, passing down any required dependencies to the service.

We could call it: "init", "register", "setup", " it does not really matter. But I like to have 1 standard and stick to it.

These terms are associated with plugins and how they work

If you look at core services, like HttpService, you will see it is not only plugins but also services that align on the setup, start and stop public method.

Is there an instance in which we'd need to start or stop a service?

Well, your question made me see that we might have a memory leak with the current License.ts implementation.

// this is the logic inside its setup() function licensing.license$.subscribe(license => { const { state, message } = license.check(pluginId, minimumLicenseType); const hasRequiredLicense = state === LICENSE_CHECK_STATE.Valid; if (hasRequiredLicense) { this.licenseStatus = { isValid: true }; } else { this.licenseStatus = { isValid: false, message: message || defaultErrorMessage, }; if (message) { logger.info(message); } } });

We might have to unsubscribe when the plugin is stopped (and call the stop() method on this service. I will check

…x-management-public-2

sebelga · 2020-02-13T08:11:32Z

Thanks for the review @cjcenizal ! I reverted the change for the constant import.

Regarding the init vs setup, if you feel strongly we'll revert. I do like to have a convention in place and not have different function names for the same concept (.register() .init() .initialize() .initService()). So my point was: let's use the core team term setup() which defines very well the setting up of a service.

cjcenizal · 2020-02-14T02:27:13Z

@sebelga It's your call! I'm happy with whatever you think makes sense.

sebelga · 2020-02-14T04:32:15Z

@elasticmachine merge upstream

mattkime

Approve management changes

sebelga · 2020-02-14T07:23:44Z

Thanks for the review @mattkime !

joshdover · 2020-02-14T17:19:27Z

Regarding the init vs setup, if you feel strongly we'll revert. I do like to have a convention in place and not have different function names for the same concept (.register() .init() .initialize() .initService()). So my point was: let's use the core team term setup() which defines very well the setting up of a service.

This is a great change! We find that it makes plugins easier to reason about if they follow the same convention as Core. One of the largest plugins, the data plugin follows this convention as well. This also makes it easier to restructure your plugin later if they all follow the same general lifecycles and which services are available during each lifecycle.

joshdover

The general structure here looks great!

joshdover · 2020-02-14T17:22:26Z

x-pack/legacy/plugins/index_management/public/application/index.tsx

+  const { Context: I18nContext } = i18n;
+  const { services } = dependencies;
+
+  render(


Does this application use a router? If so it needs to be configured with appBasePath to ensure it works with the basePath correctly. Once #56705 is merged, you will need to use the history instance that is provided with your react-router.

Setting the basename on the react router does not work as we already add the base path manually on each route like so

<Switch> <Route exact path={`${BASE_PATH}create_template`} component={TemplateCreate} /> <Route exact path={`${BASE_PATH}clone_template/:name*`} component={TemplateClone} /> <Route exact path={`${BASE_PATH}edit_template/:name*`} component={TemplateEdit} /> <Route path={`${BASE_PATH}:section(indices|templates)`} component={IndexManagementHome} /> <Redirect from={`${BASE_PATH}`} to={`${BASE_PATH}indices`} /> </Switch>

Do you recommend to refactor and use the basename prop on the router?

…x-management-public-2

kibanamachine · 2020-02-17T06:04:07Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: a32ac8d
Flaky suites:
- xpack-kibana-ciGroup10

History

💚 Build #26730 succeeded aed1ec3
💛 Build #26428 was flaky eb306e1
💔 Build #26417 failed 5ea0f5c
💚 Build #26133 succeeded 845a666
💔 Build #26110 failed 0e9427d

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

…re/files-and-filetree * 'master' of github.com:elastic/kibana: (139 commits) Move Ace XJSON lexer-rules, worker and utils to es_ui_shared (elastic#57563) [Upgrade Assistant] Fix filter deprecations search filter (elastic#57541) [ML] New Platform server shim: update indices routes (elastic#57685) Bump redux dependencies (elastic#53348) [Index management] Client-side NP ready (elastic#57295) change id of x-pack event_log plugin to eventLog (elastic#57612) [eventLog] get kibana.index name from config instead of hard-coding it (elastic#57607) revert allow using any path to generate fixes ui titles (elastic#57535) Fix login redirect for expired sessions (elastic#57157) Expose Vis on the contract as it requires visTypes (elastic#56968) [SIEM][Detection Engine] Fixes queries to ignore errors when signals index is not present [Remote clusters] Migrate client-side code out of legacy (elastic#57365) Fix failed test reporter for SIEM Cypress use (elastic#57240) skip flaky suite (elastic#45244) update chromedriver to 80.0.1 (elastic#57602) change slack action to only report on whitelisted host name (elastic#57582) [kbn/optimizer] throw errors into stream on invalid completion (elastic#57735) moving visualize/utils to new platform (elastic#56650) ...

sebelga added 22 commits February 11, 2020 12:21

Move app to "application" folder

13fecc7

Remove legacy dependencies

a566571

Refactor UiMetricService to follow np setup() pattern

f7b41a0

Add unmount() handler

e249d54

Provide UiMetricService through app context

6773b15

Use uiMetricService instance from setup() in store reducer

78af8fc

Create class for IndexManagementExtensions

0985aa3

Refactor DetailPanel to functional component

b0b2258

Provide extensions service to renderBadges()

77a2700

Refactor naming to ExtensionsService

b786f42

Expose and consume ExtensionsService through app context

9a0071b

Rename extensions service file name

7d65680

Align on "setup()" name to initialize services

9b9f740

Fix store dependencies

53c2fe3

Fix tests for index table

dc1ee41

Fix component integration tests

172bccb

Refactor to use ExtensionsService from other plugins

f1629fa

Remove unnecessary "register_routes" file

98333eb

Provide core and plugins deps to the app dependencies

dd7ac26

Add "Service" suffix to app dependencies

d5ef537

Fix service dependency name

91e3f83

Export "extensionsService" instead of "extensions"

23a210c

sebelga added Feature:Index Management Index and index templates UI Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.7.0 v8.0.0 labels Feb 11, 2020

sebelga requested review from cjcenizal and alisonelizabeth February 11, 2020 10:15

Fix tests app dependencies

0303cc5

alisonelizabeth reviewed Feb 12, 2020

View reviewed changes

sebelga added 3 commits February 13, 2020 10:42

Merge remote-tracking branch 'upstream/master' into np-migration/inde…

c927cc1

…x-management-public-2

Reenable commented tests

50836d6

Address CR feedback

5ea0f5c

cjcenizal approved these changes Feb 13, 2020

View reviewed changes

sebelga added 2 commits February 13, 2020 13:19

Revert UIM_EDIT_CLICK import from common constants folder

f495a31

Merge remote-tracking branch 'upstream/master' into np-migration/inde…

eb306e1

…x-management-public-2

cjcenizal requested a review from joshdover February 13, 2020 08:28

Merge branch 'master' into np-migration/index-management-public-2

aed1ec3

mattkime approved these changes Feb 14, 2020

View reviewed changes

joshdover approved these changes Feb 14, 2020

View reviewed changes

sebelga added 2 commits February 15, 2020 12:02

Merge branch 'master' into np-migration/index-management-public-2

0cef518

Merge remote-tracking branch 'upstream/master' into np-migration/inde…

a32ac8d

…x-management-public-2

sebelga merged commit 8e17fda into elastic:master Feb 17, 2020

sebelga deleted the np-migration/index-management-public-2 branch February 17, 2020 08:52

sebelga mentioned this pull request Feb 20, 2020

[7.x] [Index management] Client-side NP ready (#57295) #58090

Merged

sebelga added a commit that referenced this pull request Feb 20, 2020

[Index management] Client-side NP ready (#57295) (#58090)

faa22e9

This was referenced Mar 6, 2020

[Index management] Client-side NP ready #57186

Closed

License service: possible memory leak #59525

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Index management] Client-side NP ready #57295

[Index management] Client-side NP ready #57295

sebelga commented Feb 11, 2020 •

edited

Loading

elasticmachine commented Feb 11, 2020

alisonelizabeth Feb 12, 2020

alisonelizabeth Feb 12, 2020

sebelga Feb 13, 2020

sebelga commented Feb 13, 2020

cjcenizal left a comment •

edited

Loading

cjcenizal Feb 13, 2020 •

edited

Loading

sebelga Feb 13, 2020

cjcenizal Feb 13, 2020

cjcenizal Feb 13, 2020

cjcenizal Feb 13, 2020

sebelga Feb 13, 2020

cjcenizal Feb 13, 2020

sebelga Feb 13, 2020

sebelga Feb 14, 2020

sebelga commented Feb 13, 2020

cjcenizal commented Feb 14, 2020

sebelga commented Feb 14, 2020

mattkime left a comment

sebelga commented Feb 14, 2020

joshdover commented Feb 14, 2020

joshdover left a comment

joshdover Feb 14, 2020

sebelga Feb 15, 2020

kibanamachine commented Feb 17, 2020

[Index management] Client-side NP ready #57295

[Index management] Client-side NP ready #57295

Conversation

sebelga commented Feb 11, 2020 • edited Loading

ILM summary tab extension

ILM actions menu extension

ILM filters extensions

CCR badge extension

elasticmachine commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga commented Feb 13, 2020

cjcenizal left a comment • edited Loading

Choose a reason for hiding this comment

cjcenizal Feb 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga commented Feb 13, 2020

cjcenizal commented Feb 14, 2020

sebelga commented Feb 14, 2020

mattkime left a comment

Choose a reason for hiding this comment

sebelga commented Feb 14, 2020

joshdover commented Feb 14, 2020

joshdover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Feb 17, 2020

💛 Build succeeded, but was flaky

History

sebelga commented Feb 11, 2020 •

edited

Loading

cjcenizal left a comment •

edited

Loading

cjcenizal Feb 13, 2020 •

edited

Loading