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.

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

Remove Space Selector from Serverless Top Navigation #164461

Merged
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
607cee4
Use buildflavor to disable space selector and route
SiddharthMantri Aug 22, 2023
54eb873
Merge branch 'main' into disable-space-selector-serverless
SiddharthMantri Aug 22, 2023
a5532f9
Merge branch 'main' into disable-space-selector-serverless
SiddharthMantri Aug 23, 2023
1ab5558
Use isServerless boolean flag and update plugin test
SiddharthMantri Aug 23, 2023
ba0a842
Merge branch 'main' into disable-space-selector-serverless
SiddharthMantri Aug 23, 2023
65f4017
Merge branch 'main' into disable-space-selector-serverless
SiddharthMantri Aug 23, 2023
2f4df52
Merge branch 'main' into disable-space-selector-serverless
SiddharthMantri Aug 23, 2023
41f5fa1
Merge branch 'main' into disable-space-selector-serverless
SiddharthMantri Aug 23, 2023
fd7e40e
Merge branch 'main' into disable-space-selector-serverless
SiddharthMantri Aug 24, 2023
56a3fd0
Merge branch 'main' into disable-space-selector-serverless
SiddharthMantri Aug 24, 2023
ec9e5d3
Fixed tests and added serverless check for spaces app
SiddharthMantri Aug 28, 2023
6698bb8
Update signature of mocking function and wrapped check for isServerle…
SiddharthMantri Aug 29, 2023
7c1f14e
Merge branch 'main' into disable-space-selector-serverless
SiddharthMantri Aug 29, 2023
ade6b03
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine Aug 29, 2023
8ea4765
Merge branch 'main' into disable-space-selector-serverless
SiddharthMantri Aug 29, 2023
4cedff5
change test label from API to routes
SiddharthMantri Aug 29, 2023
d07844e
Update plugin.test.ts with correct test label
SiddharthMantri Aug 29, 2023
0f82e23
Update plugin.test.ts
SiddharthMantri Aug 29, 2023
28c6bb6
Merge branch 'main' into disable-space-selector-serverless
SiddharthMantri Aug 29, 2023
45655b1
Merge branch 'main' into disable-space-selector-serverless
SiddharthMantri Aug 30, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type { PublicMethodsOf } from '@kbn/utility-types';
import { loggerMock } from '@kbn/logging-mocks';
import type { PluginInitializerContext } from '@kbn/core-plugins-browser';
import type { PluginsService, PluginsServiceSetup } from '@kbn/core-plugins-browser-internal';
import type { BuildFlavor } from '@kbn/config/src/types';

const createSetupContractMock = () => {
const setupContract: jest.Mocked<PluginsServiceSetup> = {
Expand All @@ -27,7 +28,10 @@ const createStartContractMock = () => {
return startContract as PluginsServiceSetup;
};

const createPluginInitializerContextMock = (config: unknown = {}) => {
const createPluginInitializerContextMock = (
config: unknown = {},
{ buildFlavor = 'serverless' }: { buildFlavor?: BuildFlavor } = {}
Copy link
Contributor

@TinaHeiligers TinaHeiligers Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For UI-only changes, using the config should be ok although core now has an elasticsearch capabilities API that gets info from the cluster directly.

The new API is better than using a config flag because KIbana's sometimes run against a stateful ES instance. @pgayvallet is the capabilities API meant to be used instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, this is fine.

) => {
const mock: PluginInitializerContext = {
opaqueId: Symbol(),
env: {
Expand All @@ -43,7 +47,7 @@ const createPluginInitializerContextMock = (config: unknown = {}) => {
buildSha: 'buildSha',
dist: false,
buildDate: new Date('2023-05-15T23:12:09.000Z'),
buildFlavor: 'serverless',
buildFlavor,
},
},
logger: loggerMock.create(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"@kbn/logging-mocks",
"@kbn/core-plugins-browser-internal",
"@kbn/core-plugins-browser",
"@kbn/config",
],
"exclude": [
"target/**/*",
Expand Down
84 changes: 78 additions & 6 deletions x-pack/plugins/spaces/public/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,14 @@ import { SpacesPlugin } from './plugin';

describe('Spaces plugin', () => {
describe('#setup', () => {
it('should register the spaces API and the space selector app', () => {
it('should register the spaces API and the space selector app when buildFlavor is traditional', () => {
SiddharthMantri marked this conversation as resolved.
Show resolved Hide resolved
const coreSetup = coreMock.createSetup();
const mockInitializerContext = coreMock.createPluginInitializerContext(
{},
{ buildFlavor: 'traditional' }
);

const plugin = new SpacesPlugin(coreMock.createPluginInitializerContext());
const plugin = new SpacesPlugin(mockInitializerContext);
plugin.setup(coreSetup, {});

expect(coreSetup.application.register).toHaveBeenCalledWith(
Expand All @@ -33,7 +37,23 @@ describe('Spaces plugin', () => {
);
});

it('should register the management and feature catalogue sections when the management and home plugins are both available', () => {
it('should not register the spaces API and the space selector app when buildFlavor is serverless', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is meant by spaces API here? Doesn't this test confirm only the space selector app registration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeramysoucy You're right! I changed the label to make it clear. It should say spaces routes and not API.

Copy link
Contributor

@jeramysoucy jeramysoucy Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SiddharthMantri The same wording is used on line 20.

const coreSetup = coreMock.createSetup();

const plugin = new SpacesPlugin(coreMock.createPluginInitializerContext());
plugin.setup(coreSetup, {});

expect(coreSetup.application.register).not.toHaveBeenCalledWith(
expect.objectContaining({
id: 'space_selector',
chromeless: true,
appRoute: '/spaces/space_selector',
mount: expect.any(Function),
})
);
});

it('should register the management and feature catalogue sections when the management and home plugins are both available when buildFlavor is traditional', () => {
const coreSetup = coreMock.createSetup();
const home = homePluginMock.createSetupContract();

Expand All @@ -43,7 +63,12 @@ describe('Spaces plugin', () => {

management.sections.section.kibana = mockSection;

const plugin = new SpacesPlugin(coreMock.createPluginInitializerContext());
const mockInitializerContext = coreMock.createPluginInitializerContext(
{},
{ buildFlavor: 'traditional' }
);

const plugin = new SpacesPlugin(mockInitializerContext);
plugin.setup(coreSetup, {
management,
home,
Expand All @@ -62,19 +87,66 @@ describe('Spaces plugin', () => {
})
);
});

it('should not register spaces in the management plugin or the feature catalog when the management and home plugins are both available when buildFlavor is serverless', () => {
const coreSetup = coreMock.createSetup();
const home = homePluginMock.createSetupContract();

const management = managementPluginMock.createSetupContract();
const mockSection = createManagementSectionMock();
mockSection.registerApp = jest.fn();

management.sections.section.kibana = mockSection;

const plugin = new SpacesPlugin(coreMock.createPluginInitializerContext());
plugin.setup(coreSetup, {
management,
home,
});

expect(mockSection.registerApp).not.toHaveBeenCalledWith(
expect.objectContaining({ id: 'spaces' })
);

expect(home.featureCatalogue.register).not.toHaveBeenCalledWith(
expect.objectContaining({
category: 'admin',
icon: 'spacesApp',
id: 'spaces',
showOnHomePage: false,
})
);
});
});

describe('#start', () => {
it('should register the spaces nav control', () => {
it('should register the spaces nav control when buildFlavor is traditional', () => {
const coreSetup = coreMock.createSetup();
const coreStart = coreMock.createStart();

const plugin = new SpacesPlugin(coreMock.createPluginInitializerContext());
const mockInitializerContext = coreMock.createPluginInitializerContext(
{},
{ buildFlavor: 'traditional' }
);

const plugin = new SpacesPlugin(mockInitializerContext);
plugin.setup(coreSetup, {});

plugin.start(coreStart);

expect(coreStart.chrome.navControls.registerLeft).toHaveBeenCalled();
});

it('should not register the spaces nav control when buildFlavor is serverless', () => {
const coreSetup = coreMock.createSetup();
const coreStart = coreMock.createStart();

const plugin = new SpacesPlugin(coreMock.createPluginInitializerContext());
plugin.setup(coreSetup, {});

plugin.start(coreStart);

expect(coreStart.chrome.navControls.registerLeft).not.toHaveBeenCalled();
});
});
});
38 changes: 22 additions & 16 deletions x-pack/plugins/spaces/public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,11 @@ export class SpacesPlugin implements Plugin<SpacesPluginSetup, SpacesPluginStart

private managementService?: ManagementService;
private readonly config: ConfigType;
private readonly isServerless: boolean;

constructor(private readonly initializerContext: PluginInitializerContext) {
this.config = this.initializerContext.config.get<ConfigType>();
this.isServerless = this.initializerContext.env.packageInfo.buildFlavor === 'serverless';
}

public setup(core: CoreSetup<PluginsStart, SpacesPluginStart>, plugins: PluginsSetup) {
Expand All @@ -61,31 +63,35 @@ export class SpacesPlugin implements Plugin<SpacesPluginSetup, SpacesPluginStart
getActiveSpace: () => this.spacesManager.getActiveSpace(),
};

if (plugins.home) {
plugins.home.featureCatalogue.register(createSpacesFeatureCatalogueEntry());
}

if (plugins.management) {
this.managementService = new ManagementService();
this.managementService.setup({
management: plugins.management,
if (!this.isServerless) {
if (plugins.home) {
plugins.home.featureCatalogue.register(createSpacesFeatureCatalogueEntry());
}

if (plugins.management) {
this.managementService = new ManagementService();
this.managementService.setup({
management: plugins.management,
getStartServices: core.getStartServices,
spacesManager: this.spacesManager,
config: this.config,
});
}

spaceSelectorApp.create({
getStartServices: core.getStartServices,
application: core.application,
spacesManager: this.spacesManager,
config: this.config,
});
}

spaceSelectorApp.create({
getStartServices: core.getStartServices,
application: core.application,
spacesManager: this.spacesManager,
});

return {};
}

public start(core: CoreStart) {
initSpacesNavControl(this.spacesManager, core);
if (!this.isServerless) {
initSpacesNavControl(this.spacesManager, core);
}

return this.spacesApi;
}
Expand Down