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.

Sign up for GitHub

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

Jump to bottom

[Core] Move Saved objects files to core #38771

Merged
merged 9 commits into from
Jun 18, 2019
Merged

[Core] Move Saved objects files to core #38771

merged 9 commits into from
Jun 18, 2019
+2,412 −644

Conversation

rudolf
Copy link
Contributor

@rudolf rudolf commented Jun 12, 2019 •
edited
Loading

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

@epixa epixa mentioned this pull request Jun 14, 2019
Closed
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
legrego
legrego approved these changes Jun 14, 2019
View reviewed changes
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!

mshustov
mshustov reviewed Jun 14, 2019
View reviewed changes
src/core/server/server.api.md Outdated
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.

dgieselaar
dgieselaar approved these changes Jun 17, 2019
View reviewed changes
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

mshustov
mshustov approved these changes Jun 18, 2019
View reviewed changes
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 rudolf mentioned this pull request Jun 18, 2019
Merged
rudolf added a commit to rudolf/kibana that referenced this pull request Jun 18, 2019
@rudolf
[Core] Move Saved objects files to core (elastic#38771)
c99453c 
* 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 rudolf mentioned this pull request Jun 18, 2019
Closed
rudolf added a commit that referenced this pull request Jun 18, 2019
@rudolf
[Core] Move Saved objects files to core (#38771)
0f456f5 
* 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
@rudolf
[Core] Move Saved objects files to core (#38771) (#39158)
986b70c 
* 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
@wylieconlon wylieconlon mentioned this pull request Jun 20, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgieselaar dgieselaar dgieselaar approved these changes

@joshdover joshdover joshdover approved these changes

@mshustov mshustov mshustov approved these changes

@legrego legrego legrego approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rudolf @elasticmachine @dgieselaar @joshdover @mshustov @legrego