Skip to content
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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Nov 14, 2024
Merged

Sustainable Kibana Architecture: Remove dependencies between plugins that are related by _App Links_ #199492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e562900
Make `enterprise-search` part of search solution
gsoldevila Nov 8, 2024
cae7a9e
Improve `navigateToApp` description
gsoldevila Nov 9, 2024
2f1ec8f
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine Nov 9, 2024
535a789
Add unit tests for `isAppRegistered`
gsoldevila Nov 9, 2024
f8ac56d
Fix type check issues
gsoldevila Nov 11, 2024
6e1cd1f
Keep enterprise_search and search_playground as platform for now
gsoldevila Nov 12, 2024
eba5431
Fix outdated test
gsoldevila Nov 12, 2024
11102ed
Keep conflicting search plugins as search/private for now
gsoldevila Nov 13, 2024
a3583f5
Merge branch 'main' into kbn-team-1079-links-to-apps-are-not-deps
gsoldevila Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions packages/core/application/core-application-browser-internal/src/application_service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,14 @@ import { themeServiceMock } from '@kbn/core-theme-browser-mocks';
import { overlayServiceMock } from '@kbn/core-overlays-browser-mocks';
import { customBrandingServiceMock } from '@kbn/core-custom-branding-browser-mocks';
import { analyticsServiceMock } from '@kbn/core-analytics-browser-mocks';
import { MockLifecycle } from './test_helpers/test_types';
import type { MockLifecycle } from './test_helpers/test_types';
import { ApplicationService } from './application_service';
import {
App,
AppDeepLink,
type App,
type AppDeepLink,
AppStatus,
AppUpdater,
PublicAppInfo,
type AppUpdater,
type PublicAppInfo,
} from '@kbn/core-application-browser';
import { act } from 'react-dom/test-utils';
import { DEFAULT_APP_VISIBILITY } from './utils';
Expand Down Expand Up @@ -618,6 +618,26 @@ describe('#start()', () => {
});
});

describe('isAppRegistered', () => {
let isAppRegistered: any;
beforeEach(async () => {
const { register } = service.setup(setupDeps);
register(Symbol(), createApp({ id: 'one_app' }));
register(Symbol(), createApp({ id: 'another_app', appRoute: '/custom/path' }));

const start = await service.start(startDeps);
isAppRegistered = start.isAppRegistered;
});

it('returns false for unregistered apps', () => {
expect(isAppRegistered('oneApp')).toEqual(false);
});

it('returns true for registered apps', () => {
expect(isAppRegistered('another_app')).toEqual(true);
});
});

describe('getUrlForApp', () => {
it('creates URL for unregistered appId', async () => {
service.setup(setupDeps);
Expand Down
3 changes: 3 additions & 0 deletions packages/core/application/core-application-browser-internal/src/application_service.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,9 @@ export class ApplicationService {
takeUntil(this.stop$)
),
history: this.history!,
isAppRegistered: (appId: string): boolean => {
return applications$.value.get(appId) !== undefined;
},
getUrlForApp: (
appId,
{
Expand Down
2 changes: 2 additions & 0 deletions packages/core/application/core-application-browser-mocks/src/application_service.mock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const createStartContractMock = (): jest.Mocked<ApplicationStart> => {
navigateToApp: jest.fn(),
navigateToUrl: jest.fn(),
getUrlForApp: jest.fn(),
isAppRegistered: jest.fn(),
};
};

Expand Down Expand Up @@ -92,6 +93,7 @@ const createInternalStartContractMock = (
currentActionMenu$: new BehaviorSubject<MountPoint | undefined>(undefined),
getComponent: jest.fn(),
getUrlForApp: jest.fn(),
isAppRegistered: jest.fn(),
navigateToApp: jest.fn().mockImplementation((appId) => currentAppId$.next(appId)),
navigateToUrl: jest.fn(),
history: createHistoryMock(),
Expand Down
15 changes: 13 additions & 2 deletions packages/core/application/core-application-browser/src/contracts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,12 @@ export interface ApplicationStart {
applications$: Observable<ReadonlyMap<string, PublicAppInfo>>;

/**
* Navigate to a given app
* Navigate to a given app.
* 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.
*
* @param appId
* @param appId - The identifier of the app to navigate to
* @param options - navigation options
*/
navigateToApp(appId: string, options?: NavigateToAppOptions): Promise<void>;
Expand Down Expand Up @@ -114,6 +117,14 @@ export interface ApplicationStart {
*/
navigateToUrl(url: string, options?: NavigateToUrlOptions): Promise<void>;

/**
Copy link
Contributor

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.

* Checks whether a given application is registered.
*
* @param appId - The identifier of the app to check
* @returns true if the given appId is registered in the system, false otherwise.
*/
isAppRegistered(appId: string): boolean;

/**
* Returns the absolute path (or URL) to a given app, including the global base path.
*
Expand Down
1 change: 1 addition & 0 deletions packages/core/plugins/core-plugins-browser-internal/src/plugin_context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ export function createPluginStartContext<
navigateToApp: deps.application.navigateToApp,
navigateToUrl: deps.application.navigateToUrl,
getUrlForApp: deps.application.getUrlForApp,
isAppRegistered: deps.application.isAppRegistered,
currentLocation$: deps.application.currentLocation$,
},
customBranding: deps.customBranding,
Expand Down
1 change: 1 addition & 0 deletions src/plugins/discover/public/__mocks__/start_contract.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export const createStartContractMock = (): jest.Mocked<ApplicationStart> => {
capabilities,
navigateToApp: jest.fn(),
navigateToUrl: jest.fn(),
isAppRegistered: jest.fn(),
getUrlForApp: jest.fn(),
};
};
1 change: 1 addition & 0 deletions ...t/public/management_section/objects_table/__snapshots__/saved_objects_table.test.tsx.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 5 additions & 3 deletions x-pack/plugins/enterprise_search/kibana.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
"type": "plugin",
"id": "@kbn/enterprise-search-plugin",
"owner": "@elastic/search-kibana",
// Could be categorised as Search in the future, but it currently needs to run in Observability too
"group": "platform",
"visibility": "shared",
// TODO this is currently used from Observability too, must be refactored before solution-specific builds
// see x-pack/plugins/observability_solution/observability_ai_assistant_management/public/routes/components/search_connector_tab.tsx
// cc sphilipse
"group": "search",
Copy link
Contributor Author

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?

Copy link
Member

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)

Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

"visibility": "private",
"description": "Adds dashboards for discovering and managing Enterprise Search products.",
"plugin": {
"id": "enterpriseSearch",
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/enterprise_search/public/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ export type EnterpriseSearchPublicStart = ReturnType<EnterpriseSearchPlugin['sta

interface PluginsSetup {
cloud?: CloudSetup;
licensing: LicensingPluginStart;
home?: HomePublicPluginSetup;
licensing: LicensingPluginStart;
security?: SecurityPluginSetup;
share?: SharePluginSetup;
}
Expand All @@ -98,8 +98,8 @@ export interface PluginsStart {
ml?: MlPluginStart;
navigation: NavigationPublicPluginStart;
searchConnectors?: SearchConnectorsPluginStart;
searchPlayground?: SearchPlaygroundPluginStart;
searchInferenceEndpoints?: SearchInferenceEndpointsPluginStart;
searchPlayground?: SearchPlaygroundPluginStart;
security?: SecurityPluginStart;
share?: SharePluginStart;
}
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/fleet/.storybook/context/application.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export const getApplication = () => {
navigateToApp: async (app: string) => {
action(`Navigate to: ${app}`);
},
isAppRegistered: (appId: string) => true,
getUrlForApp: (url: string) => url,
capabilities: {
catalogue: {},
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/observability_solution/observability_ai_assistant_management/kibana.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
"optionalPlugins": [
"home",
"serverless",
"enterpriseSearch"
],
"requiredBundles": [
"kibanaReact",
Expand Down
10 changes: 4 additions & 6 deletions x-pack/plugins/observability_solution/observability_ai_assistant_management/public/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,10 @@
*/

import { i18n } from '@kbn/i18n';
import { CoreSetup, Plugin, PluginInitializerContext } from '@kbn/core/public';
import { ManagementSetup } from '@kbn/management-plugin/public';
import { HomePublicPluginSetup } from '@kbn/home-plugin/public';
import { ServerlessPluginStart } from '@kbn/serverless/public';
import { EnterpriseSearchPublicStart } from '@kbn/enterprise-search-plugin/public';
import type { CoreSetup, Plugin, PluginInitializerContext } from '@kbn/core/public';
import type { ManagementSetup } from '@kbn/management-plugin/public';
import type { HomePublicPluginSetup } from '@kbn/home-plugin/public';
import type { ServerlessPluginStart } from '@kbn/serverless/public';

import type {
ObservabilityAIAssistantPublicSetup,
Expand All @@ -32,7 +31,6 @@ export interface SetupDependencies {
export interface StartDependencies {
observabilityAIAssistant: ObservabilityAIAssistantPublicStart;
serverless?: ServerlessPluginStart;
enterpriseSearch?: EnterpriseSearchPublicStart;
}

export interface ConfigSchema {
Expand Down
2 changes: 1 addition & 1 deletion ...n/observability_ai_assistant_management/public/routes/components/search_connector_tab.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' });
Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Member

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 🤔

Copy link
Contributor Author

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).


return (
<>
Expand Down
5 changes: 2 additions & 3 deletions ...solution/observability_ai_assistant_management/public/routes/components/settings_page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@ export function SettingsPage() {
const { setBreadcrumbs } = useAppContext();
const {
services: {
application: { navigateToApp },
application: { navigateToApp, isAppRegistered },
serverless,
enterpriseSearch,
},
} = useKibana();

Expand Down Expand Up @@ -98,7 +97,7 @@ export function SettingsPage() {
}
),
content: <SearchConnectorTab />,
disabled: enterpriseSearch == null,
disabled: !isAppRegistered('enterpriseSearch'),
},
];

Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/observability_solution/observability_ai_assistant_management/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
"@kbn/core-chrome-browser",
"@kbn/observability-ai-assistant-plugin",
"@kbn/serverless",
"@kbn/enterprise-search-plugin",
"@kbn/management-settings-components-field-row",
"@kbn/observability-shared-plugin",
"@kbn/config-schema",
Expand Down
7 changes: 5 additions & 2 deletions x-pack/plugins/search_inference_endpoints/kibana.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@
"type": "plugin",
"id": "@kbn/search-inference-endpoints",
"owner": "@elastic/search-kibana",
"group": "platform",
"visibility": "shared",
// TODO enterpriseSearch depends on it, and Observability has a menu entry for enterpriseSearch
// must be refactored / fixed before solution-specific builds
// cc sphilipse
"group": "search",
"visibility": "private",
"plugin": {
"id": "searchInferenceEndpoints",
"server": true,
Expand Down
7 changes: 4 additions & 3 deletions x-pack/plugins/search_playground/kibana.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
"type": "plugin",
"id": "@kbn/search-playground",
"owner": "@elastic/search-kibana",
// @kbn/enterprise-search-plugin (platform) and @kbn/serverless-search (search) depend on it
"group": "platform",
"visibility": "shared",
// TODO @kbn/enterprise-search-plugin (platform) and @kbn/serverless-search (search) depend on it
// cc sphilipse
"group": "search",
"visibility": "private",
"plugin": {
"id": "searchPlayground",
"server": true,
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/serverless_search/kibana.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
"indexManagement",
"searchConnectors",
"searchInferenceEndpoints",
"searchPlayground",
"usageCollection"
],
"requiredBundles": ["kibanaReact"]
Expand Down
Loading