IDM: Migrate core server-side core_app to package #144075

TinaHeiligers · 2022-10-27T00:03:03Z

Create package:
@kbn/core-apps-server-internal

Notes:

I couldn't find a public mock for the core_app service class, hence no corresponding mock package.
src/core/server/core_app contains assets included in the bundled routes. Following how I understood the inner workings of bundling and serving these assets, I tried moving the files to the package and updating the build scripts to account for the new paths. I also had to update the import path for the legacy_light_theme.css in canvas. That didn't work, so I had to duplicate the files and leave a subset of the assets in src/core.

…config to include more file types for the assets

TinaHeiligers

As mentioned in the PR description, I took the best guess at handling the assets, while leaving a copy of them in src/core/server/core_app.

TinaHeiligers · 2022-10-27T00:03:48Z

packages/core/apps/core-apps-server-internal/BUILD.bazel

+SOURCE_FILES = glob(
+  [
+    "**/*.ts",
+    "src/**/*.css",


On a whim, I took the same approach as previously when handling code that includes stylesheets.

TinaHeiligers · 2022-10-27T00:04:42Z

packages/core/apps/core-apps-server-internal/index.ts

@@ -6,4 +6,4 @@
 * Side Public License, v 1.
 */

-export { CoreApp } from './core_app';
+export { CoreAppsService } from './src';


Changed the service class name to keep consistent with the browser-side service class counterpart.

TinaHeiligers · 2022-10-27T00:06:44Z

packages/core/apps/core-apps-server-internal/src/bundle_routes/register_bundle_routes.ts

 import { fromRoot } from '@kbn/utils';
 import UiSharedDepsNpm from '@kbn/ui-shared-deps-npm';
-import * as UiSharedDepsSrc from '@kbn/ui-shared-deps-src';
+import { distDir as UiSharedDepsSrcDistDir } from '@kbn/ui-shared-deps-src';


import * as ... doesn't work in packages. I took the approach of renaming the specific dependency.

…s suplicated in package

…ore-apps-server-internal

TinaHeiligers · 2022-10-27T22:46:38Z

@elasticmachine merge upstream

TinaHeiligers · 2022-10-28T01:05:29Z

packages/core/apps/core-apps-server-internal/index.ts

+
+export { CoreAppsService } from './src';
+export {
+  registerBundleRoutes,


These are needed for the integration tests in src/core/server/integration_tests.
We can decide as a team how to handle the integration tests during the migration cleanup phase. The pending tooling changes should give us a bit more freedom in package structure and inteneral dependencies.

NIT: In similar cases for other packages, I added a comment to 'separate' the 'real' imports from the integration-test-only imports.

E.g:

kibana/packages/core/saved-objects/core-saved-objects-server-internal/index.ts

Lines 17 to 22 in 92ca42f

// only used by integration tests

export { registerDeleteUnknownTypesRoute } from './src/routes/deprecations';

export { registerLegacyExportRoute } from './src/routes/legacy_import_export/export';

export { registerLegacyImportRoute } from './src/routes/legacy_import_export/import';

export { registerBulkCreateRoute } from './src/routes/bulk_create';

export { registerBulkGetRoute } from './src/routes/bulk_get';

TinaHeiligers · 2022-10-28T01:09:36Z

packages/core/apps/core-apps-server-internal/BUILD.bazel

+SOURCE_FILES = glob(
+  [
+    "**/*.ts",
+    "src/**/*.css",


@elastic/kibana-operations is there a trick to serving the assets directly from the package so that we don't need to keep them in src/core?

WDYM by 'serving' here exactly?

To my knowledge, there are two usages of these files:

The correct one:

kibana/src/core/server/core_app/core_app.ts

Line 206 in 8176ac7

core.http.registerStaticDir('/ui/{path*}', Path.resolve(__dirname, './assets'));

which should work out of the box when the assets are moved in the package (right?)

And that scss import, that I didn't even knew about:

kibana/x-pack/plugins/canvas/shareable_runtime/index.ts

Line 9 in bebec37

import '@kbn/core/server/core_app/assets/legacy_light_theme.css';

You're referring to the second one here, right?

yes, I'm referring to the second one.

TinaHeiligers · 2022-10-28T01:10:36Z

packages/core/apps/core-apps-server-internal/index.ts

+export type {
+  InternalCoreAppsServiceRequestHandlerContext,
+  InternalCoreAppsServiceRouter,
+} from './src';


@elastic/kibana-operations how would we expose the assets from this package?

TinaHeiligers · 2022-10-28T01:12:53Z

packages/core/apps/core-apps-server-internal/src/core_app.test.ts

-  let coreApp: CoreApp;
-  let internalCorePreboot: ReturnType<typeof coreMock.createInternalPreboot>;
+  let coreApp: CoreAppsService;
+  let internalCorePreboot: ReturnType<typeof coreInternalLifecycleMock.createInternalPreboot>;


As of #141891, core's internal lifecycles are exposed in a package, so we can use those directly.

src/dev/build/tasks/copy_source_task.ts

TinaHeiligers

Adds more self review comments

TinaHeiligers · 2022-10-28T01:20:22Z

@elasticmachine merge upstream

…-fix'

TinaHeiligers · 2022-10-29T00:16:09Z

@elasticmachine merge upstream

TinaHeiligers

remove location from dev script

src/dev/build/tasks/copy_source_task.ts

src/dev/precommit_hook/casing_check_config.js

pgayvallet

Implementation looks good.

I'd like to have a better understanding of what the issue with the assets is (like, are we forced to duplicate all assets or not) before definitive approval:

pgayvallet · 2022-11-01T07:03:44Z

packages/core/apps/core-apps-server-internal/BUILD.bazel

+SOURCE_FILES = glob(
+  [
+    "**/*.ts",
+    "src/**/*.css",


WDYM by 'serving' here exactly?

To my knowledge, there are two usages of these files:

The correct one:

kibana/src/core/server/core_app/core_app.ts

Line 206 in 8176ac7

core.http.registerStaticDir('/ui/{path*}', Path.resolve(__dirname, './assets'));

which should work out of the box when the assets are moved in the package (right?)

And that scss import, that I didn't even knew about:

kibana/x-pack/plugins/canvas/shareable_runtime/index.ts

Line 9 in bebec37

import '@kbn/core/server/core_app/assets/legacy_light_theme.css';

You're referring to the second one here, right?

pgayvallet · 2022-11-01T07:05:36Z

packages/core/apps/core-apps-server-internal/index.ts

+
+export { CoreAppsService } from './src';
+export {
+  registerBundleRoutes,


NIT: In similar cases for other packages, I added a comment to 'separate' the 'real' imports from the integration-test-only imports.

E.g:

kibana/packages/core/saved-objects/core-saved-objects-server-internal/index.ts

Lines 17 to 22 in 92ca42f

// only used by integration tests

export { registerDeleteUnknownTypesRoute } from './src/routes/deprecations';

export { registerLegacyExportRoute } from './src/routes/legacy_import_export/export';

export { registerLegacyImportRoute } from './src/routes/legacy_import_export/import';

export { registerBulkCreateRoute } from './src/routes/bulk_create';

export { registerBulkGetRoute } from './src/routes/bulk_get';

pgayvallet · 2022-11-01T07:10:00Z

packages/core/apps/core-apps-server-internal/src/assets/fonts/roboto_mono/LICENSE.txt

+
+                                 Apache License
+                           Version 2.0, January 2004
+                        http://www.apache.org/licenses/
+


(commenting here because GH doesn't allow me to do it on font/binary files): related to my first comment: What's the issue with the assets exactly? Are we forced to duplicate all of them (meaning keeping them all under src/core/server/core_app/assets too), or is the issue just about the scss import of the src/core/server/core_app/assets/legacy_light_theme.css file from

kibana/x-pack/plugins/canvas/shareable_runtime/index.ts

Line 9 in bebec37

import '@kbn/core/server/core_app/assets/legacy_light_theme.css';

?

Because if that's the latest, I'd rather only keep the strict subset of required files in src/core/server/core_app/assets instead of duplicating them all.

Agreed, the only asset we need to keep for now is legacy_light_theme.css. We can follow up with changing the export at a later stage when we're forced to touch other teams' code.
Update, it turns out we need the favicons too.
Follow up WIP in #144375

pgayvallet · 2022-11-01T07:12:32Z

packages/core/apps/core-apps-server-internal/src/internal_types.ts

@@ -13,15 +13,15 @@ import type { UiSettingsRequestHandlerContext } from '@kbn/core-ui-settings-serv
 * Request handler context used by core's coreApp routes.
 * @internal
 */
-export interface InternalCoreAppRequestHandlerContext extends RequestHandlerContextBase {
+export interface InternalCoreAppsServiceRequestHandlerContext extends RequestHandlerContextBase {


This was internal, so the change wasn't strictly necessary, but 👍 for consistency.

TinaHeiligers · 2022-11-01T13:53:55Z

Implementation looks good.

I'd like to have a better understanding of what the issue with the assets is (like, are we forced to duplicate all assets or not) before definitive approval:

here

and here

On further investigation, the build scripts use the favicons in src/core/server/core_app (added here), and when I went through that PR I saw yet another reference to the favicons in their original location in core (security prompt_page).

For now, I'd prefer to keep the number of code-owners' reviews small while we finish our first pass at migrating core services to packages. we can clean all this up during the cleanup phase once we've had a chance to huddle with the @elastic/kibana-operations team.

I've began to attempt to change this in #144375

…ver/core_app

TinaHeiligers · 2022-11-01T23:08:17Z

@elasticmachine merge upstream

pgayvallet

LGTM given we have a follow-up created for my questions

TinaHeiligers · 2022-11-02T16:49:56Z

@elasticmachine merge upstream

mistic

Changes under operations team code owners LGTM

kibana-ci · 2022-11-02T17:53:47Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 13989b5

Failed CI Steps

FTR Configs #18

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-apps-server-internal`	-	8	+8

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-apps-server-internal`	-	1	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-apps-server-internal`	-	8	+8

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	58	64	+6
`osquery`	103	108	+5
`securitySolution`	440	446	+6
total			+19

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	66	72	+6
`osquery`	104	110	+6
`securitySolution`	517	523	+6
total			+20

History

💚 Build #84792 succeeded fe0cf8c
💚 Build #84528 succeeded 7fddac0
💚 Build #84498 succeeded 071301c
💚 Build #84468 succeeded 7afc72b
💔 Build #84452 failed 9f0513179394fb1ba7e30edb63af3b9e335daf02
💚 Build #83774 succeeded cab4632

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

TinaHeiligers added 5 commits October 26, 2022 12:19

change service class name for consistency

4b17361

Migrates lifecycle contract types off of coreMock

ba15bed

Creates package core-apps-server-internal

c13039c

updates dependencies in BAZEL config

81e9bd6

Moves a copy of the assets folder over to the package, updates BAZEL …

911c9ad

…config to include more file types for the assets

TinaHeiligers 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:skip This commit does not require backporting v8.6.0 labels Oct 27, 2022

TinaHeiligers commented Oct 27, 2022

View reviewed changes

TinaHeiligers and others added 8 commits October 26, 2022 17:28

exports from bundle_routes

be16ecf

Exports dependencies needed for integration test

ef67567

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

fd5db8a

Merge branch 'main' into kbn-135865-server-core-app-migration

2093f78

Updates casing_check_config to include the new path for core_app font…

f90f8d3

…s suplicated in package

Updates build files to account for new repo location of core_app assets

5d95b9f

Deletes src/core/server/core_app assets directory as duplicate from c…

fd17d2b

…ore-apps-server-internal

try importing css directly from package in canvas

10768ae

kibanamachine and others added 3 commits October 27, 2022 18:46

Merge branch 'main' into kbn-135865-server-core-app-migration

4bd53f0

Restores assets in src/core/core_app

9671923

restores build files to last green build

e756969

TinaHeiligers commented Oct 28, 2022

View reviewed changes

src/dev/build/tasks/copy_source_task.ts Outdated Show resolved Hide resolved

TinaHeiligers commented Oct 28, 2022

View reviewed changes

Merge branch 'main' into kbn-135865-server-core-app-migration

9a20901

kibanamachine and others added 3 commits October 28, 2022 21:52

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

2c38ea8

…-fix'

Merge branch 'main' of github.com:elastic/kibana into pr/144075

0e4a56a

migrate @kbn/core-apps-server-internal ts config

ca480ac

Merge branch 'main' into kbn-135865-server-core-app-migration

099f5ea

TinaHeiligers commented Oct 29, 2022

View reviewed changes

src/dev/build/tasks/copy_source_task.ts Outdated Show resolved Hide resolved

TinaHeiligers added 2 commits October 28, 2022 17:21

Update src/dev/build/tasks/copy_source_task.ts

dd1e0a1

Merge branch 'main' into kbn-135865-server-core-app-migration

cab4632

TinaHeiligers requested a review from spalger October 31, 2022 23:04

spalger reviewed Oct 31, 2022

View reviewed changes

src/dev/precommit_hook/casing_check_config.js Outdated Show resolved Hide resolved

pgayvallet reviewed Nov 1, 2022

View reviewed changes

TinaHeiligers added 2 commits November 1, 2022 10:33

Annotates exports for integration tests only, removes unused exports

65c4d1b

deletes fonts and unreferenced theme-related styles from src/core/ser…

7afc72b

…ver/core_app

TinaHeiligers force-pushed the kbn-135865-server-core-app-migration branch from 9f05131 to 7afc72b Compare November 1, 2022 19:09

Removes fonts directory from ignored file globs

071301c

Merge branch 'main' into kbn-135865-server-core-app-migration

7fddac0

TinaHeiligers mentioned this pull request Nov 1, 2022

Server core app import assets directly from the package #144375

Closed

pgayvallet approved these changes Nov 2, 2022

View reviewed changes

Merge branch 'main' into kbn-135865-server-core-app-migration

fe0cf8c

TinaHeiligers enabled auto-merge (squash) November 2, 2022 15:18

TinaHeiligers disabled auto-merge November 2, 2022 16:48

Merge branch 'main' into kbn-135865-server-core-app-migration

13989b5

mistic approved these changes Nov 2, 2022

View reviewed changes

TinaHeiligers merged commit 0ba81e1 into elastic:main Nov 2, 2022

TinaHeiligers deleted the kbn-135865-server-core-app-migration branch November 2, 2022 19:00

TinaHeiligers mentioned this pull request Nov 2, 2022

Read coreApp assets directly from package #144492

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IDM: Migrate core server-side core_app to package #144075

IDM: Migrate core server-side core_app to package #144075

TinaHeiligers commented Oct 27, 2022 •

edited

Loading

TinaHeiligers left a comment

TinaHeiligers Oct 27, 2022

TinaHeiligers Oct 27, 2022

TinaHeiligers Oct 27, 2022

TinaHeiligers commented Oct 27, 2022

TinaHeiligers Oct 28, 2022

pgayvallet Nov 1, 2022

TinaHeiligers Oct 28, 2022

pgayvallet Nov 1, 2022

TinaHeiligers Nov 1, 2022

TinaHeiligers Oct 28, 2022

TinaHeiligers Oct 28, 2022

TinaHeiligers left a comment

TinaHeiligers commented Oct 28, 2022

TinaHeiligers commented Oct 29, 2022

TinaHeiligers left a comment

pgayvallet left a comment •

edited

Loading

pgayvallet Nov 1, 2022

pgayvallet Nov 1, 2022

pgayvallet Nov 1, 2022

TinaHeiligers Nov 1, 2022 •

edited

Loading

pgayvallet Nov 1, 2022

TinaHeiligers commented Nov 1, 2022 •

edited

Loading

TinaHeiligers commented Nov 1, 2022

pgayvallet left a comment

TinaHeiligers commented Nov 2, 2022

mistic left a comment

kibana-ci commented Nov 2, 2022

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

	// only used by integration tests
	export { registerDeleteUnknownTypesRoute } from './src/routes/deprecations';
	export { registerLegacyExportRoute } from './src/routes/legacy_import_export/export';
	export { registerLegacyImportRoute } from './src/routes/legacy_import_export/import';
	export { registerBulkCreateRoute } from './src/routes/bulk_create';
	export { registerBulkGetRoute } from './src/routes/bulk_get';

IDM: Migrate core server-side core_app to package #144075

IDM: Migrate core server-side core_app to package #144075

Conversation

TinaHeiligers commented Oct 27, 2022 • edited Loading

Notes:

TinaHeiligers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers commented Oct 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

TinaHeiligers commented Oct 28, 2022

TinaHeiligers commented Oct 29, 2022

TinaHeiligers left a comment

Choose a reason for hiding this comment

pgayvallet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers commented Nov 1, 2022 • edited Loading

TinaHeiligers commented Nov 1, 2022

pgayvallet left a comment

Choose a reason for hiding this comment

TinaHeiligers commented Nov 2, 2022

mistic left a comment

Choose a reason for hiding this comment

kibana-ci commented Nov 2, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

TinaHeiligers commented Oct 27, 2022 •

edited

Loading

pgayvallet left a comment •

edited

Loading

TinaHeiligers Nov 1, 2022 •

edited

Loading

TinaHeiligers commented Nov 1, 2022 •

edited

Loading