Read coreApp assets directly from package #144492

TinaHeiligers · 2022-11-02T23:31:05Z

Handles the follow-up work from #144075
Part of #134112

In #144075, we had to leave duplicates of the assets declared in coreApp within src/core/server/core_app because these assets were imported or referenced directly elsewhere.

This PR removes these duplicates and exposes the assets directly from the package.

References updated:

canvas shareable_runtime

The changes involved:

moving the assets folder out of the package src folder with corresponding updates to the Bazel build file
updating the references to these assets, and
changes to dev build scripts, including moving the ReplaceFavicon build task to after empty directories are created (thanks @mistic!).

…ssets, export post-build paths to assets

… page to read assets from new location

…app's assets

pgayvallet · 2022-11-03T07:10:09Z

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

+ASSETS = glob(["assets/**/*"])
+
+filegroup(
+  name = "assets",
+  srcs = ASSETS,
+)
+
 NPM_MODULE_EXTRA_FILES = [
  "package.json",
+  ":assets"
 ]


So, what does that do exactly?

But I think I spotted the problem:

This PR moved the assets from {package}/src/assets to {package}/assets. However you did not change the path in the route registered to serve them:

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

Lines 205 to 206 in 0ba81e1

private registerStaticDirs(core: InternalCoreSetup | InternalCorePreboot) {

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

Changing the Path.resolve(__dirname, './assets') to target the correct folder should hopefully do the trick.

TinaHeiligers · 2022-11-03T16:54:26Z

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

-    core.http.registerStaticDir('/ui/{path*}', Path.resolve(__dirname, './assets'));
+    core.http.registerStaticDir(
+      '/ui/{path*}',
+      fromRoot('node_modules/@kbn/core-apps-server-internal/assets')


We need to load these assets from their location post-build.

TinaHeiligers · 2022-11-03T16:56:31Z

src/dev/build/build_distributables.ts

    await run(Tasks.CreateEmptyDirsAndFiles);
    await run(Tasks.CreateReadme);
    await run(Tasks.BuildBazelPackages);
+    await run(Tasks.ReplaceFavicon);


The build process expects an empty directory into which to replace the distributable versions of favicons with their source. At least, that's how I understand the flow here.

TinaHeiligers · 2022-11-03T16:57:09Z

src/dev/build/tasks/copy_source_task.ts

@@ -33,7 +33,6 @@ export const CopySource: Task = {
      '!src/**/mocks.{js,ts}',
      '!src/cli*/dev.js',
      '!src/plugins/telemetry/schema/**',
-      '!src/core/server/core_app/assets/favicons/favicon.distribution.{ico,png,svg}',


These files moved and the path doesn't exist anymore.

TinaHeiligers · 2022-11-03T16:58:01Z

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

@@ -6,7 +6,7 @@
 */

 export * from './api';
-import '@kbn/core/server/core_app/assets/legacy_light_theme.css';
+import '@kbn/core-apps-server-internal/assets/legacy_light_theme.css';


Updates path to assets

mind if we add the ci:build-canvas-shareable-runtime label to test this?

I don't mind. I've added the label, and kicked off another run.
@jbudz FMI, what does that label setup and run given that the label needs to be added explicitly? The only reference I can find is here and I'm interpreting that to mean we don't build the canvas shareable runtime by default. Is that correct?

@jbudz after adding the flag, the ci run fails on "canvas shareable runtime" for "total size" is 192390 over the configured limit of 8200000". commit:9471f5c
The only thing this PR does in the canvas shareable_runtime module is to update the location from which the legacy_light_theme stylesheet is read.
From what I can see, the theme itself is 100Kb and the minimized version is sitting at 79Kb, and don't appear to have changed in size from what's in main right now.

Is there anything I need to do here? Should we be updating the limit?

LGTM. The runtime isn't built in PR's anymore by default because it adds 3-4 minutes to build times and doesn't have any tests. The webpack config doesn't change too often so we were okay with the tradeoff.

Limit looks like it's something for ops to triage, that's about expected.

elasticmachine · 2022-11-03T18:16:30Z

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

TinaHeiligers · 2022-11-03T18:23:00Z

@elasticmachine merge upstream

Heenawter

Code only review - import change LGTM 👍

TinaHeiligers · 2022-11-03T20:44:01Z

@elasticmachine merge upstream

TinaHeiligers · 2022-11-03T22:48:15Z

@elasticmachine merge upstream

kibana-ci · 2022-11-03T23:47:42Z

💚 Build Succeeded

Buildkite Build
Commit: 803eccc

Metrics [docs]

Unknown metric groups

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`	108	113	+5
`securitySolution`	440	446	+6
total			+19

Total ESLint disabled count

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

History

💔 Build #85298 failed 9471f5c
💚 Build #85254 succeeded 09c99c2
💚 Build #85211 succeeded d0213c3
💔 Build #84941 failed 4b0f296
💔 Build #84939 failed c862640
💔 Build #84936 failed 2faf6e6

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

pgayvallet

LGTM

TinaHeiligers added 6 commits November 2, 2022 14:39

Moves assets to root of package, update bazel build file to extract a…

72d225c

…ssets, export post-build paths to assets

Updates dev build tasks and precommit hook

6ccc2a8

Removes duplicate subset of assets from core

58dfd1a

Updates path to legacy_light_theme css in canvas shareable_runtime

70a15ca

exports path to assets from core-app package, updates security prompt…

e15b8b2

… page to read assets from new location

Updates jest unit test snapshots to account for new location of core …

2faf6e6

…app's 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 labels Nov 2, 2022

TinaHeiligers added 3 commits November 2, 2022 16:34

restores prompt_page

67023d0

restores snapshots from reverted cahnges in prompt_page

c862640

Moves ReplaceFavicon task to after BuilBazelPackages

4b0f296

pgayvallet reviewed Nov 3, 2022

View reviewed changes

TinaHeiligers added 3 commits November 3, 2022 08:25

Servers assets directly from node modules after the package is built

c8e9380

Updates registerStaticDirs to read from built-location of assets

3274c9b

Removes unused exported constants

d0213c3

TinaHeiligers commented Nov 3, 2022

View reviewed changes

TinaHeiligers added the v8.6.0 label Nov 3, 2022

TinaHeiligers marked this pull request as ready for review November 3, 2022 18:16

TinaHeiligers requested review from a team as code owners November 3, 2022 18:16

Merge branch 'main' into kbn-read-core-app-assets-from-package

09c99c2

Heenawter approved these changes Nov 3, 2022

View reviewed changes

TinaHeiligers added the ci:build-canvas-shareable-runtime label Nov 3, 2022

Merge branch 'main' into kbn-read-core-app-assets-from-package

9471f5c

TinaHeiligers removed the ci:build-canvas-shareable-runtime label Nov 3, 2022

Merge branch 'main' into kbn-read-core-app-assets-from-package

803eccc

pgayvallet approved these changes Nov 4, 2022

View reviewed changes

jbudz approved these changes Nov 4, 2022

View reviewed changes

TinaHeiligers merged commit b5ee95d into elastic:main Nov 4, 2022

TinaHeiligers deleted the kbn-read-core-app-assets-from-package branch November 4, 2022 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read coreApp assets directly from package #144492

Read coreApp assets directly from package #144492

TinaHeiligers commented Nov 2, 2022 •

edited

Loading

pgayvallet Nov 3, 2022

TinaHeiligers Nov 3, 2022

TinaHeiligers Nov 3, 2022

TinaHeiligers Nov 3, 2022

TinaHeiligers Nov 3, 2022

jbudz Nov 3, 2022

TinaHeiligers Nov 3, 2022 •

edited

Loading

TinaHeiligers Nov 3, 2022 •

edited

Loading

jbudz Nov 4, 2022 •

edited

Loading

jbudz Nov 4, 2022

elasticmachine commented Nov 3, 2022

TinaHeiligers commented Nov 3, 2022

Heenawter left a comment

TinaHeiligers commented Nov 3, 2022

TinaHeiligers commented Nov 3, 2022

kibana-ci commented Nov 3, 2022

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

pgayvallet left a comment

	private registerStaticDirs(core: InternalCoreSetup \| InternalCorePreboot) {
	core.http.registerStaticDir('/ui/{path*}', Path.resolve(__dirname, './assets'));

Read coreApp assets directly from package #144492

Read coreApp assets directly from package #144492

Conversation

TinaHeiligers commented Nov 2, 2022 • edited Loading

References updated:

The changes involved:

pgayvallet Nov 3, 2022

Choose a reason for hiding this comment

TinaHeiligers Nov 3, 2022

Choose a reason for hiding this comment

TinaHeiligers Nov 3, 2022

Choose a reason for hiding this comment

TinaHeiligers Nov 3, 2022

Choose a reason for hiding this comment

TinaHeiligers Nov 3, 2022

Choose a reason for hiding this comment

jbudz Nov 3, 2022

Choose a reason for hiding this comment

TinaHeiligers Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

jbudz Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

jbudz Nov 4, 2022

Choose a reason for hiding this comment

elasticmachine commented Nov 3, 2022

TinaHeiligers commented Nov 3, 2022

Heenawter left a comment

Choose a reason for hiding this comment

TinaHeiligers commented Nov 3, 2022

TinaHeiligers commented Nov 3, 2022

kibana-ci commented Nov 3, 2022

💚 Build Succeeded

Metrics [docs]

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

pgayvallet left a comment

Choose a reason for hiding this comment

TinaHeiligers commented Nov 2, 2022 •

edited

Loading

TinaHeiligers Nov 3, 2022 •

edited

Loading

TinaHeiligers Nov 3, 2022 •

edited

Loading

jbudz Nov 4, 2022 •

edited

Loading