Migrate SO management section to NP #61700

pgayvallet · 2020-03-27T20:27:26Z

Summary

Last part of the #50308 'epic'. This moves the SO management section from the legacy kibana plugin in src/legacy/core_plugins/kibana/public/management/sections to a new savedObjectsManagement platform plugin.

I tried to reduce this PR's size with the preliminary PRs I did last weeks, however this one is still quite big. I can't really split that any more, so this last part will have to be reviewed in a single shot unfortunately.

Most UI components were migrated in the simplest way, by converting them to TS and using NP APIs instead of legacy imports. Code cleanup, refactoring and type fixing should be considered as outside of the scope of this PR (as much as possible), as they should be addressed in the #62048 follow-up instead.

Plugin bundle size:

before optimization: 4mb.
after async require for the management section and removing static import from discover plugin: 50kb.

Notes for reviewers

security

I think only owned files impacting changes are on copy_saved_objects_to_space_service.ts, and are trivial.

app-arch

All src/legacy/core_plugins/kibana/public/management/sections/objects files got moved to the new SOM plugin. This changes their ownership from your team to ours, however a 'full' review is still very welcome.

platform

See self-review

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

…gement-final-countdown

elasticmachine · 2020-03-27T20:37:03Z

Pinging @elastic/kibana-platform (Team:Platform)

…gement-final-countdown

pgayvallet · 2020-04-06T07:30:53Z

src/plugins/discover/public/plugin.ts

+  savedSearches: {
+    /**
+     * Create a {@link SavedObjectLoader | loader} to handle the saved searches type.
+     * @param services
+     */
+    createLoader(services: SavedObjectKibanaServices): SavedObjectLoader;
+  };


While trying to reduce the initial bundle size, I saw that the async import of the management section mount I added was only reducing the SOM bundle size from 4mb to 3.7mb. After a few checks, the cause appeared to be the createSavedSearchesLoader static import I'm using in the SOM plugin, that caused the full discover plugin + deps to be bundled into the SOM plugin.

After exposing this method from the contract instead, the SOM plugin is no longer using non-type imports from any plugin, and the initial bundle size went from 3.8mb to 51kb.

cc @elastic/kibana-app (btw it seems the discover NP plugin is not present in CODEOWNERS)

Thanks for pinging about the codeowners, I will set up a PR for this.

flash1293

Changes in discover plugin LGTM, code review only.

flash1293 · 2020-04-06T16:18:32Z

@pgayvallet There is already a plugin saved_objects providing some functionality around saved objects - should this be merged with saved_objects_management?

pgayvallet · 2020-04-07T08:10:19Z

@flash1293 This was an option, but after some discussion, we decided (at least for now) to create a distinct plugin.

Main reasons would be better isolation/ per plugin responsibilities, and the fact that atm, the SOM plugin is depending on plugins exposing their SO loaders, and that we didn't want to propagate these dependencies to the SO plugin, as that would cause cyclic dependencies (namely in the dashboard plugin that requires the savedObject plugin)

We have a follow-up issue (#62048) to get rid of the loaders usages in the SOM plugin that would technically allow to merge SO and SOM at some point though.

Happy to discuss it further if you have an opinion on the subject.

…gement-final-countdown

pgayvallet · 2020-04-08T08:51:20Z

@flash1293 just merged master, the new discover plugin added to CODEOWNER triggered a review from kibana-app (TIL as your initial approval was done before the CODEOWNER update landed in the PR, it's not taken into account), however there are no additional changes.

flash1293

Still LGTM :) Not even code review

mshustov · 2020-04-08T14:12:37Z

ack: reviewing...

mshustov · 2020-04-08T14:28:36Z

src/plugins/saved_objects_management/public/index.ts

@@ -22,10 +22,15 @@ import { SavedObjectsManagementPlugin } from './plugin';

 export { SavedObjectsManagementPluginSetup, SavedObjectsManagementPluginStart } from './plugin';
 export {
-  ISavedObjectsManagementActionRegistry,
+  SavedObjectsManagementActionService,


don't see any usages outside of the plugin

mshustov · 2020-04-08T14:43:08Z

src/plugins/saved_objects_management/public/plugin.ts

+    // depends on `getStartServices`, should not be awaited
+    registerServices(this.serviceRegistry, core.getStartServices);


Is it a result of the poor implementation ? as per https://github.com/elastic/kibana/pull/61700/files#r401190691

mshustov · 2020-04-08T14:44:10Z

src/plugins/saved_objects_management/public/plugin.ts

    return {
-      actionRegistry: this.actionRegistry,
+      actions: actionSetup,
+      serviceRegistry: this.serviceRegistry,


+1 to remove unused API

mshustov · 2020-04-08T14:46:26Z

src/plugins/saved_objects_management/public/plugin.ts

@@ -65,12 +86,27 @@ export class SavedObjectsManagementPlugin
      category: FeatureCatalogueCategory.ADMIN,
    });

+    const actionSetup = this.actionService.setup();
+
+    registerManagementSection({


optional: why not to keep this logic in the plugin? it would make it possible to assess dependencies from the first glance.

mshustov · 2020-04-08T14:50:49Z

src/plugins/saved_objects_management/public/management_section/mount_section.tsx

+}: MountParams) => {
+  const [coreStart, { data }, pluginStart] = await core.getStartServices();
+  const { element, basePath, setBreadcrumbs } = mountParams;
+  const allowedTypes = await getAllowedTypes(coreStart.http);


Is it okay to fetch types on every mount?

Low impact, but obviously unnecessary, as these will never change during the app lifecycle. Will fix to only perform the http call on initial mount.

mshustov · 2020-04-08T15:33:30Z

...ins/saved_objects_management/public/management_section/objects_table/saved_objects_table.tsx

+}
+
+export class SavedObjectsTable extends Component<SavedObjectsTableProps, SavedObjectsTableState> {
+  private _isMounted = false;


🙈 on that one. This is there because debouncedFetch is performing debounced http calls followed by setState calls (which will raise an error when called after unmount). Removing that would require to be able to interrupt the http requests on unmount, which is indeed better and technically possible with NP apis but way more complicated than this ugly manual mount stats property (which is indeed an anti-pattern)...

mshustov · 2020-04-08T15:36:44Z

...ins/saved_objects_management/public/management_section/objects_table/saved_objects_table.tsx

@@ -124,21 +158,20 @@ export class ObjectsTable extends Component {
  }

  fetchCounts = async () => {
+    const { allowedTypes } = this.props;
    const { queryText, visibleTypes } = parseQuery(this.state.activeQuery);


can be typed now when we have usage examples? or you'd prefer to do it in a follow-up?

Added proper typing to parseQuery param & return type

mshustov · 2020-04-08T15:46:17Z

src/plugins/saved_objects_management/public/lib/resolve_import_errors.ts

+  return http.post<any>('/api/saved_objects/_resolve_import_errors', {
    headers: {
      // Important to be undefined, it forces proper headers to be set for FormData
      'Content-Type': undefined,


I'd say it shouldn't be necessary, but I can see

kibana/src/core/public/http/fetch.ts

Line 131 in 3ec71c3

'Content-Type': 'application/json',

mshustov · 2020-04-08T15:50:06Z

src/plugins/saved_objects_management/public/lib/resolve_import_errors.ts

@@ -27,13 +27,11 @@ interface RetryObject {
  replaceReferences?: any[];
 }

-async function callResolveImportErrorsApi(file: File, retries: any) {
+async function callResolveImportErrorsApi(http: HttpStart, file: File, retries: any) {


I wish we split the logic in this file...

Yea. Honestly the whole src/plugins/saved_objects_management/public/lib utilities are a mess, with honourable mentions for this file and resolve_saved_objects. I want to avoid as must as possible to modify them until I got a clearer vision on what needs to be done to get rid of the SavedObjectLoader usages (in #62048), as this will impact a lot of the lib files.

mshustov · 2020-04-08T15:57:26Z

...s/saved_objects_management/public/management_section/objects_table/components/table.test.tsx

 import { findTestSubject } from '@elastic/eui/lib/test';
+// @ts-ignore


this API is not meant to be used as public ?

Though I did try to import from @elastic/eui, but apparently not, as keyCodes is actually exported from the root package.

changed to import { keyCodes } from '@elastic/eui';

findTestSubject however is declared as also available from '@elastic/eui'; in the definition file, but is actually not exported... If I try to import { findTestSubject } from '@elastic/eui';, TS compiler does not complain, however at runtime I get:

TypeError: (0 , _eui.findTestSubject) is not a function

So I still need the ts-ignore on import { findTestSubject } from '@elastic/eui/lib/test';

Ping @elastic/kibana-design ^

…gement-final-countdown

kibanamachine · 2020-04-13T10:20:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1583d98

History

💚 Build #39805 succeeded 14420a8
💚 Build #39528 succeeded 767c993
💚 Build #38735 succeeded 43a9d45
💚 Build #38343 succeeded fad42bc
💚 Build #37647 succeeded 857c684

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

* move libs to new plugin * adapt libs to use NP apis * add required plugins * add get_allowed_types route * move object_view components * add service registry * migrate table header component * migrate table component * migrate saved_objects_table component * remove migrated legacy files * fix re-export from legacy management + section label * migrate services registration * adapt management section mock * fix imports * migrate flyout component * migrate relationships component * migrate saved_objects_table tests * migrate breadcrumb * add redirect if unauthorized check * migrate translations to new savedObjectsManagement prefix * remove obsolete translations * convert action registry to service pattern * wire extra actions * remove importAndExportableTypes from injected vars * handle newIndexPatternUrl * remove duplicate dashboard dependency * remove old TODO * remove old TODO * properly mock lodash in tests * add async management section loading * expose createSavedSearchesLoader from discover plugin contract * address most review comments * fix merge conflicts

* Migrate SO management section to NP (#61700) * move libs to new plugin * adapt libs to use NP apis * add required plugins * add get_allowed_types route * move object_view components * add service registry * migrate table header component * migrate table component * migrate saved_objects_table component * remove migrated legacy files * fix re-export from legacy management + section label * migrate services registration * adapt management section mock * fix imports * migrate flyout component * migrate relationships component * migrate saved_objects_table tests * migrate breadcrumb * add redirect if unauthorized check * migrate translations to new savedObjectsManagement prefix * remove obsolete translations * convert action registry to service pattern * wire extra actions * remove importAndExportableTypes from injected vars * handle newIndexPatternUrl * remove duplicate dashboard dependency * remove old TODO * remove old TODO * properly mock lodash in tests * add async management section loading * expose createSavedSearchesLoader from discover plugin contract * address most review comments * fix merge conflicts * fix types for 7.x

pgayvallet added 14 commits March 27, 2020 07:58

move libs to new plugin

9bf165a

adapt libs to use NP apis

3236ac6

add required plugins

e065f27

add get_allowed_types route

53ba5e3

move object_view components

9ff9026

add service registry

8de9be6

migrate table header component

d45a77b

migrate table component

b7f3f3d

migrate saved_objects_table component

0781cde

remove migrated legacy files

2d423b5

fix re-export from legacy management + section label

4635130

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

101380a

…gement-final-countdown

migrate services registration

83d270c

adapt management section mock

fbc8931

rudolf mentioned this pull request Mar 27, 2020

[Platform] SavedObject Management #50308

Closed

5 tasks

fix imports

0a8949b

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes v7.8 v8.0.0 Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Mar 27, 2020

pgayvallet added this to Pending Review in kibana-core [DEPRECATED] via automation Mar 27, 2020

pgayvallet moved this from Pending Review to In Progress in kibana-core [DEPRECATED] Mar 27, 2020

pgayvallet added 5 commits March 30, 2020 11:18

migrate flyout component

7d9d6f9

migrate relationships component

ccf3112

migrate saved_objects_table tests

dfbec16

migrate breadcrumb

9be3b46

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

50ccb8e

…gement-final-countdown

botelastic bot added the Team:AppArch label Mar 30, 2020

pgayvallet added 4 commits April 3, 2020 10:17

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

fad42bc

…gement-final-countdown

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

5a0d083

…gement-final-countdown

add async management section loading

1d810f0

expose createSavedSearchesLoader from discover plugin contract

43a9d45

pgayvallet commented Apr 6, 2020

View reviewed changes

flash1293 approved these changes Apr 6, 2020

View reviewed changes

rayafratkina added v7.8.0 and removed v7.8 labels Apr 6, 2020

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

767c993

…gement-final-countdown

pgayvallet requested a review from a team April 8, 2020 08:46

flash1293 approved these changes Apr 8, 2020

View reviewed changes

mshustov approved these changes Apr 8, 2020

View reviewed changes

streamich approved these changes Apr 9, 2020

View reviewed changes

pgayvallet added 4 commits April 9, 2020 09:28

address most review comments

9d94d9a

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

06b1675

…gement-final-countdown

fix merge conflicts

14420a8

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

1583d98

…gement-final-countdown

pgayvallet merged commit 358d139 into elastic:master Apr 13, 2020

kibana-core [DEPRECATED] automation moved this from Pending Review to Done (7.8) Apr 13, 2020

pgayvallet mentioned this pull request Apr 13, 2020

[7.x] Migrate SO management section to NP (#61700) #63334

Merged

markov00 mentioned this pull request May 14, 2020

Editing a saved object from Management UI can corrupt the saved object #66542

Closed

pgayvallet mentioned this pull request May 26, 2020

Kibana i18n Setting Leads to Unknown saved object type Warning #52165

Closed

stefnestor mentioned this pull request Mar 3, 2022

[Doc] Kibana Saved Objects Allowed Types #126860

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate SO management section to NP #61700

Migrate SO management section to NP #61700

pgayvallet commented Mar 27, 2020 •

edited

Loading

elasticmachine commented Mar 27, 2020

pgayvallet Apr 6, 2020 •

edited

Loading

flash1293 Apr 6, 2020

flash1293 left a comment

flash1293 commented Apr 6, 2020

pgayvallet commented Apr 7, 2020

pgayvallet commented Apr 8, 2020

flash1293 left a comment

mshustov commented Apr 8, 2020

mshustov Apr 8, 2020

mshustov Apr 8, 2020

mshustov Apr 8, 2020

mshustov Apr 8, 2020

mshustov Apr 8, 2020

pgayvallet Apr 9, 2020

mshustov Apr 8, 2020

pgayvallet Apr 9, 2020

mshustov Apr 8, 2020

pgayvallet Apr 9, 2020

mshustov Apr 8, 2020

mshustov Apr 8, 2020

pgayvallet Apr 9, 2020

mshustov Apr 8, 2020

pgayvallet Apr 9, 2020

kibanamachine commented Apr 13, 2020

		// depends on `getStartServices`, should not be awaited
		registerServices(this.serviceRegistry, core.getStartServices);

		import { findTestSubject } from '@elastic/eui/lib/test';
		// @ts-ignore

Migrate SO management section to NP #61700

Migrate SO management section to NP #61700

Conversation

pgayvallet commented Mar 27, 2020 • edited Loading

Summary

Notes for reviewers

security

app-arch

platform

Checklist

elasticmachine commented Mar 27, 2020

pgayvallet Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 commented Apr 6, 2020

pgayvallet commented Apr 7, 2020

pgayvallet commented Apr 8, 2020

flash1293 left a comment

Choose a reason for hiding this comment

mshustov commented Apr 8, 2020

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

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

kibanamachine commented Apr 13, 2020

💚 Build Succeeded

History

pgayvallet commented Mar 27, 2020 •

edited

Loading

pgayvallet Apr 6, 2020 •

edited

Loading