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

[Core] Move Saved objects files to core #38771

Merged
merged 9 commits into from
Jun 18, 2019

Conversation

rudolf
Copy link
Contributor

@rudolf rudolf commented Jun 12, 2019

Summary

This is the first phase of exposing Saved Objects client from Core.

  • Moves (most) files in src/legacy/server/saved_objects/ -> src/core/server/saved_objects.
  • Expose (most) SavedObject types through src/core/server
  • Left some files in legacy and kept some imports that reach deep into src/core/server/saved_objects to ease the migration.

@rudolf rudolf force-pushed the saved-object-to-core branch 2 times, most recently from 3596fbd to 0b7a0df Compare June 13, 2019 01:30
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@rudolf rudolf added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Jun 13, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@rudolf rudolf added non-issue Indicates to automation that a pull request should not appear in the release notes v7.3.0 v8.0.0 Feature:New Platform Feature:Saved Objects labels Jun 13, 2019
@rudolf rudolf changed the title Saved object to core Move Saved objects files to core Jun 13, 2019
@rudolf rudolf changed the title Move Saved objects files to core [Core] Move Saved objects files to core Jun 13, 2019
@rudolf rudolf marked this pull request as ready for review June 14, 2019 08:07
@rudolf rudolf requested review from a team as code owners June 14, 2019 08:07
@rudolf rudolf added the release_note:skip Skip the PR/issue when compiling release notes label Jun 14, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@rudolf rudolf force-pushed the saved-object-to-core branch from b59a1ca to 514e79b Compare June 14, 2019 08:53
@epixa epixa mentioned this pull request Jun 14, 2019
20 tasks
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@rudolf rudolf added the review label Jun 14, 2019
@legrego legrego self-requested a review June 14, 2019 12:35
@elastic elastic deleted a comment from elasticmachine Jun 14, 2019
@elastic elastic deleted a comment from elasticmachine Jun 14, 2019
@elastic elastic deleted a comment from elasticmachine Jun 14, 2019
@elastic elastic deleted a comment from elasticmachine Jun 14, 2019
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.

Changes for spaces and encrypted_saved_objects LGTM!

delete(type: string, id: string, options?: SavedObjectsBaseOptions): Promise<{}>;
// (undocumented)
errors: {
isSavedObjectsClientError: typeof import("./lib/errors").isSavedObjectsClientError;
Copy link
Contributor

Choose a reason for hiding this comment

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

1 nit: not really useful typings
2 expose the same things as a class property and class static property can be confusing for customers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I've addressed this by creating a SavedObjectsErrorHelpers class so that the type documentation is better.
  2. Because of the way I type SavedObjectsClientContract the static errors property isn't included in the type. So when I remove the public errors there's a lot of type errors even though this should technically resolve to the static function. I can probably fix this by creating a SavedObjectsClientContract interface and duplicating the types from the class. However, because SavedObjectsErrorHelpers is just static functions, I've been thinking of removing these convenience errors properties and instead let consumers rather import SavedObjectsErrorHelpers directly. Because this is a breaking API change, I'd prefer to do this in a separate PR.

Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

LGTM, based on the assumption that this is just type/file changes.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

👍 on green CI

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@rudolf rudolf merged commit 260d907 into elastic:master Jun 18, 2019
@rudolf rudolf deleted the saved-object-to-core branch June 18, 2019 11:10
rudolf added a commit that referenced this pull request Jun 18, 2019
* Move src/legacy/server/saved_objects -> src/core/server/saved_objects

* Fix SavedObject import references after moving files to core

* First pass at SavedObjects api docs

* Expose and import all saved object types through core/server

* Don't expose SavedObjectsManagement from core and fix imports

* Improve typings for SavedObject error helpers

* Fix type errors after master merge

* Fix SavedObjectErrorHelpers tests
rudolf added a commit that referenced this pull request Jun 19, 2019
* Move src/legacy/server/saved_objects -> src/core/server/saved_objects

* Fix SavedObject import references after moving files to core

* First pass at SavedObjects api docs

* Expose and import all saved object types through core/server

* Don't expose SavedObjectsManagement from core and fix imports

* Improve typings for SavedObject error helpers

* Fix type errors after master merge

* Fix SavedObjectErrorHelpers tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform Feature:Saved Objects non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.3.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants