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

[NP] Move ui/saved_objects to NP #57452

Merged
merged 15 commits into from
Feb 20, 2020

Conversation

maryia-lapata
Copy link
Contributor

@maryia-lapata maryia-lapata commented Feb 12, 2020

Fixes: #56954.

This PR contains movement of src/legacy/ui/public/saved_objects to src/plugins/saved_objects/public. All consumers were switched to the new location directly. The old unit tests were converted to Jest.

@maryia-lapata maryia-lapata added Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0 labels Feb 17, 2020
@elasticmachine
Copy link
Contributor

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

@maryia-lapata maryia-lapata changed the title Move saved objects [NP] Move ui/saved_objects to NP Feb 17, 2020
@ppisljar ppisljar mentioned this pull request Feb 17, 2020
11 tasks
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, great work! Left one comment I'm not sure about.

I know it's already like this in master, but the plugin should contain a kibana.json descriptor file on the top level. Check out the other plugins in src/plugins. I would have thought this would fail the build somehow, interesting.

src/core/MIGRATION.md Show resolved Hide resolved
@maryia-lapata maryia-lapata marked this pull request as ready for review February 19, 2020 13:22
@maryia-lapata maryia-lapata requested a review from a team February 19, 2020 13:22
@maryia-lapata maryia-lapata requested review from a team as code owners February 19, 2020 13:22
@joshdover
Copy link
Contributor

I know it's already like this in master, but the plugin should contain a kibana.json descriptor file on the top level. Check out the other plugins in src/plugins. I would have thought this would fail the build somehow, interesting.

This is only required if the plugin is a real "runtime" plugin, not just static exports. I don't think this is necessary until we migrate the management UI into this plugin as well, but doesn't hurt to add it now. 🤷‍♂

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.

I know this is just a move to unblock other teams, but I am a bit concerned about the lack of test coverage, especially on the SavedObjectsLoader class. I think it's fine if we merge without additional coverage, but I'd like to see a follow-up issue/PR for getting at least some more coverage on this.

# Conflicts:
#	src/plugins/data/public/mocks.ts
#	src/plugins/data/public/ui/query_string_input/__snapshots__/query_string_input.test.tsx.snap
@maryia-lapata
Copy link
Contributor Author

I know it's already like this in master, but the plugin should contain a kibana.json descriptor file on the top level. Check out the other plugins in src/plugins. I would have thought this would fail the build somehow, interesting.

This is only required if the plugin is a real "runtime" plugin, not just static exports. I don't think this is necessary until we migrate the management UI into this plugin as well, but doesn't hurt to add it now. 🤷‍♂

@flash1293 , @joshdover I added kibana.json and almost empty plugin.ts here. Please take a look.

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Code LGTM.
I am working on finishing shim for src/legacy/core_plugins/visualizations/ and this was one of the biggest dependencies there. Thanks 👍

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Changes LGTM, thanks a lot for this! This is another major blocker gone for multiple teams. I will create a follow-up issue to subsequently clean up the implementation and increase test coverage.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

maps changes lgtm
code review

# Conflicts:
#	src/legacy/core_plugins/visualizations/public/embeddable/visualize_embeddable.ts
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM 👍

@maryia-lapata maryia-lapata merged commit 857f9f8 into elastic:master Feb 20, 2020
@maryia-lapata maryia-lapata deleted the move-saved-objects branch February 20, 2020 18:20
rylnd added a commit to rylnd/kibana that referenced this pull request Feb 21, 2020
PR elastic#57452 added an empty savedObjects plugin with the same name as the
core plugin. Due to the way we were spreading into our context coupled
with the fact that we don't get NP's whitelisting of plugins on legacy,
we were overriding the core plugin here.

If this happens again, we should perhaps whitelist our plugins here.
rylnd added a commit that referenced this pull request Feb 21, 2020
PR #57452 added an empty savedObjects plugin with the same name as the
core plugin. Due to the way we were spreading into our context coupled
with the fact that we don't get NP's whitelisting of plugins on legacy,
we were overriding the core plugin here.

If this happens again, we should perhaps whitelist our plugins here.
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 21, 2020
…-out-of-legacy

* 'master' of github.com:elastic/kibana: (109 commits)
  document difference between log record formats (elastic#57798)
  Expose elasticsearch config schema (elastic#57655)
  [ui/agg_response/tabify] update types for search/expressions/build_tabular_inspector_data.ts (elastic#58130)
  [SIEM] Cleans Cypress tests code (elastic#58134)
  fix: 🐛 make dev server Storybook builds work again (elastic#58188)
  Prevent core savedObjects plugin from being overridden (elastic#58193)
  Expose serverBasePath on client-side (elastic#58070)
  Fix legend sizing on area charts (elastic#58083)
  Drilldown plugin (elastic#58097)
  [skip-ci] Fix broken links to saved objects APIs in MIGRATION.md (elastic#58033)
  [ML] New Platform server shim: update datafeed routes (elastic#57739)
  Add flag for building static storybook site (elastic#58050)
  add monaco to kbn/ui-shared-deps and load required features for all uses (elastic#58075)
  [SIEM] Let us try out code owners for a little while and see what happens
  Add throttle param to Alerting readme (elastic#57609)
  [NP] Move ui/saved_objects to NP (elastic#57452)
  [Logs UI]  Fix column reordering in settings page (elastic#58104)
  Fix browser date format (elastic#57714)
  Add filter for ILM phase to Index Management (revert elastic#45486) (elastic#57402)
  Clarify Precision function in Timelion Kibana (elastic#58031)
  ...

# Conflicts:
#	x-pack/.i18nrc.json
maryia-lapata added a commit that referenced this pull request Feb 21, 2020
* Move saved_objects to NP

* Update path for imports

* Remove ui/saved_objects

* Update i18n IDs

* Convert test

* Replace Bluebird to Promise; fix unit tests

* Mock openConfirm in test

* Add kibana.json

* Check unit test

* Update unit tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
rylnd added a commit that referenced this pull request Feb 21, 2020
PR #57452 added an empty savedObjects plugin with the same name as the
core plugin. Due to the way we were spreading into our context coupled
with the fact that we don't get NP's whitelisting of plugins on legacy,
we were overriding the core plugin here.

If this happens again, we should perhaps whitelist our plugins here.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move saved objects helper to new platform
8 participants