Migrate SO management routes to new plugin #59734

pgayvallet · 2020-03-10T10:39:27Z

Summary

Part of #50308

Migrate the savedObject management routes to a new savedObjectsManagement plugin

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

elasticmachine · 2020-03-10T10:47:40Z

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

…gement-server-side

pgayvallet · 2020-03-11T12:42:51Z

retest

…gement-server-side

pgayvallet · 2020-03-16T11:39:25Z

src/plugins/so_management/server/routes/scroll_export.ts

+export const registerScrollForExportRoute = (router: IRouter) => {
+  router.post(
+    {
+      path: '/api/kibana/management/saved_objects/scroll/export',
+      validate: {
+        body: schema.object({
+          typesToInclude: schema.arrayOf(schema.string()),
+        }),
+      },
+    },


So, this route seems unused. At least I didn't find any usage in our code. Was not sure if I should remove it though.

another good candidate for the platform telemetry project🤔

pgayvallet · 2020-03-16T11:40:31Z

src/plugins/so_management/server/plugin.ts

+    return {
+      management: managementService,
+    };


I'm exposing that from the start contract even if currently no plugin uses it.

I'd rather keep it private until requested to keep the API surface smaller.

pgayvallet · 2020-03-16T11:43:37Z

src/plugins/so_management/server/lib/find_all.ts

+export const findAll = async (
+  client: SavedObjectsClientContract,
+  findOptions: SavedObjectsFindOptions
+): Promise<SavedObject[]> => {
+  return recursiveFind(client, findOptions, 1, []);
+};
+
+const recursiveFind = async (


I added test for them, but appart from that the so_management/server/lib utilities were almost migrated to TS without any changes

pgayvallet · 2020-03-16T11:44:46Z

src/plugins/so_management/server/index.ts

+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under


the plugin name is savedObjectsManagement. Should I rename the plugin folder from so_management to saved_objects_management ?

I would prefer saved_objects_management despite verbosity as more descriptive.

Yea, that's probably the right call, as the folder should probably match the plugin name. Gonna rename

lizozom

Tested and overall works great!
Plugin structure looks good too.

Noticed that changing the types drop down, causes a double fetch of Request URL: http://localhost:5620/api/kibana/management/saved_objects/scroll/counts (one of them seems to fetch only the selected counts, and I don't see why you need to)

lizozom · 2020-03-16T09:12:58Z

src/legacy/core_plugins/kibana/inject_vars.js

@@ -21,11 +21,7 @@ export function injectVars(server) {
  const serverConfig = server.config();

  // Get types that are import and exportable, by default yes unless isImportableAndExportable is set to false


Is this comment still relevant?

Probably not (and it was wrong, as the default value is actually false...). removed. The whole importAndExportableTypes injection is going to be removed in next stage anyway

lizozom · 2020-03-16T11:42:08Z

src/plugins/so_management/server/routes/find.ts

+      path: '/api/kibana/management/saved_objects/_find',
+      validate: {
+        query: schema.object({
+          perPage: schema.number({ min: 0, defaultValue: 20 }),


I think this should be coming from savedObjects:perPage

The savedObjects:perPage value is actually accessed and sent by the client.

kibana/src/legacy/core_plugins/kibana/public/management/sections/objects/_objects.js

Line 60 in 56571ba

perPageConfig={config.get('savedObjects:perPage')}

The route's joi schema was not, and I did a rather straight forward migration to config-schema. As this is an internal API and to avoid adding another dependency to the route definition, I'm leaning to keep it that way, unless you want me to perform the change?

We can do it later when moving savedObjects:perPage registration to saved_objects_management plugin.

pgayvallet · 2020-03-16T12:07:01Z

@lizozom

Noticed that changing the types drop down, causes a double fetch of Request URL: http://localhost:5620/api/kibana/management/saved_objects/scroll/counts (one of them seems to fetch only the selected counts, and I don't see why you need to)

Thanks. will take a look when migration the client-side code in the follow-up PR.

mshustov · 2020-03-17T16:23:00Z

ack: will review tomorrow

…gement-server-side

mshustov

some nits, LGTM

src/legacy/server/saved_objects/saved_objects_mixin.js

mshustov · 2020-03-18T16:25:21Z

src/plugins/so_management/server/index.ts

+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under


I would prefer saved_objects_management despite verbosity as more descriptive.

mshustov · 2020-03-18T16:36:58Z

src/plugins/so_management/server/routes/find.ts

+        .map(so => injectMetaAttributes(so, managementService))
+        .map(obj => {
+          const result = { ...obj, attributes: {} as Record<string, any> };
+          for (const field of includedFields || []) {


unnecessary || []? you have [] as default value

mshustov · 2020-03-18T16:46:03Z

test/api_integration/apis/saved_objects_management/scroll_count.ts

+        dashboard: 1,
+        visualization: 1,
+        'index-pattern': 0,
+        search: 0,


could you add an explicit test that it sets0 for all the types listed in typesToInclude?

mshustov · 2020-03-18T16:48:09Z

src/plugins/so_management/server/routes/scroll_export.ts

+export const registerScrollForExportRoute = (router: IRouter) => {
+  router.post(
+    {
+      path: '/api/kibana/management/saved_objects/scroll/export',
+      validate: {
+        body: schema.object({
+          typesToInclude: schema.arrayOf(schema.string()),
+        }),
+      },
+    },


another good candidate for the platform telemetry project🤔

mshustov · 2020-03-18T17:00:49Z

src/plugins/so_management/server/routes/find.ts

+      path: '/api/kibana/management/saved_objects/_find',
+      validate: {
+        query: schema.object({
+          perPage: schema.number({ min: 0, defaultValue: 20 }),


We can do it later when moving savedObjects:perPage registration to saved_objects_management plugin.

mshustov · 2020-03-18T17:02:52Z

src/plugins/so_management/server/plugin.ts

+    return {
+      management: managementService,
+    };


I'd rather keep it private until requested to keep the API surface smaller.

pgayvallet · 2020-03-19T10:46:08Z

@elastic/kibana-app-arch @lizozom Could we get another look on this one?

…gement-server-side

kibanamachine · 2020-03-26T11:32:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d2fe4b4

History

💚 Build #36433 succeeded d9dc39e
💔 Build #36417 failed 0a2cb33
💚 Build #35291 succeeded c86ee15
💚 Build #34980 succeeded e770084
💔 Build #34857 failed cc9f846

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

* unexpose SavedObjectsManagement from legacy server * migrate saved object management routes to new plugin * fix endpoint methods * adapt code due to rebase * extract types * improve findAll params * adapt existing api integration tests and migrate to TS * update generated doc * add API integration tests for /scroll/count * add unit tests for plugin and routes * add injectMetaAttributes tests * extract relation type * add find_relationships tests * add find_all tests * do not complete migrator$ to avoid unhandled promise rejection * fix data for search endpoint integration tests * remove falsy comment * rename plugin folder to match plugin id * address review comments * update CODEOWNERS

pgayvallet added 4 commits March 10, 2020 11:26

unexpose SavedObjectsManagement from legacy server

db5af58

migrate saved object management routes to new plugin

b0f12e8

fix endpoint methods

1f0e935

adapt code due to rebase

53030ff

rudolf mentioned this pull request Mar 10, 2020

[Platform] SavedObject Management #50308

Closed

5 tasks

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

pgayvallet added 11 commits March 10, 2020 11:48

extract types

8e71b92

improve findAll params

6eaff34

adapt existing api integration tests and migrate to TS

dc9bad9

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

1518159

…gement-server-side

update generated doc

bc352e9

add API integration tests for /scroll/count

3982289

add unit tests for plugin and routes

f350b8c

add injectMetaAttributes tests

07bfe52

extract relation type

5a9d9f1

add find_relationships tests

fd70c8b

add find_all tests

b24b37f

pgayvallet added 3 commits March 11, 2020 15:51

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

ebf3751

…gement-server-side

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

5fc0ced

…gement-server-side

do not complete migrator$ to avoid unhandled promise rejection

23581cd

pgayvallet marked this pull request as ready for review March 16, 2020 08:59

pgayvallet requested a review from a team as a code owner March 16, 2020 08:59

fix data for search endpoint integration tests

9f5df69

pgayvallet commented Mar 16, 2020

View reviewed changes

lizozom reviewed Mar 16, 2020

View reviewed changes

remove falsy comment

b1ef936

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

cb35fdb

…gement-server-side

mshustov approved these changes Mar 18, 2020

View reviewed changes

pgayvallet added 3 commits March 18, 2020 18:30

rename plugin folder to match plugin id

dc4e2bd

address review comments

703bc80

update CODEOWNERS

4ac3e3f

pgayvallet requested a review from lizozom March 19, 2020 10:45

pgayvallet added 3 commits March 20, 2020 13:20

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

cc9f846

…gement-server-side

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

e770084

…gement-server-side

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

c86ee15

…gement-server-side

mattkime approved these changes Mar 23, 2020

View reviewed changes

pgayvallet added 2 commits March 26, 2020 07:40

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

0a2cb33

…gement-server-side

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

d9dc39e

…gement-server-side

pgayvallet added v7.8 and removed v7.7.0 labels Mar 26, 2020

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

d2fe4b4

…gement-server-side

pgayvallet merged commit a7508b8 into elastic:master Mar 26, 2020

pgayvallet mentioned this pull request Mar 26, 2020

[7.x] Migrate SO management routes to new plugin (#59734) #61439

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate SO management routes to new plugin #59734

Migrate SO management routes to new plugin #59734

pgayvallet commented Mar 10, 2020

elasticmachine commented Mar 10, 2020

pgayvallet commented Mar 11, 2020

pgayvallet Mar 16, 2020

mshustov Mar 18, 2020

pgayvallet Mar 16, 2020

mshustov Mar 18, 2020

pgayvallet Mar 16, 2020

pgayvallet Mar 16, 2020

mshustov Mar 18, 2020

pgayvallet Mar 18, 2020

lizozom left a comment

lizozom Mar 16, 2020

pgayvallet Mar 16, 2020

lizozom Mar 16, 2020

pgayvallet Mar 16, 2020

mshustov Mar 18, 2020

pgayvallet commented Mar 16, 2020

mshustov commented Mar 17, 2020

mshustov left a comment

mshustov Mar 18, 2020

mshustov Mar 18, 2020

mshustov Mar 18, 2020

mshustov Mar 18, 2020

mshustov Mar 18, 2020

mshustov Mar 18, 2020

pgayvallet commented Mar 19, 2020

kibanamachine commented Mar 26, 2020

		@@ -21,11 +21,7 @@ export function injectVars(server) {
		const serverConfig = server.config();

		// Get types that are import and exportable, by default yes unless isImportableAndExportable is set to false

Migrate SO management routes to new plugin #59734

Migrate SO management routes to new plugin #59734

Conversation

pgayvallet commented Mar 10, 2020

Summary

Checklist

elasticmachine commented Mar 10, 2020

pgayvallet commented Mar 11, 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

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

Choose a reason for hiding this comment

pgayvallet commented Mar 16, 2020

mshustov commented Mar 17, 2020

mshustov 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

pgayvallet commented Mar 19, 2020

kibanamachine commented Mar 26, 2020

💚 Build Succeeded

History