-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Serve static assets from NP #60490
Serve static assets from NP #60490
Conversation
Pinging @elastic/kibana-platform (Team:Platform) |
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LP exposes the whole folder https://github.com/elastic/kibana/pull/60490/files#diff-15c0a3d71c405cd45e19b0114aecb64eR76
@@ -52,7 +52,7 @@ export default function(kibana: any) { | |||
namespace, | |||
}); | |||
return { success: true }; | |||
}, | |||
}) as Hapi.Lifecycle.Method, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hapi doesn't allow to define a type for request payload DefinitelyTyped/DefinitelyTyped#25605
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no idea to be honest, I suspect adding h2o2
& inert
types affects hapi
somehow, since they both extend hapi
interfaces.
protocol: request.server.info.protocol, | ||
pathname: `${this.httpConfig.basePath}/${request.params.kbnPath}`, | ||
query: request.query, | ||
mapUri: (request: Request) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed type errors
/** | ||
* Path to the plugin assets directory. | ||
*/ | ||
readonly publicAssetsDir: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to server build artifacts from the /assets
folder, so I separated them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alerting changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions, overall looking good to me in current state.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
coreSetup.http.registerStaticDir( | ||
'/node_modules/@kbn/ui-framework/dist/{path*}', | ||
fromRoot('node_modules/@kbn/ui-framework/dist') | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. We sure were doing fun things.
deps.http.registerStaticDir( | ||
`/plugins/${pluginName}/assets/{path*}`, | ||
pluginInfo.publicAssetsDir | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not see hapi
package upgrade in package.json
. Why was these Hapi.Lifecycle.Method
additions needed?
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be in a separate PR, but I don't want to spend more time on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Canvas changes lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operations: LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SIEM changes LGTM! 👍
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't need to expose to plugins, then as-is is good with me 👍
* 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
* 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 Out
Stack Trace
Kibana 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 Out
Stack Trace
Kibana 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 Out
Stack Trace
and 4 more failures, only showing the first 3. History
To update your PR or re-run it, just comment with: |
Summary
Part of #50654
Changes:
src/legacy/ui/public/assets
-->src/core/server/core_app/assets/
src/core/server/core_app/assets/
/public/assets
folderChecklist
For maintainers
Dev docs
Kibana platform serves plugin static assets from the
my_plugin/public/assets
folder. No additional configuration is required.