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

Migrate savedObjectManagementActionRegistry to NP plugin #60481

Merged

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Mar 18, 2020

Summary

Part of #50308

Migrate the savedObjects management actions registry to the new SavedObjectsManagement plugin and adapt existing calls.

Checklist

@rudolf rudolf mentioned this pull request Mar 18, 2020
5 tasks
@pgayvallet pgayvallet changed the title Migrate savedObjectManagementRegistry to NP plugin Migrate savedObjectManagementActionRegistry to NP plugin Mar 18, 2020
@pgayvallet pgayvallet added Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Mar 18, 2020
@elasticmachine
Copy link
Contributor

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

@pgayvallet pgayvallet added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Mar 18, 2020
Comment on lines 21 to 22
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { homePluginMock } from '../../home/public/mocks';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

home/public/mocks is a folder, it seems to break out eslint rule, and I was forced to eslint-disable

Comment on lines 22 to 26
export interface SavedObjectsManagementRecordReference {
type: string;
id: string;
name: string;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duplicate of SavedObjectReference, removed it.

Comment on lines +7 to +14
"optionalPlugins": [
"advancedSettings",
"home",
"management",
"security",
"usageCollection",
"savedObjectsManagement"
],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess is that this dependency should be optional. @elastic/kibana-security tell me if this should move to required.

Copy link
Member

Choose a reason for hiding this comment

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

You're absolutely right, we should make this an optional dependency

notificationsSetup: core.notifications,
});
},
registerLegacyAPI: (legacyAPI: LegacyAPI) => {},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elastic/kibana-security As I said on slack, this is now an empty function. Should I remove it and associated legacy call, or do you want to keep it in case of?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if you're able to easily remove this, then please do so. If it becomes too much, I'll happily to it in a followup PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 4b99311

@pgayvallet pgayvallet marked this pull request as ready for review March 18, 2020 09:49
@pgayvallet pgayvallet requested review from a team as code owners March 18, 2020 09:49
@pgayvallet pgayvallet requested a review from a team March 18, 2020 09:50
@@ -0,0 +1,7 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Should CODEOWNERS be updated to give this plugin an owner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. updated

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Functionally LGTM - tested with both spaces enabled and disabled. I'll leave the review of the so_management plugin itself to the platform team, but I didn't see anything out of place

@mattkime
Copy link
Contributor

LGTM once registerLegacyAPI is removed (or an issue is created to remove it later) and SavedObjectManagement is made optional for Spaces

@spalger
Copy link
Contributor

spalger commented Mar 26, 2020

@elasticmachine merge upstream

@pgayvallet pgayvallet added v7.8 and removed v7.7.0 labels Mar 26, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@pgayvallet pgayvallet merged commit 7ab38ff into elastic:master Mar 26, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Mar 26, 2020
* create empty plugin + move home feature registration to it

* move the so action_registry to new plugin

* adapt existing calls to the registry

* fix i18n namespace

* fix table unit tests

* update codeowners

* rename plugin to match other PRs

* remove registerLegacyAPI from spaces public plugin

* fix typo

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
pgayvallet added a commit that referenced this pull request Mar 26, 2020
…1492)

* create empty plugin + move home feature registration to it

* move the so action_registry to new plugin

* adapt existing calls to the registry

* fix i18n namespace

* fix table unit tests

* update codeowners

* rename plugin to match other PRs

* remove registerLegacyAPI from spaces public plugin

* fix typo

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@rayafratkina rayafratkina added v7.8.0 and removed v7.8 labels Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants