-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Refactor saved object management registry usage #54155
Refactor saved object management registry usage #54155
Conversation
…-07-refactor-saved-object-management-registry
…-07-refactor-saved-object-management-registry
…-07-refactor-saved-object-management-registry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, didn't test locally. This change might be worth a dev doc section.
thx for the review dear @flash1292 and I do agree, there should be a dev doc section about this change 👍 |
@elasticmachine merge upstream |
…-management-registry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested transform, saved objects management and timelion on Ubuntu \ Chrome
Overall code seems LGTM.
Added a couple of comments.
src/legacy/core_plugins/kibana/public/management/sections/objects/_view.js
Outdated
Show resolved
Hide resolved
src/legacy/core_plugins/kibana/public/management/saved_object_registry.ts
Show resolved
Hide resolved
src/legacy/core_plugins/kibana/public/management/sections/objects/breadcrumbs.js
Outdated
Show resolved
Hide resolved
src/legacy/core_plugins/kibana/public/management/sections/objects/_view.js
Show resolved
Hide resolved
src/legacy/core_plugins/timelion/public/services/saved_sheets.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the transform
plugin changes. Code LGTM, also tested a local checkout.
thx @flash1293 , @lizozom , @walterra for your reviews 🙇 . @lizozom , I'll address you're useful hints soon |
src/legacy/core_plugins/kibana/public/management/saved_object_registry.ts
Show resolved
Hide resolved
src/legacy/core_plugins/kibana/public/visualize_embeddable/visualize_embeddable_factory.tsx
Outdated
Show resolved
Hide resolved
…egistry' of github.com:kertal/kibana into kertal-pr-2020-01-07-refactor-saved-object-management-registry
due to the switch to NP API angular didn't get noticed when the request finished
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm, thanks for moving saved vis stuff to visualizations plugin!
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* Migrate registry to TypeScript * Migrate management code * Migrate SavedObjectLoader services registration to management section * Replace Angular SavedSearchLoader in transform plugin * Migrate saved_visualizations from visualize to visualizations plugin
* Migrate registry to TypeScript * Migrate management code * Migrate SavedObjectLoader services registration to management section * Replace Angular SavedSearchLoader in transform plugin * Migrate saved_visualizations from visualize to visualizations plugin
…ve-out-legacy * 'master' of github.com:elastic/kibana: (187 commits) [ML] Reseting categorization validation if category field is cleared (elastic#56029) [SIEM] Fields browser readable (elastic#56000) [docs] Remove unused callout (elastic#56032) Refactor saved object management registry usage (elastic#54155) [SIEM][Detection Engine] critical blocker, updates the pre-packaged rules, removes dead ones, adds license file (elastic#56090) Fix failing snapshot artifact tests when using env var (elastic#56063) Fix Github PR comment formatting (elastic#56078) [Maps] fix join metric field selection bugs (elastic#56044) Create a new menu for observability links (elastic#54847) [SIEM] [Detection Engine] Fixes histogram intervals (elastic#55969) make test less flaky by retrying if list is re-rendered (elastic#55949) Remove matrix build support (elastic#54202) Add animation to service map layout (elastic#56042) [Canvas] Remove Angular and unnecessary reporting config from Canvas (elastic#54050) [Uptime] Simplify snapshot max to Infinity (elastic#55931) [Uptime] Reintroduce a column for url (elastic#55451) Cleanup action task params objects after successful execution (elastic#55227) [CI] Retry flaky tests (elastic#53961) Expose NP FieldFormats service to server side (elastic#55419) [Endpoint] EMT-65: make endpoint data types common, restructure (elastic#54772) ... # Conflicts: # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/__snapshots__/split_panel.test.tsx.snap # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/containers/panel.tsx # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/context.tsx # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/index.ts # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/split_panel.test.tsx # src/legacy/ui/public/vis/editors/default/default_editor.tsx # src/plugins/console/public/application/components/split_panel/__snapshots__/split_panel.test.tsx.snap # src/plugins/console/public/application/components/split_panel/components/resizer.tsx # src/plugins/console/public/application/components/split_panel/containers/panel.tsx # src/plugins/console/public/application/components/split_panel/containers/panel_container.tsx # src/plugins/console/public/application/components/split_panel/context.tsx # src/plugins/console/public/application/components/split_panel/index.ts # src/plugins/console/public/application/components/split_panel/registry.ts # src/plugins/console/public/application/components/split_panel/split_panel.test.tsx # src/plugins/kibana_react/public/split_panel/__snapshots__/split_panel.test.tsx.snap # src/plugins/kibana_react/public/split_panel/containers/panel.tsx # src/plugins/kibana_react/public/split_panel/context.tsx # src/plugins/kibana_react/public/split_panel/index.ts # src/plugins/kibana_react/public/split_panel/split_panel.test.tsx
* master: (77 commits) [ML] Reseting categorization validation if category field is cleared (elastic#56029) [SIEM] Fields browser readable (elastic#56000) [docs] Remove unused callout (elastic#56032) Refactor saved object management registry usage (elastic#54155) [SIEM][Detection Engine] critical blocker, updates the pre-packaged rules, removes dead ones, adds license file (elastic#56090) Fix failing snapshot artifact tests when using env var (elastic#56063) Fix Github PR comment formatting (elastic#56078) [Maps] fix join metric field selection bugs (elastic#56044) Create a new menu for observability links (elastic#54847) [SIEM] [Detection Engine] Fixes histogram intervals (elastic#55969) make test less flaky by retrying if list is re-rendered (elastic#55949) Remove matrix build support (elastic#54202) Add animation to service map layout (elastic#56042) [Canvas] Remove Angular and unnecessary reporting config from Canvas (elastic#54050) [Uptime] Simplify snapshot max to Infinity (elastic#55931) [Uptime] Reintroduce a column for url (elastic#55451) Cleanup action task params objects after successful execution (elastic#55227) [CI] Retry flaky tests (elastic#53961) Expose NP FieldFormats service to server side (elastic#55419) [Endpoint] EMT-65: make endpoint data types common, restructure (elastic#54772) ...
Summary
Management will continue to use the
SavedObjectManagementRegistry
for a while. However, the way it's currently used isn't necessary any more. TheSavedObjectLoader
services first had to be registered in the global Angular, the ids were saved in the registry. For use in Management, the services where retrieved from the global angular. This PR now registers the services directly, removes global angular registration ofSavedObjectLoader
servives in discover / visualize / dashboard. Timelion still needs the SavedObjectLoader registered in Angular at the moment.Since
savedSearches
is no longer registered in Angular by Discover, I've also converted the last client, thetransform
plugin, to get this service by calling thecreateSavedSearchesService
function that thediscover
plugin offers instead.Dev docs
Registration of the following
SavedObjectLoader
in Angular was removed:savedSearches
savedVisualizations
savedDashboard
The plugins now provide the functions to create a
SavedObjectLoader
service, here's an example how the services are created now:Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] This was checked for cross-browser compatibility, including a check against IE11- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] Unit or functional tests were updated or added to match the most common scenarios- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately