Removing stateful saved object finder by flash1293 · Pull Request #52166 · elastic/kibana

flash1293 · 2019-12-04T11:05:16Z

This PR removes the legacy SavedObjectFinder and changes all usages to switch to the one provided by kibana_react. The biggest change is that it's now necessary to pass in uiSettings and savedObjects core services to the component.

flash1293 · 2019-12-04T14:27:18Z

Jenkins, test this.

elasticmachine · 2019-12-04T15:42:34Z

💔 Build Failed

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

…ct-finder

elasticmachine · 2019-12-05T14:47:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 5a52169

…ct-finder

elasticmachine · 2019-12-05T16:52:25Z

💔 Build Failed

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

…ct-finder

elasticmachine · 2019-12-06T10:26:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 751a185

elasticmachine · 2019-12-06T10:54:24Z

Pinging @elastic/kibana-app (Team:KibanaApp)

majagrubic · 2019-12-09T09:50:36Z

...lugins/embeddable/public/lib/panel/panel_header/panel_actions/add_panel/add_panel_flyout.tsx

  notifications: CoreSetup['notifications'];
-  SavedObjectFinder: React.ComponentType<any>;
+  savedObjects: CoreStart['savedObjects'];
+  uiSettings: CoreSetup['uiSettings'];


for my own understanding: what's the difference between CoreStart and CoreSetup? Everywhere else uiSettings is read from CoreStart, so why is it here defined as property of CoreSetup?

That's not cleanly done by me, I reverted those changes. The difference between CoreSetup and CoreStart is that they are services available during different startup phases of plugins. For the particular case of uiSettings, start and setup core should be identical so it wouldn't matter.

You can find more information here: https://github.com/elastic/kibana/blob/master/src/core/CONVENTIONS.md

Edit: Copied wrong link

majagrubic · 2019-12-09T10:06:01Z

src/plugins/kibana_react/public/saved_objects/saved_object_finder.tsx

  }, 300);

-  constructor(props: SavedObjectFinderProps) {
+  constructor(props: SavedObjectFinderUiProps) {


Why was introducing this wrapper component necessary, instead of just introducing two new props on the existing SavedObjectFinder component? Are there still places where SavedObjectFinder is used, without uiSettings and savedObjects props?

Not at the moment, but this is the target architecture we are moving towards (see also #52493 ). Passing uiSettings and savedObjects services down through the component tree can be cumbersome in some cases, so the wrapper component is able to pick it out of the Kibana context: https://github.com/elastic/kibana/tree/master/src/plugins/kibana_react#context

The usages of saved object finder in embeddables will move into this direction soon. Future uses should be guided into this direction.

majagrubic

looks good, I just left two questions, mostly for my own understanding

streamich

Why are you doing these changes to embeddables? We are in a process of refactoring embeddables where one does not need to pass all these services, like uiSettings and savedObject through props at every step but can instead consume them through React context. As I see it, your changes are basically step backwards that passes uiSettings and savedObjects services manually at every step through props—something we removed a couple of months ago.

…ct-finder

kertal

Code LGTM, tested in Discover, Visualize, Dashboard, Graph in Chrome, works as expected

flash1293 · 2019-12-09T13:09:01Z

After syncing with @streamich I reverted the changes in embeddables. This means consumers of embeddables are still required to pass in a SavedObjectFinder component. This will be changed once #52491 is implemented. Till then I moved the creation of the component into the places where it's still needed (similar to how it's done in https://github.com/elastic/kibana/blob/master/src/plugins/dashboard_embeddable_container/public/plugin.tsx#L64 )

streamich

App Arch changes LGTM

jgowdyelastic

ML and Transform changes LGTM

…ct-finder

crob611

Canvas changes look good 👍

elasticmachine · 2019-12-10T16:45:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 27ce33a

History

💔 Build #14488 failed 9cf97d1
💚 Build #14182 succeeded db4ae45

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

…le-sql-highlighting * 'master' of github.com:elastic/kibana: (56 commits) Migrate url shortener service (elastic#50896) Re-enable datemath in from/to canvas timelion args (elastic#52159) [Logs + Metrics UI] Remove eslint exceptions (elastic#50979) [Logs + Metrics UI] Add missing headers in Logs & metrics (elastic#52405) [ML] API integration tests - initial tests for bucket span estimator (elastic#52636) [Watcher] New Platform (NP) Migration (elastic#50908) Decouple Authorization subsystem from Legacy API. (elastic#52638) [APM] Fix some warnings logged in APM tests (elastic#52487) [ui/public/utils] Delete unused base_object & find_by_param (elastic#52500) [ui/public/utils] Move items into ui/vis (elastic#52615) fix newlines in kbn-analytics build script Add top level examples folder and command to run, `--run-examples`. (elastic#52027) feat(NA): add trap for SIGINT in the git precommit hook (elastic#52662) [DOCS] Updtes description of elasticsearch.requestHeadersWhitelist (elastic#52675) [Telemetry/Pulse] Updates advanced settings text for usage data (elastic#52657) [SIEM][Detection Engine] Adds the default name space to the end of the signals index [Logs UI] Generalize ML module management (elastic#50662) Removing stateful saved object finder (elastic#52166) Shim oss telemetry (elastic#51168) [Reporting/Screenshots] Do not fail the report if request is aborted (elastic#52344) ... # Conflicts: # src/legacy/core_plugins/console/public/legacy.ts # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/mode/elasticsearch_sql_highlight_rules.ts # src/legacy/core_plugins/console/public/np_ready/lib/autocomplete/components/full_request_component.ts # src/legacy/core_plugins/console/public/quarantined/src/sense_editor/row_parser.js

flash1293 added 2 commits December 4, 2019 12:01

start removing stateful saved object finder

6bbdddc

remove legacy saved object finder

a2fbc47

flash1293 added release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0 labels Dec 4, 2019

flash1293 added 2 commits December 5, 2019 14:30

Merge remote-tracking branch 'upstream/master' into remove-saved-obje…

0c19145

…ct-finder

fix unit test

5a52169

flash1293 added 2 commits December 5, 2019 16:16

Merge remote-tracking branch 'upstream/master' into remove-saved-obje…

98ad3d2

…ct-finder

fix typings

9447808

flash1293 added 2 commits December 6, 2019 10:04

Merge remote-tracking branch 'upstream/master' into remove-saved-obje…

8b76153

…ct-finder

update snapshot

751a185

flash1293 marked this pull request as ready for review December 6, 2019 10:54

flash1293 requested a review from a team December 6, 2019 10:54

flash1293 requested review from a team as code owners December 6, 2019 10:54

flash1293 added the Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// label Dec 6, 2019

majagrubic reviewed Dec 9, 2019

View reviewed changes

streamich suggested changes Dec 9, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into remove-saved-obje…

f1dea23

…ct-finder

kertal approved these changes Dec 9, 2019

View reviewed changes

revert embeddable changes and create stateful wrapper on the boundaries

db4ae45

streamich approved these changes Dec 9, 2019

View reviewed changes

jgowdyelastic approved these changes Dec 10, 2019

View reviewed changes

flash1293 added 2 commits December 10, 2019 12:50

Merge remote-tracking branch 'upstream/master' into remove-saved-obje…

9cf97d1

…ct-finder

Merge remote-tracking branch 'upstream/master' into remove-saved-obje…

39cb7d2

…ct-finder

crob611 approved these changes Dec 10, 2019

View reviewed changes

flash1293 added 2 commits December 10, 2019 16:00

fix uninitialized visualization app for wizard

146b3ab

fix tests

27ce33a

flash1293 merged commit 3c57f71 into elastic:master Dec 10, 2019

flash1293 added a commit to flash1293/kibana that referenced this pull request Dec 10, 2019

Removing stateful saved object finder (elastic#52166)

df68cb7

flash1293 mentioned this pull request Dec 10, 2019

[7.x] Removing stateful saved object finder (#52166) #52654

Merged

flash1293 added a commit that referenced this pull request Dec 11, 2019

Removing stateful saved object finder (#52166) (#52654)

0ce0245

spong mentioned this pull request Jan 29, 2020

[SIEM] Migrate to new platform #45831

Closed

26 tasks

Conversation

flash1293 commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 commented Dec 4, 2019

Uh oh!

elasticmachine commented Dec 4, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Dec 5, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Dec 5, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Dec 6, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Dec 6, 2019

Uh oh!

majagrubic Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

flash1293 Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

majagrubic Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

flash1293 Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

majagrubic left a comment

Choose a reason for hiding this comment

Uh oh!

streamich left a comment

Choose a reason for hiding this comment

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 commented Dec 9, 2019

Uh oh!

streamich left a comment

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic left a comment

Choose a reason for hiding this comment

Uh oh!

crob611 left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Dec 10, 2019

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

flash1293 commented Dec 4, 2019 •

edited

Loading

majagrubic Dec 9, 2019 •

edited

Loading

flash1293 Dec 9, 2019 •

edited

Loading