Serve static assets from NP #60490
Merged
Serve static assets from NP #60490
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mshustov
added
Team:Core
|
Pinging @elastic/kibana-platform (Team:Platform) |
mshustov
commented
Mar 18, 2020
| @@ -217,6 +218,7 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS | |||
| if (plugin.includesUiPlugin) { | |||
| this.uiPluginInternalInfo.set(plugin.name, { | |||
| publicTargetDir: Path.resolve(plugin.path, 'target/public'), | |||
| publicAssetsDir: Path.resolve(plugin.path, 'public/assets'), | |||
There was a problem hiding this comment.
LP exposes the whole folder https://github.com/elastic/kibana/pull/60490/files#diff-15c0a3d71c405cd45e19b0114aecb64eR76
mshustov
commented
Mar 18, 2020
| @@ -52,7 +52,7 @@ export default function(kibana: any) { | |||
| namespace, | |||
| }); | |||
| return { success: true }; | |||
| }, | |||
| }) as Hapi.Lifecycle.Method, | |||
There was a problem hiding this comment.
Hapi doesn't allow to define a type for request payload DefinitelyTyped/DefinitelyTyped#25605
There was a problem hiding this comment.
I did not see hapi package upgrade in package.json. Why was these Hapi.Lifecycle.Method additions needed?
There was a problem hiding this comment.
no idea to be honest, I suspect adding h2o2 & inert types affects hapi somehow, since they both extend hapi interfaces.
mshustov
commented
Mar 18, 2020
| protocol: request.server.info.protocol, | ||
| pathname: `${this.httpConfig.basePath}/${request.params.kbnPath}`, | ||
| query: request.query, | ||
| mapUri: (request: Request) => |
mshustov
commented
Mar 18, 2020
| /** | ||
| * Path to the plugin assets directory. | ||
| */ | ||
| readonly publicAssetsDir: string; |
There was a problem hiding this comment.
I don't think we want to server build artifacts from the /assets folder, so I separated them.
4 tasks
spalger
reviewed
Mar 18, 2020
spalger
reviewed
Mar 19, 2020
pgayvallet
approved these changes
Mar 20, 2020
| @@ -49,4 +53,12 @@ export class CoreApp { | |||
| res.ok({ body: { version: '0.0.1' } }) | |||
| ); | |||
| } | |||
| private registerStaticDirs(coreSetup: InternalCoreSetup) { | |||
| coreSetup.http.registerStaticDir('/ui/{path*}', Path.resolve(__dirname, './assets')); | |||
There was a problem hiding this comment.
I don't think we should expose this API directly to the plugins. Otherwise, any plugin can expose an arbitrary folder (config folder with kibana.yml, for example)
We could have an API to only allow to serve from within the plugin's base folder. That would avoid the risk of exposing files from outside their folder.
i.e
// src/core/plugins/my_plugin/server/plugin.ts
coreSetup.http.registerStaticDir('/my-plugin-data/{path*}', 'my_assets_folder');that would serve the src/core/plugins/my_plugin/my_assets_folder (or maybe src/core/plugins/my_plugin/server/my_assets_folder, need to decide)
I know this PR doesn't yet expose this to plugins directly, but I wonder if there's really any value into allowing/requiring plugins to add the {path*}
I agree, that doesn't seems necessary imho
Previous proposal example should probably be
coreSetup.http.registerStaticDir('/my-plugin-data', 'my_assets_folder');I think that if there is a real use case to only serving files of a certain directory depth, this should be exposed more explicitly via an option to registerStaticDir
Don't really see any obvious need for that. KISS imho.
Comment on lines
+59
to
+62
| coreSetup.http.registerStaticDir( | ||
| '/node_modules/@kbn/ui-framework/dist/{path*}', | ||
| fromRoot('node_modules/@kbn/ui-framework/dist') | ||
| ); |
There was a problem hiding this comment.
Wow. We sure were doing fun things.
Comment on lines
+264
to
+267
| deps.http.registerStaticDir( | ||
| `/plugins/${pluginName}/assets/{path*}`, | ||
| pluginInfo.publicAssetsDir | ||
| ); |
There was a problem hiding this comment.
Is that a temporary measure to allow plugins to move their static assets to NP plugins until #50654 lands? Didn't we want to make all assets exposition explicit? Or is this going to be permanent to allow client-only plugins to expose assets?
| @@ -52,7 +52,7 @@ export default function(kibana: any) { | |||
| namespace, | |||
| }); | |||
| return { success: true }; | |||
| }, | |||
| }) as Hapi.Lifecycle.Method, | |||
There was a problem hiding this comment.
I did not see hapi package upgrade in package.json. Why was these Hapi.Lifecycle.Method additions needed?
mshustov
commented
Mar 20, 2020
| @@ -33,7 +33,7 @@ export default function createActionTests({ getService }: FtrProviderContext) { | |||
| }, | |||
| }); | |||
|
|
|||
| expect(response.statusCode).to.eql(200); | |||
| expect(response.status).to.eql(200); | |||
There was a problem hiding this comment.
That should be in a separate PR, but I don't want to spend more time on it.
joshdover
approved these changes
Mar 26, 2020
| @@ -49,4 +53,12 @@ export class CoreApp { | |||
| res.ok({ body: { version: '0.0.1' } }) | |||
| ); | |||
| } | |||
| private registerStaticDirs(coreSetup: InternalCoreSetup) { | |||
| coreSetup.http.registerStaticDir('/ui/{path*}', Path.resolve(__dirname, './assets')); | |||
There was a problem hiding this comment.
If we don't need to expose to plugins, then as-is is good with me 👍
mshustov
added a commit
to mshustov/kibana
that referenced
this pull request
Mar 27, 2020
* add hapi.inert plugin to NP * update tests * move serving static assets * update tests * add functional tests * fix type errors. Hapi.Request doesn't support typings for payload * update docs * remove comment * move assets to NP * update all assets references * address Spencer's comments * move ui settings migration to migration examples * document legacy plugin spec * move platform assets test to integration_tests * address Spencer's comment p.2 * try to fix type errors * fix merge commit * update tests
mshustov
added a commit
that referenced
this pull request
Mar 27, 2020
* add hapi.inert plugin to NP * update tests * move serving static assets * update tests * add functional tests * fix type errors. Hapi.Request doesn't support typings for payload * update docs * remove comment * move assets to NP * update all assets references * address Spencer's comments * move ui settings migration to migration examples * document legacy plugin spec * move platform assets test to integration_tests * address Spencer's comment p.2 * try to fix type errors * fix merge commit * update tests
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/dashboard_mode/dashboard_view_mode·js.dashboard mode Dashboard View Mode "before all" hook: initialize tests in "Dashboard View Mode"Standard OutStack TraceKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/machine_learning/feature_controls/ml_security·ts.machine learning feature controls security machine_learning_user and global all shows ML navlinkStandard OutStack TraceKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/machine_learning/feature_controls/ml_security·ts.machine learning feature controls security machine_learning_user and global all shows ML navlinkStandard OutStack Traceand 4 more failures, only showing the first 3. History
To update your PR or re-run it, just comment with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Part of #50654
Changes:
src/legacy/ui/public/assets-->src/core/server/core_app/assets/src/core/server/core_app/assets//public/assetsfolderChecklist
For maintainers
Dev docs
Kibana platform serves plugin static assets from the
my_plugin/public/assetsfolder. No additional configuration is required.