-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Switch to core application service #63443
Switch to core application service #63443
Conversation
# Conflicts: # src/legacy/core_plugins/kibana/index.js # src/legacy/core_plugins/kibana/public/kibana.js # src/legacy/core_plugins/kibana/public/visualize/_index.scss # src/legacy/core_plugins/kibana/public/visualize/legacy_imports.ts # src/plugins/dashboard/public/index.ts # src/plugins/vis_default_editor/public/default_editor.tsx # src/plugins/vis_default_editor/public/default_editor_controller.tsx # src/plugins/visualize/public/kibana_services.ts # src/plugins/visualize/public/plugin.ts
…plication-service
…plication-service
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.
These are only SIEM changes and they've already been approved so 👍
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 retested (in Chrome, sorry), and the issues I raised previously have been fixed. I'd recommend (if you haven't already) to definitely check in IE to make sure the layouts still work.
Added a quick change which prevents a load screen when switching back to canvas from lens and visualize. This makes the issues in Canvas much more apparent, but also aligns better with the goals of this PR |
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.
Overall changes look good to me!
A lot of things were discussed/fixed offline.
Was mostly focused on testing transitions through Discover/Visualize/Dashboard/Home/Graph/Canvas/Management in Chrome browser.
Great work @flash1293 ! And a great step for Kibana 🎉
P.S.: Didn't look through >300 files, but quickly look at some points. Left the last nit, but feel free to forget it 😆
@@ -33,7 +33,7 @@ export default function({ getService, getPageObjects }: FtrProviderContext) { | |||
|
|||
describe('Print PDF button', () => { | |||
it('is not available if new', async () => { | |||
await PageObjects.common.navigateToUrl('visualize', 'new'); | |||
await PageObjects.common.navigateToUrl('visualize', 'new', { useActualUrl: true }); |
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.
nit: Would this work PageObjects.common.navigateToActualUrl('visualize', 'new')
?
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.
LGTM!
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
@elasticmachine merge upstream |
* upstream/master: (223 commits) [Ingest] Support root level yaml variables in agent stream template (elastic#66120) [Snapshot Restore] Fix error when deleting snapshots behind reverse proxy (elastic#66147) [Lens] fix empty state for pie (elastic#66206) [APM] Improve e2e tests (elastic#66373) [ML] Data Frame Analytics: Fix steps to be named phases. (elastic#65855) [Discover] Encode context link filter part (elastic#63831) [APM] Scope APM alert creation to environment (elastic#65681) Move Kibana Usage collectors outside the telemetry plugin (elastic#65663) [ML] Data Frame Analytics: Fix confusion matrix data grid width. (elastic#65818) Switch to core application service (elastic#63443) Removes use of prefer_v2_templates (elastic#66316) [Maps] handle case where fit to bounds does not match any documents (elastic#66307) log error instead of throw (elastic#66254) [plugin-helpers] remove outdated postinstall task (elastic#66324) Spaces - migrate default space and enter space view to KP (elastic#66098) [APM] Change plugin id for `apm_oss` to `apmOss` (elastic#66164) [Maps] convert map_selectors to TS (elastic#65905) [uptime/usage-collector] add missing await (elastic#66079) [Ingest] Add additional attributes to the Datasources Saved Object (elastic#66127) [Endpoint]EMT-339: add new policy response schema (elastic#66264) ...
Fixes #61304
Fixes #49060
This PR switches the Kibana app apps (Home, Lens, Visualize, Discover, Dashboard, Dev tools) from the legacy app renderer to the core application service.
Did you get here because your team got pinged and you are wondering what this is about?
This is not a rebase gone bad as it happens from time to time.
A lot of teams ended up in this PR because of changed links to the home application - all links change from
app/kibana#/home/...
toapp/home#/...
. If this is the case for your code area, then the only thing to do is to make sure these links still end up at the right place (if your app is already in the new platform, there should be no page load when going there).How to test this PR
This is a big PR that makes changes in a lot of different places. This means it's also not easy to test. This is a (non-exhaustive) list of things which can be used as starters, but be creative :)
dashboard_only_user
role should still workdefaultAppId
config key should still workdefaultRoute
ui setting should still workIn general, it helps to start two instances of Kibana (this PR and master) in parallel to see how they differentiate.
Notable changes
URL changes
This PR changes the URLs of some apps:
app/kibana#/home
->app/home#/
app/kibana#/discover
->app/discover#/
app/kibana#/visualize
->app/visualize#/
app/kibana#/dashboards
->app/dashboards#/list
app/kibana#/dev_tools
->app/dev_tools#/
app/kibana#/lens
->app/lens#/
For all apps, sub paths change by having the app name removed as part of the hash. The only exception doing other changes as well is the dashboards app, like discussed here: #49060
This PR does not change from hash history to browser history to keep the change manageable.
Old URLs should be forwarded automatically. The definitions for this are located in the plugins itself (like here: https://github.com/elastic/kibana/pull/63443/files#diff-673a132a1b2e5bef49aaefa30ee13899R76)
The majority of touched files is due to this change (functional test adjustments and crosslinking)
Page reload when going to management
As the management app is still located in the legacy platform, going from Dashboard/Discover/Visualize/Home to Management now causes a page reload.
Home logo
The home logo now uses the
navigateToApp
service to navigate to the home app because the old way of just using a native anchor tag would cause a full page reload.Scoped history in-app navigation
When triggering navigations within the same app (just changing the hash part of the url) using the scoped history object provided by core, the hash-change event is not triggered, causing angular routing, the kbn url tracker and state sync utilities to not pick up this change correctly.
This PR refactors the kbn url tracker to work with browser history instead of hash history objectsplugin.ts
https://github.com/elastic/kibana/pull/63443/files#diff-6ddbb6d7fc0fa22c821b2091a1d154ea
For the state sync utilities this change would be more difficult and for angular it's downright impossible to use the scoped history object, so
https://github.com/elastic/kibana/pull/63443/files#diff-82fbb8f394a64ab0b3d09519e72ea36fR150 makes sure that in apps requiring this behavior, a navigation change triggered by the global scoped history object is always triggering a hashchange event as well which is notifying the hash history objects and angular routing.
This behavior is actively used in two places: Clicking the nav link of the currently active app and using drilldowns on dashboards.
Telemetry access right probing
The telemetry plugin is requesting a saved object on app change which might not be available. If this is the case, the saved object request will fail. This becomes a problem when it is batched together with a saved object request of the app currently mounting, because that request will fail as well. This PR switches to the
bulkGet
api instead of the regularget
api to avoid the telemetry request getting batched with other requests.https://github.com/elastic/kibana/pull/63443/files#diff-422a281bf153a09aa4a2ab8b77df1867
Security behavior change
In the legacy
kibana
app, users would get redirected to home if their access rights wouldn't match the requested app. Now, as they are split out into regular new platform applications, the request fails already at the server side and just shows a technical 404 page: https://github.com/elastic/kibana/pull/63443/files#diff-3da7b4ce0de91b903189fde128a83a79R425Alias vis type interface change
When navigating to a visualization editor from the new vis modal, we can't just assign a new location because it would trigger a page load. Because of this, the vis alias type is now not featuring a
aliasUrl
, but analiasApp
and analiasPath
, to make it possible to trigger navigation usingnavigateToApp
https://github.com/elastic/kibana/pull/63443/files#diff-9eb2b74fb351fd8d9b9729cc8ac8c553
Edit panel navigation change
The edit panel action currently directly sets
window.location.href
. This causes a page load when the path is not the same as in the current url. To avoid this problem, this PR makes use of theeditPath
andeditApp
embeddable inputs to trigger a navigation without page reload instead.https://github.com/elastic/kibana/pull/63443/files#diff-dbb0b091a47e51c3022284d8e2ad4d70
"Back to dashboard" navigation change
To navigate back to the dashboard after editing an embeddable, lens and visualize used to mess with the dashboard url directly plucked from the navlink. This doesn't work anymore in the new platform. This PR moves this responsibility to the dashboard plugin. It exposes an API which takes the user back to the last edited dashboard while adding the embeddable to add to the URL.
This also allows us to get rid of the duplicated helpers for this.
Dashboard mode partial migration
Dashboard mode is slated to be removed with 8.0, but for this cutover it was necessary to migrate the server part to the new platform - otherwise it can't control the functionality and hide other navlinks. The server part of dashboard mode was changed to redirect to the NP app if the user has the dashboard only user role. The app restricts the UI as expected, migrates the path if necessary and then loads the actual dashboard app which handles the rest of the user experience.
New dashboard mode plugin: https://github.com/elastic/kibana/pull/63443/files#diff-ee10a7f4fda8f42acda3547c487ec3ab
Getting rid of redirect message helper
The redirect message helper was used to load the home page and show an error toast. It was only used in Graph to redirect away if the licensing level is not sufficient. As this navigation now happens without a page load, Graph can simply show the toast and then navigate to the home app using core services.
Temporary workaround for "not found" messages in management
The thing that could be removed for Graph now doesn't work anymore for the banners shown in management when the user gets redirected because there are no index patterns or a saved object couldn't be found when trying to access it in Visualize.
This PR implements a workaround for this by adding the message to the URL before redirecting. The legacy management app picks up the URL parameter and shows the banner
https://github.com/elastic/kibana/pull/63443/files#diff-024022209a9bd71018503285c51df1ca
https://github.com/elastic/kibana/pull/63443/files#diff-ceac0240dab178033ddad287293d7f11R62
This can get removed once management is also rendered in the new platform.
Temporary workaround for passing legacy settings to input control vis
The input control vis needs access to the legacy yml settings
kibana.autocompleteTerminateAfter
andkibana.autocompleteTimeout
. These are currently only exposed on the server side and can't use theexposeToBrowser
property to bring them to the client. To unblock this PR, I put a workaround in place which introduces a new API which exposes these settings. When the input control vis is used, it fetches these settings once and then operates on the cache. Once these settings are properly migrated (e.g. to the data plugin), this can be cleaned up by usingexposeToBrowser
and client side plugin contracts instead.https://github.com/elastic/kibana/pull/63443/files#diff-d7eb42f5ac5ddbbc41af5644c3138ba8R67
https://github.com/elastic/kibana/pull/63443/files#diff-42b6b53a2fe8aa6213120f242562b4ecR34
Setting doctitle when mounting an app
When switching between apps, the page reload often took care to correctly reset the title. This doesn't work anymore because a lot of app switches are now happening on the same page. All apps should take care of setting the title to the correct value when a navigation happens.
Using navigateToApp for home tiles to avoid page load
The tiles on the home page link to a lot of apps. To make sure
navigateToApp
is used when possible, this PR introduced a helper that checks the URL and usesnavigateToApp
when possible.https://github.com/elastic/kibana/pull/63443/files#diff-0a07343036159004454ec36157f72dd9
defaultAppId behavior
There is a deprecated yml setting
defaultAppId
which specifies the app kibana navigates to when the requested url is not matching anything else. To keep this behavior as close as possible, the helper implemented in https://github.com/elastic/kibana/pull/63443/files#diff-643a25a80e351c5303b4866205fd915b will take the current path, try to migrate it and navigate to the respective place.Remove discover link from context error message
When the context page can't load its anchor document, it will show a message to the user explaining the problem offering a link to go back to "the discover app". This link is fetched directly from the nav link which doesn't work anymore in the new platform. As context and discover are now the same app anyway and the back button has the same functionality, this navigation didn't seem relevant enough to implement a lot of code to fetch the last visited url. This PR simplifies the message and tells the user to just go back which seems sufficient for this rare edge case.
https://github.com/elastic/kibana/pull/63443/files#diff-a534ce79cd4b61e4ed069c191664b8b7R47
https://github.com/elastic/kibana/pull/63443/files#diff-4c23724f1b54556ac8f97bfb5bdf3e17L89
Create app wrapper styles for each app separately
In the current state on master, apps rely on the styling put in place by the local application service to be styled correctly. To avoid a dependency to legacy styling in each app, this PR creates separate stylesheets for the individual cases (e.g. https://github.com/elastic/kibana/pull/63443/files#diff-b3f4a09b020ab577358b8d3db8c0bc58R111 )
Migrating short URLs
Existing short URLs could still point to legacy URLs. Those would get forwarded fine, but cause a double page load - once for the legacy platform just to redirect to the new platform directly after. Unfortunately migrating the URLs directly to the new URL is not trivial because this information is not available during migration (as it happens before other plugins have the chance to register their forwarded URLs). To avoid making the short url migration directly aware of all existing URL schemes, this PR migrates all short urls pointing to the kibana app by moving them to the
url_migrate
app without changing the hash. This app will do the redirect normally happening in the legacy platform completelty in the new platform. https://github.com/elastic/kibana/pull/63443/files#diff-b9927d073469e0dfc479a59eac402cefAlso this redirect app will become handy once the legacy platform is turned off completely: #61308