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

SKA: Relocate @kbn/core inside src/core/packages/core #205036

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

gsoldevila
Copy link
Contributor

@gsoldevila gsoldevila commented Dec 20, 2024

Summary

We want to relocate all of packages/core/** modules inside src/core/packages, and we believe some tooling (e.g. build_api_docs) might not like the fact that we're creating a packages folder inside the @kbn/core package folder.

This PR is an attempt at moving it within packages, as the title describes. cc @afharo

@gsoldevila gsoldevila 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 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Dec 20, 2024
Copy link
Contributor

A documentation preview will be available soon.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

LGTM! Let's see if this change has the effect we expect.

I noticed many (previously) broken links. I wouldn't update them here to reduce the PR codereview impact.

@@ -14,7 +14,7 @@ The folder should contain a file per type, named after the snake_case name of th
**src/plugins/my_plugin/server/saved_objects/dashboard_visualization.ts**

```ts
import { SavedObjectsType } from 'src/core/server';
import { SavedObjectsType } from 'src/core/packages/core/server';
Copy link
Member

Choose a reason for hiding this comment

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

All the imports here should actually be @kbn/core/server (or the individual saved-objects package)

@@ -76,7 +76,7 @@ A Bazel [macro](https://docs.bazel.build/versions/master/skylark/macros.html) wi

A Bazel [macro](https://docs.bazel.build/versions/master/skylark/macros.html) will be created to centralize the usage of Webpack. The macro will, at minimum, accept a configuration file and supply a base `webpack.config.js` file. Currently, all plugins share the same Webpack configuration. Allowing a plugin to provide additional configuration will allow plugins the ability to add loaders without affecting the performance of others.

While running Kibana from source in development, the proxy server will ensure that client-side code for plugins is compiled and available. This is currently handled by the [basePathProxy](https://github.com/elastic/kibana/blob/main/src/core/server/http/base_path_proxy_server.ts), where server restarts and optimizer builds are observed and cause the proxy to pause requests. With Bazel, we will utilize [iBazel](https://github.com/bazelbuild/bazel-watche) to watch for file changes and re-build the plugin targets when necessary. The watcher will emit [events](https://github.com/bazelbuild/bazel-watcher#remote-events) that we will use to block requests and provide feedback to the logs.
While running Kibana from source in development, the proxy server will ensure that client-side code for plugins is compiled and available. This is currently handled by the [basePathProxy](https://github.com/elastic/kibana/blob/main/src/core/packages/core/server/http/base_path_proxy_server.ts), where server restarts and optimizer builds are observed and cause the proxy to pause requests. With Bazel, we will utilize [iBazel](https://github.com/bazelbuild/bazel-watche) to watch for file changes and re-build the plugin targets when necessary. The watcher will emit [events](https://github.com/bazelbuild/bazel-watcher#remote-events) that we will use to block requests and provide feedback to the logs.
Copy link
Member

Choose a reason for hiding this comment

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

this path is no-longer working (it didn't work before this PR anyways) 🫠

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll exclude all changes in legacy_rfcs

@@ -67,7 +67,7 @@ export interface AppMountParameters<HistoryLocationState = unknown> {
* import ReactDOM from 'react-dom';
* import { Router, Route } from 'react-router-dom';
*
* import { CoreStart, AppMountParameters } from 'src/core/public';
* import { CoreStart, AppMountParameters } from 'src/core/packages/core/public';
Copy link
Member

Choose a reason for hiding this comment

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

@kbn/core/public

@@ -20,12 +20,12 @@ export interface DeprecationDetailsMessage {
export interface BaseDeprecationDetails {
/**
* The title of the deprecation.
* Check the README for writing deprecations in `src/core/server/deprecations/README.mdx`
* Check the README for writing deprecations in `src/core/packages/core/server/deprecations/README.mdx`
Copy link
Member

Choose a reason for hiding this comment

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

I doubt this link worked 🫠

@@ -58,7 +58,7 @@ export class HomePageObject extends FtrService {
}

async isWelcomeInterstitialDisplayed() {
// This element inherits style defined {@link https://github.com/elastic/kibana/blob/v8.14.3/src/core/public/styles/core_app/_mixins.scss#L72|here}
// This element inherits style defined {@link https://github.com/elastic/kibana/blob/v8.14.3/src/core/packages/core/public/styles/core_app/_mixins.scss#L72|here}
Copy link
Member

Choose a reason for hiding this comment

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

we are not backporting to v8.14.3 😬

Comment on lines 39 to 40
click Collectors "https://github.com/elastic/kibana/tree/main/src/core/packages/core/server/metrics/collectors"
click OpsMetricsObservable "https://github.com/elastic/kibana/blob/92a8636f0ff63ab072527574e96e6616327b2ea4/src/core/packages/core/server/metrics/metrics_service.ts#L32"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
click Collectors "https://github.com/elastic/kibana/tree/main/src/core/packages/core/server/metrics/collectors"
click OpsMetricsObservable "https://github.com/elastic/kibana/blob/92a8636f0ff63ab072527574e96e6616327b2ea4/src/core/packages/core/server/metrics/metrics_service.ts#L32"
click Collectors "https://github.com/elastic/kibana/tree/main/src/core/server/metrics/collectors"
click OpsMetricsObservable "https://github.com/elastic/kibana/blob/92a8636f0ff63ab072527574e96e6616327b2ea4/src/core/server/metrics/metrics_service.ts#L32"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually moving the server folder, so I think the 1st line is correct? I'll revert changes in the second.

Copy link
Member

Choose a reason for hiding this comment

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

My point is that src/core/metrics don't exist since we moved the service to a package. But we forgot to update this reference.

I'd prefer to update these already broken URLs in a separate PR to avoid making too much noise in this important PR.

@@ -85,6 +85,6 @@ export const deprecations = ({
// vice versa.
// ^ These deprecations should only be shown if they are explicitly configured for monitoring -- we should not show Monitoring
// deprecations for these settings if they are inherited from the Core elasticsearch settings.
// See the Core implementation: src/core/server/elasticsearch/elasticsearch_config.ts
// See the Core implementation: src/core/packages/core/server/elasticsearch/elasticsearch_config.ts
Copy link
Member

Choose a reason for hiding this comment

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

Old path was still broken... let's minimize noise.

Suggested change
// See the Core implementation: src/core/packages/core/server/elasticsearch/elasticsearch_config.ts
// See the Core implementation: src/core/server/elasticsearch/elasticsearch_config.ts

@@ -29,7 +29,7 @@ These surface runtime deprecations, e.g. a Painless script that uses a deprecate
request to a deprecated API. These are also generally surfaced as deprecation headers within the
response. Even if the cluster state is good, app maintainers need to watch the logs in case
deprecations are discovered as data is migrated. Starting in 7.x, deprecation logs can be written to a file or a data stream ([#58924](https://github.com/elastic/elasticsearch/pull/58924)). When the data stream exists, the Upgrade Assistant provides a way to analyze the logs through Observability or Discover ([#106521](https://github.com/elastic/kibana/pull/106521)).
* [**Kibana deprecations API.**](https://github.com/elastic/kibana/blob/main/src/core/server/docs/kib_core_deprecations_service.mdx) This is information about deprecated features and configs in Kibana. These deprecations are only communicated to the user if the deployment is using these features. Kibana engineers are responsible for adding deprecations to the deprecations API for their respective team.
* [**Kibana deprecations API.**](https://github.com/elastic/kibana/blob/main/src/core/packages/core/server/docs/kib_core_deprecations_service.mdx) This is information about deprecated features and configs in Kibana. These deprecations are only communicated to the user if the deployment is using these features. Kibana engineers are responsible for adding deprecations to the deprecations API for their respective team.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@gsoldevila gsoldevila force-pushed the move-kbn-core-to-src-core-packages branch 4 times, most recently from 2a25ad1 to a11d70f Compare December 20, 2024 14:57
@gsoldevila gsoldevila marked this pull request as ready for review December 20, 2024 20:41
@gsoldevila gsoldevila requested review from a team as code owners December 20, 2024 20:41
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

codeowner changes lgtm

@gsoldevila gsoldevila force-pushed the move-kbn-core-to-src-core-packages branch from 0a5657f to d7e122e Compare December 23, 2024 08:33
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM (relocation of src/core/packages/core/server/integration_tests/config/check_dynamic_config.test.ts)

@gsoldevila gsoldevila force-pushed the move-kbn-core-to-src-core-packages branch from d7e122e to d9b8cfc Compare December 23, 2024 14:56
@gsoldevila gsoldevila force-pushed the move-kbn-core-to-src-core-packages branch from d9b8cfc to bb393a9 Compare December 23, 2024 16:45
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --include-path /api/dashboards --update'
@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 23, 2024

💔 Build Failed

Failed CI Steps

Metrics [docs]

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 38 2 -36
@kbn/core-analytics-browser-internal 0 1 +1
@kbn/core-application-browser-internal 0 3 +3
@kbn/core-apps-server-internal 0 1 +1
@kbn/core-base-browser-internal 0 2 +2
total -29
Unknown metric groups

References to deprecated APIs

id before after diff
@kbn/core 35 117 +82
@kbn/core-application-browser-internal 0 1 +1
@kbn/core-apps-server-internal 0 3 +3
total +86

History

Copy link

@l-suarez l-suarez left a comment

Choose a reason for hiding this comment

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

SCSS checked

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.