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

Migrate remaining parts of server-side SO domain to packages #139305

Merged
merged 44 commits into from
Aug 26, 2022
Merged

Migrate remaining parts of server-side SO domain to packages #139305

merged 44 commits into from
Aug 26, 2022

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Aug 23, 2022 •
edited
Loading

Summary

Fix #135820
Follow-up of #136722, #137183 and #137448

This PR migrates the remaining parts of Core's server-side SO domain to packages.

The PR introduces the following SO packages:

  • @kbn/core-saved-objects-api-server-internal
  • @kbn/core-saved-objects-api-server-mocks
  • @kbn/core-saved-objects-import-export-server-internal
  • @kbn/core-saved-objects-import-export-server-mocks
  • @kbn/core-saved-objects-migration-server-internal
  • @kbn/core-saved-objects-migration-server-mocks
  • @kbn/core-saved-objects-server-internal
  • @kbn/core-saved-objects-server-mocks

And the following packages that were required because of the cyclic / bidirectional dependencies against the SO service:

  • @kbn/core-usage-data-base-server-internal
  • @kbn/core-usage-data-server
  • @kbn/core-deprecations-server

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v8.5.0 labels Aug 23, 2022
@pgayvallet pgayvallet force-pushed the kbn-135820-so-domain-last-part branch from 27c7723 to 08efcb2 Compare August 24, 2022 08:39
@pgayvallet pgayvallet force-pushed the kbn-135820-so-domain-last-part branch from c2c4e3e to cf84faf Compare August 24, 2022 12:52
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

juliaElastic
juliaElastic approved these changes Aug 25, 2022
View reviewed changes
Copy link
Contributor

@juliaElastic juliaElastic left a comment

Choose a reason for hiding this comment

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

LGTM

@TinaHeiligers
Copy link
Contributor

@jeramysoucy Once this PR merges, you should be unblocked on #134395.
There's no need to wait for #30503.
cc @legrego

@TinaHeiligers TinaHeiligers self-requested a review August 25, 2022 16:10
TinaHeiligers
TinaHeiligers reviewed Aug 25, 2022
View reviewed changes
packages/core/saved-objects/core-saved-objects-api-server-internal/package.json
"name": "@kbn/core-saved-objects-api-server-internal",
"private": true,
"version": "1.0.0",
"main": "./target_node/index.js",
Copy link
Contributor

@TinaHeiligers TinaHeiligers Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
"main": "./target_node/index.js",
"main": "./target_node/index.js",
"author": "Kibana Core",

TinaHeiligers
TinaHeiligers reviewed Aug 25, 2022
View reviewed changes
packages/core/saved-objects/core-saved-objects-migration-server-internal/package.json Show resolved Hide resolved
packages/core/saved-objects/core-saved-objects-migration-server-mocks/package.json Show resolved Hide resolved
packages/core/saved-objects/core-saved-objects-server-internal/package.json Show resolved Hide resolved
packages/core/saved-objects/core-saved-objects-server-mocks/package.json Show resolved Hide resolved
packages/core/usage-data/core-usage-data-base-server-internal/package.json Show resolved Hide resolved
packages/core/usage-data/core-usage-data-server/package.json Show resolved Hide resolved
packages/core/usage-data/core-usage-data-base-server-internal/README.md
This package contains internal base types and constants for Core's server-side `usage-data` domain that are used
across some other domains.

The purpose of the package is mostly to break the cyclic dependency between the `core-usage-data` and `savedObjects` services.
Copy link
Contributor

Choose a reason for hiding this comment

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

aside: Are we going to standardize on using the *-base-* package name tag for extra packages we need to break cyclic dependencies?
If so, can we generally assume that these packages won’t have associated public types packages?

At the end, in the audit and clean up phase, we should consider adding another folder for shared service dependencies and move the packages to those. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Regarding standardization, tbh I'm even starting to think that the current structure / suffix/prefix naming may not be the best one. But let's finish the migration the way the started it, and we then should discuss a bit regarding what we think we may want to improve.

TinaHeiligers
TinaHeiligers reviewed Aug 25, 2022
View reviewed changes
src/core/server/index.ts
export type {
SavedObjectsRepository,
SavedObjectsExporter,
SavedObjectsExportError,
Copy link
Contributor

Choose a reason for hiding this comment

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

SavedObjectExportError is public. Is there a reason we don't have public type packages for import/export ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SavedObjectExportError is public

Yea, and no... tbh it wasn't supposed to be public, but some consumers started using it, so.. here we are.

SavedObjectExportError (and the import counterpart) are concrete implementations atm, without an explicit type equivalent. I basically didn't want to increase the changes of this PR, as introducing a type would mean adapting the existing external usages.

But I can take another look

Is there a reason we don't have public type packages for import/export

I regrouped all the public types of all sub domains in the same @kbn/core-saved-objects-server package. Because it was easier, but also because we effectively have some cyclic dependencies between the public types of the subdomains IIRC.

Copy link
Contributor

Choose a reason for hiding this comment

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

ya, the cyclic dependencies would've meant we need somewhere common to share these types from.

TinaHeiligers
TinaHeiligers reviewed Aug 25, 2022
View reviewed changes
src/core/server/deprecations/index.ts
RegisterDeprecationsConfig,
} from '@kbn/core-deprecations-server';

export type {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to update #135961 with the remaining work to migrate the deprecations service.

TinaHeiligers
TinaHeiligers reviewed Aug 25, 2022
View reviewed changes
packages/core/saved-objects/core-saved-objects-server-mocks/README.md
@@ -0,0 +1,5 @@
# @kbn/core-saved-objects-server-mocks

This package contains the mocks for core's server-side savedObjects service.
Copy link
Contributor

@TinaHeiligers TinaHeiligers Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm starting to think we'll need a core-dev-only README in the root packages/core/saved-objects folder as an index to where all the sub-domain services and mocks are within the package architecture.

For now, we can use core/server/server, core/server/mocks, core/server/index and core/server/types (if we have to) as a lookup guide.

OTOH, these root files will likely move to their own packages later on and we could extend their README's.

TinaHeiligers
TinaHeiligers approved these changes Aug 25, 2022
View reviewed changes
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

I've gone through the code and changes and left a few comments and nits. Nothing stands out to me that would prevent merging and CI seems to agree.

Nice work migrating the massive domain!

@pgayvallet pgayvallet mentioned this pull request Aug 26, 2022
Closed
@pgayvallet pgayvallet enabled auto-merge (squash) August 26, 2022 08:25
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/core-deprecations-server - 11 +11
@kbn/core-saved-objects-api-server 125 126 +1
@kbn/core-saved-objects-api-server-internal - 49 +49
@kbn/core-saved-objects-api-server-mocks - 6 +6
@kbn/core-saved-objects-base-server-internal 29 31 +2
@kbn/core-saved-objects-import-export-server-internal - 23 +23
@kbn/core-saved-objects-import-export-server-mocks - 4 +4
@kbn/core-saved-objects-migration-server-internal - 19 +19
@kbn/core-saved-objects-migration-server-mocks - 12 +12
@kbn/core-saved-objects-server-internal - 9 +9
@kbn/core-saved-objects-server-mocks - 13 +13
@kbn/core-usage-data-server - 135 +135
core 216 136 -80
total +204

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/core-saved-objects-server-internal - 3 +3
Unknown metric groups

API count

id before after diff
@kbn/core-deprecations-server - 12 +12
@kbn/core-saved-objects-api-server 288 289 +1
@kbn/core-saved-objects-api-server-internal - 68 +68
@kbn/core-saved-objects-api-server-mocks - 6 +6
@kbn/core-saved-objects-base-server-internal 35 37 +2
@kbn/core-saved-objects-import-export-server-internal - 25 +25
@kbn/core-saved-objects-import-export-server-mocks - 4 +4
@kbn/core-saved-objects-migration-server-internal - 24 +24
@kbn/core-saved-objects-migration-server-mocks - 12 +12
@kbn/core-saved-objects-server-internal - 9 +9
@kbn/core-saved-objects-server-mocks - 14 +14
@kbn/core-usage-data-server - 146 +146
core 2524 2674 +150
total +473

ESLint disabled in files

id before after diff
@kbn/core-saved-objects-api-server-internal - 1 +1
core 5 4 -1
total -0

ESLint disabled line counts

id before after diff
@kbn/core-saved-objects-api-server-internal - 5 +5
@kbn/core-usage-data-server - 1 +1
core 29 23 -6
kibanaUsageCollection 5 6 +1
total +1

References to deprecated APIs

id before after diff
@kbn/core-saved-objects-migration-server-internal - 2 +2

Total ESLint disabled count

id before after diff
@kbn/core-saved-objects-api-server-internal - 6 +6
@kbn/core-usage-data-server - 1 +1
core 34 27 -7
kibanaUsageCollection 8 9 +1
total +1

History

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

@pgayvallet pgayvallet merged commit 34c228b into elastic:main Aug 26, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 26, 2022
This was referenced Aug 31, 2022
Merged
Merged
Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022
@pgayvallet @kibanamachine @Mpdreamz
Migrate remaining parts of server-side SO domain to packages (elastic…
a051d6c 
…#139305)

* create empty packages

* create empty mock packages

* start moving client/repository implementation

* finish moving client/repo code

* move import/export code

* move migration code

* create empty mocks package

* start moving service code

* continue fixing stuff, create yet another package

* create usage-data package for internal contract

* create core-deprecations-server package

* fix last problems

* fix mock package

* start fixing usages

* fix index exports

* start fixing unit tests for new packages

* lint

* fix invalid char

* fix more violations and test failures

* fix more package tests

* adapt more test files

* one more fix

* adapt repo tests

* fix last (?) package tests

* fix moment mocking

* expose import/export error types again

* remove test mocking

* adapt imports

* export as type

* trying to fix the schema check task

* duplicating usage collection types for now

* fix stack trace assertion

* fix duplicate import from different paths

* [CI] Auto-commit changed files from 'node scripts/generate packages_build_manifest'

* update readme's

* move mocks to a dedicated folder

* self review

* move test_utils to integration tests

* update package files

* rename mocks

* manually adapting exclude

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TinaHeiligers TinaHeiligers TinaHeiligers approved these changes

@spalger spalger spalger left review comments

@juliaElastic juliaElastic juliaElastic approved these changes

@Kerry350 Kerry350 Kerry350 approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate types to packages: Server-side saved_objects service
8 participants
@pgayvallet @elasticmachine @TinaHeiligers @kibana-ci @Kerry350 @spalger @juliaElastic @kibanamachine