Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SO import hook / warnings API #87996

Merged
merged 33 commits into from
Jan 20, 2021

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Jan 12, 2021

Summary

Fix #84977

This PR adds hook support to SO imports. Type owners can now register an import hook using the new SavedObjectType.onImport property when registering their type via registerType.

The import hooks currently only support a single feature: returning import warnings that will be displayed to the user when the import succeeds.

API usage example

public setup(core: CoreSetup, plugins: SetupDeps) {
  core.savedObjects.registerType({
    name: 'my_type',
    hidden: false,
    namespaceType: 'single',
    mappings: { /* [...] */ },
    management: {
        importableAndExportable: true,
        onImport: async (objects) => {
          if(await someActionIsNeeded(objects)) {
            return {
              warnings: [
                { type: 'simple', message: 'Just some basic warning about the import' },
                {
                  type: 'action_required',
                  message: 'Alerts have been imported, but are disabled',
                  actionUrl: '/app/dashboards?foo=bar',
                  buttonLabel: 'Enabled here',
                },
              ],
            };
          }
          return {};
      },
      // [...]
    },
  });

  return {};
}

Screenshots

Screenshot 2021-01-13 at 21 56 00

How to test

I personally used this diff to add warnings to dashboards during testing
Index: src/plugins/dashboard/server/plugin.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/plugins/dashboard/server/plugin.ts b/src/plugins/dashboard/server/plugin.ts
--- a/src/plugins/dashboard/server/plugin.ts	(revision 0ba9aea5f2aa9b7afe885a2bd364610656db8376)
+++ b/src/plugins/dashboard/server/plugin.ts	(date 1610725322281)
@@ -49,13 +49,30 @@
   public setup(core: CoreSetup, plugins: SetupDeps) {
     this.logger.debug('dashboard: Setup');
 
-    core.savedObjects.registerType(
-      createDashboardSavedObjectType({
-        migrationDeps: {
-          embeddable: plugins.embeddable,
+    const dashboardType = createDashboardSavedObjectType({
+      migrationDeps: {
+        embeddable: plugins.embeddable,
+      },
+    });
+    core.savedObjects.registerType({
+      ...dashboardType,
+      management: {
+        ...dashboardType.management,
+        onImport: (dashboards) => {
+          return {
+            warnings: [
+              { type: 'simple', message: 'Just some basic warning about the import' },
+              {
+                type: 'action_required',
+                message: 'Alerts have been imported, but are disabled',
+                actionPath: '/app/dashboards?foo=bar',
+                buttonLabel: 'Enable here',
+              },
+            ],
+          };
         },
-      })
-    );
+      },
+    });
     core.capabilities.registerProvider(capabilitiesProvider);
 
     registerDashboardUsageCollector(plugins.usageCollection, plugins.embeddable);

Checklist

Delete any items that are not applicable to this PR.

@pgayvallet pgayvallet added Feature:Saved Objects 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.12.0 v8.0.0 labels Jan 12, 2021
@pgayvallet pgayvallet requested a review from a team as a code owner January 14, 2021 14:02
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the functionality in the context of alerting's needs and LGTM 👍

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in Spaces plugin tests LGTM and Copy to Space API/UI can be updated in a follow-up.

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very little to add here. Follows a similar structure to #87807 so it's quite easy to follow. Let's make sure to track Alerting's and SIEM's integration with this once these two PRs are merged.

Comment on lines 204 to 205
/** The path (without the basePath) that the user should be redirect to to address this warning. */
actionUrl: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we name this actionPath instead of URL to make this more clear that it should be an in-app path and cannot be an external URL?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
savedObjectsManagement 190.6KB 192.9KB +2.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 562.3KB 563.6KB +1.3KB
savedObjectsManagement 72.0KB 72.0KB +33.0B
total +1.3KB

History

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

@pgayvallet pgayvallet merged commit edb338a into elastic:master Jan 20, 2021
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Jan 20, 2021
* initial commit

* adapt client-side signatures

* more type fixes

* adapt api IT asserts

* fix some unit tests

* fix more test usages

* fix integration tests

* fix FT test assertions

* fix FT test assertions

* add FTR API integ test suite

* create the plugin_api_integration test suite

* adapt and fix flyout tests

* update documentation

* update generated doc

* add unit tests for `executeImportHooks`

* wire resolve_import_errors and add unit tests

* move hooks registration to SO type API

* update generated doc

* design integration

* update generated doc

* Add FTR tests for import warnings

* deletes plugins api integ tests

* self review

* move onImport to management definition

* update license header

* rename actionUrl to actionPath
jportner added a commit to jportner/kibana that referenced this pull request Jan 20, 2021
pgayvallet added a commit that referenced this pull request Jan 20, 2021
* initial commit

* adapt client-side signatures

* more type fixes

* adapt api IT asserts

* fix some unit tests

* fix more test usages

* fix integration tests

* fix FT test assertions

* fix FT test assertions

* add FTR API integ test suite

* create the plugin_api_integration test suite

* adapt and fix flyout tests

* update documentation

* update generated doc

* add unit tests for `executeImportHooks`

* wire resolve_import_errors and add unit tests

* move hooks registration to SO type API

* update generated doc

* design integration

* update generated doc

* Add FTR tests for import warnings

* deletes plugins api integ tests

* self review

* move onImport to management definition

* update license header

* rename actionUrl to actionPath
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Saved Objects 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.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Saved-object import warnings
7 participants