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

[Platform] SavedObject Management #50308

Closed
5 tasks done
rudolf opened this issue Nov 12, 2019 · 10 comments · Fixed by #61700
Closed
5 tasks done

[Platform] SavedObject Management #50308

rudolf opened this issue Nov 12, 2019 · 10 comments · Fixed by #61700
Assignees
Labels
blocker Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0

Comments

@rudolf
Copy link
Contributor

rudolf commented Nov 12, 2019

Move Saved Object Management into a separate Plugin.

  • Allow plugins to configure the values in
    SavedObjectsManagementTypeDefinition for each of their Saved Object types.
    interface SavedObjectsManagementTypeDefinition {
      isImportableAndExportable?: boolean;
      defaultSearchField?: string;
      icon?: string;
      getTitle?: (savedObject: SavedObject) => string;
      getEditUrl?: (savedObject: SavedObject) => string;
      getInAppUrl?: (savedObject: SavedObject) => { path: string; uiCapabilitiesPath: string };
    }
  • Move legacy UI to new plugin src/legacy/core_plugins/kibana/public/management/sections/objects
  • Move legacy routes to new plugin src/legacy/core_plugins/kibana/server/routes/api/management/saved_objects
  • Don't convert deprecated JSON import code to typescript since we will be removing the JSON import UI and routes (src/legacy/core_plugins/kibana/server/routes/api/{import/export}) in 8.0.0 see Remove Dashboard Import/Export API #41439

Progress

@rudolf rudolf added blocker Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v8.0.0 labels Nov 12, 2019
@elasticmachine
Copy link
Contributor

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

@pgayvallet
Copy link
Contributor

isImportableAndExportable is currently used by the export and import routes, that are going to be moved inside core. This flag needs to be kept inside core, probably in the new SavedObjectType introduced in #56378

@pgayvallet pgayvallet self-assigned this Feb 13, 2020
@joshdover
Copy link
Contributor

Any good reason not to just migrate all of this info to the new registry in Core? Seems like poor dev UX to have to register info in two separate places about the same thing.

@pgayvallet
Copy link
Contributor

I think the idea was that as the UI part was going to move to it's own plugin, it was making sense to also move the SavedObjectsManagementTypeDefinition to the plugin, and then performing registration using the plugin's API.

However I agree that the dual registration is bothersome (and the isImportableAndExportable is even required in core as it's used by some routes) We could just add a management 'section' in the new SavedObjectsType, and only move the UI to the new plugin.

@rudolf wdyt?

@rudolf
Copy link
Contributor Author

rudolf commented Feb 28, 2020

Yes, I think the primary motivation for a plugin was to move the UI out of core. Partly because there's a lot of legacy that we can only remove in 8.0.0 and also because we might want to make breaking changes in the UI more often than we would otherwise do in Core.

One could argue that most of these configuration options are UI focussed and are closely related to the plugin's public source code such as it's routes:

getInAppUrl(obj: SavedQuery) {
return {
path: `/app/kibana#/discover?_a=(savedQuery:'${encodeURIComponent(obj.id)}')`,
uiCapabilitiesPath: 'discover.show',
};
},

But I think it will make it much less likely to forget if there's only one registration point that has everything.

@pgayvallet
Copy link
Contributor

One could argue that most of these configuration options are UI focussed and are closely related to the plugin's public source code such as it's routes:

Yea, I agree that these config part of the type are (almost exclusively) related to the UI part that is going to be outside of core. But I think the upside of having to perform only one call to register everything that is related to a specific SO type is probably more important.

@pgayvallet
Copy link
Contributor

pgayvallet commented Mar 4, 2020

The client side part is big and still got some issues that may not be solvable atm (I will add more details on the associated PR).

I will split this into 3 separates PRs:

  • allowing to register management section for SO types from NP API (this should be the last remaining part to allow plugins to fully get rid of legacy plugin SO registration): Add SavedObject management section registration in core  #59291
  • migrate the server-side part of the management section (quite short, only 3 routes)
  • migrate the client-side part of the management section (biggest part of the work)

@timroes
Copy link
Contributor

timroes commented Mar 4, 2020

The saved object management UI also still has very hard links to the old clientside (Angular) services, and requires any type of saved object to register a SavedObjectLoader. Since we want to throw away those old client side services completely, we should also make sure the saved object management UI, does no longer require them, but instead just require the registration of a way more slim configuration (since it anyway didn't use most methods/properties in those services).

@pgayvallet
Copy link
Contributor

@timroes

The saved object management UI also still has very hard links to the old clientside (Angular) services

Working on that atm in #59110. All angular services will be removed. Biggest issue atm is that we don't have a replacement in the NP management plugin for kbnUrl.

goInspectObject={object => {
if (object.meta.editUrl) {
kbnUrl.change(object.meta.editUrl);
$scope.$apply();
}
}}

Probably more are not discovered yet, I'm far from done yet.

and requires any type of saved object to register a SavedObjectLoader. Since we want to throw away those old client side services completely, we should also make sure the saved object management UI, does no longer require them,

I'm still using them in #59110, and as migrating the whole UI part is already quite massive work, the initial version is probably still going to rely on them. Removing / replacing them could definitely be a follow-up though.

Just to be sure, it doesn't look like the registered SavedObjectLoaders still relies on angular services, or does they?

const services = {
savedObjectsClient: npStart.core.savedObjects.client,
indexPatterns: npStart.plugins.data.indexPatterns,
chrome: npStart.core.chrome,
overlays: npStart.core.overlays,
};
savedObjectManagementRegistry.register({
id: 'savedVisualizations',
service: createSavedVisLoader({
...services,
...{ visualizationTypes: new TypesService().start() },
}),
title: 'visualizations',
});

but instead just require the registration of a way more slim configuration (since it anyway didn't use most methods/properties in those services).

The SO management API is using the loader to (at least) load and save objects during import conflict resolution.

export async function saveObject(obj, overwriteAll) {
return await obj.save({ confirmOverwrite: !overwriteAll });
}

What alternative could we be using for that?

@pgayvallet
Copy link
Contributor

@timroes The SavedObjectLoader removal is now a part of #62048

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants