-
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
Sustainable Kibana Architecture: Remove dependencies between plugins that are related by _App Links_ #199492
Sustainable Kibana Architecture: Remove dependencies between plugins that are related by _App Links_ #199492
Conversation
@@ -16,7 +16,7 @@ export const SELECTED_CONNECTOR_LOCAL_STORAGE_KEY = | |||
|
|||
export function SearchConnectorTab() { | |||
const { application } = useKibana().services; | |||
const url = application.getUrlForApp('enterprise_search', { path: '/content/connectors' }); | |||
const url = application.getUrlForApp('enterpriseSearch', { path: '/content/connectors' }); |
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.
Am I wrong or this URL was broken?
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 think you've got the right appId https://github.com/elastic/kibana/blob/main/packages/deeplinks/search/constants.ts#L10
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.
This was briefly broken last week in the O11y Solution nav because enterprise search was disabled, but we fixed that and the link worked when I checked. But you're right that the appId should be enterpriseSearch
so that is very confusing 🤔
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 confirm this is fixed externally to this PR too (noticed when rebasing).
@@ -16,7 +16,7 @@ export const SELECTED_CONNECTOR_LOCAL_STORAGE_KEY = | |||
|
|||
export function SearchConnectorTab() { | |||
const { application } = useKibana().services; | |||
const url = application.getUrlForApp('enterprise_search', { path: '/content/connectors' }); | |||
const url = application.getUrlForApp('enterpriseSearch', { path: '/content/connectors' }); |
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 think you've got the right appId https://github.com/elastic/kibana/blob/main/packages/deeplinks/search/constants.ts#L10
@@ -114,6 +114,14 @@ export interface ApplicationStart { | |||
*/ | |||
navigateToUrl(url: string, options?: NavigateToUrlOptions): Promise<void>; | |||
|
|||
/** |
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: it might be useful to mention in the tsdocs of navigateToApp
something like:
If a plugin is disabled any applications it registers won't be available either. Before rendering a UI element that a user could use to navigate to another application, first check if the destination application is actually available using the
isAppRegistered
API.
0e6742f
to
9ef9cfa
Compare
Pinging @elastic/kibana-core (Team:Core) |
Pinging @elastic/obs-ai-assistant (Team:Obs AI Assistant) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
1771ae8
to
6c57443
Compare
6c57443
to
cb54b2c
Compare
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.
response-ops
ok 👍
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.
Changes in Data Discovery mocks LGTM
cb54b2c
to
29177a9
Compare
// Could be categorised as Search in the future, but it currently needs to run in Observability too | ||
"group": "platform", | ||
"visibility": "shared", | ||
"group": "search", |
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.
Here I believe I went too far, I made enterpriseSearch
part of "Search" solution only.
This will make observability-ai-assistant-management
menu entry totally useless when we have independent solution builds.
However, if we rollback the changes, we have a domino effect that forces us to expose searchPlayground
as platform/shared
too.
@sphilipse WDYT?
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.
Yeah, I think we need to make this platform/shared for now. Exposing playground as platform/shared is fine for now IMO. We have open tickets to separate this stuff and implement better RBAC which will eventually fix all of these problems. (we need to do that for our ent-search deprecation anyway)
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.
This will make observability-ai-assistant-management menu entry totally useless when we have independent solution builds.
Sorry, what does this mean?
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.
The observability-ai-assistant-management
has a link to the searchPlayground
.
This creates a dependency O11y
=>Search
that we are solving my making searchPlayground
platform/shared for now.
I rollbacked the change at the root of this discussion in 0aa726b
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.
@dgieselaar could you PTAL? TIA
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.
As discussed, we can keep the conflicting search
plugins as search/private for now.
These plugins need to be refactored and external dependencies issues addressed before solution-specific builds.
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.
Fleet change 🚀
29177a9
to
dbc531b
Compare
enterprise-search
part of Search solutiondbc531b
to
eba5431
Compare
💔 Build Failed
Failed CI StepsMetrics [docs]
History
cc @gsoldevila |
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/11836591000 |
…that are related by _App Links_ (elastic#199492) ## Summary This PR introduces a Core API to check whether a given application has been registered. Plugins can use this for their _App Links_, without having to depend on the referenced plugin(s) anymore. This way, we can get rid of some inter-solution dependencies, and categorise plugins more appropriately. --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit ad56ec5)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…ugins that are related by _App Links_ (#199492) (#200159) # Backport This will backport the following commits from `main` to `8.x`: - [Sustainable Kibana Architecture: Remove dependencies between plugins that are related by _App Links_ (#199492)](#199492) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Gerard Soldevila","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-14T11:49:21Z","message":"Sustainable Kibana Architecture: Remove dependencies between plugins that are related by _App Links_ (#199492)\n\n## Summary\r\n\r\nThis PR introduces a Core API to check whether a given application has\r\nbeen registered.\r\nPlugins can use this for their _App Links_, without having to depend on\r\nthe referenced plugin(s) anymore.\r\n\r\nThis way, we can get rid of some inter-solution dependencies, and\r\ncategorise plugins more appropriately.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"ad56ec5f1a838486d9b38c2d5aa92bbf77e127a3","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Core","release_note:skip","v9.0.0","backport:prev-minor","Team:Obs AI Assistant","ci:project-deploy-observability"],"title":"Sustainable Kibana Architecture: Remove dependencies between plugins that are related by _App Links_","number":199492,"url":"https://github.com/elastic/kibana/pull/199492","mergeCommit":{"message":"Sustainable Kibana Architecture: Remove dependencies between plugins that are related by _App Links_ (#199492)\n\n## Summary\r\n\r\nThis PR introduces a Core API to check whether a given application has\r\nbeen registered.\r\nPlugins can use this for their _App Links_, without having to depend on\r\nthe referenced plugin(s) anymore.\r\n\r\nThis way, we can get rid of some inter-solution dependencies, and\r\ncategorise plugins more appropriately.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"ad56ec5f1a838486d9b38c2d5aa92bbf77e127a3"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/199492","number":199492,"mergeCommit":{"message":"Sustainable Kibana Architecture: Remove dependencies between plugins that are related by _App Links_ (#199492)\n\n## Summary\r\n\r\nThis PR introduces a Core API to check whether a given application has\r\nbeen registered.\r\nPlugins can use this for their _App Links_, without having to depend on\r\nthe referenced plugin(s) anymore.\r\n\r\nThis way, we can get rid of some inter-solution dependencies, and\r\ncategorise plugins more appropriately.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"ad56ec5f1a838486d9b38c2d5aa92bbf77e127a3"}}]}] BACKPORT--> Co-authored-by: Gerard Soldevila <[email protected]>
…that are related by _App Links_ (elastic#199492) ## Summary This PR introduces a Core API to check whether a given application has been registered. Plugins can use this for their _App Links_, without having to depend on the referenced plugin(s) anymore. This way, we can get rid of some inter-solution dependencies, and categorise plugins more appropriately. --------- Co-authored-by: kibanamachine <[email protected]>
…that are related by _App Links_ (elastic#199492) ## Summary This PR introduces a Core API to check whether a given application has been registered. Plugins can use this for their _App Links_, without having to depend on the referenced plugin(s) anymore. This way, we can get rid of some inter-solution dependencies, and categorise plugins more appropriately. --------- Co-authored-by: kibanamachine <[email protected]>
Summary
This PR introduces a Core API to check whether a given application has been registered.
Plugins can use this for their App Links, without having to depend on the referenced plugin(s) anymore.
This way, we can get rid of some inter-solution dependencies, and categorise plugins more appropriately.