Skip to content

Commit

Permalink
implement "plugin" package type (#149370)
Browse files Browse the repository at this point in the history
This PR updates the core discovery logic to support loading plugins from
packages. This logic is additive, so that the existing plugins in the
repo and third-party plugins can continue to be loaded via the existing
mechanism, but with #148130 we
will be automatically migrating all plugins in the repo to packages,
which will use this logic.

The logic is already in-use in that PR, and was developed there, but
extracted here for easier review.

The logic is relatively simple, where a list of packages in the repo are
attached to the core `Env` and then filtered by core before converting
all plugin packages to `PluginWrapper`. The `PluginWrapper` still
exposes the plugin manifest to the rest of the code, and it is used in
many places, so rather than making changes to the `PluginWrapper` I'm
faking a legacy plugin manifest with the plugin package manifest.

@elastic/kibana-core: I'm going to need some help identifying what we
need to get test coverage for. This is a pretty simple addition to the
core IMO, and if it didn't work then nothing would work, so I'm pretty
confident in it, but would still appreciate your feedback.
  • Loading branch information
Spencer authored Jan 30, 2023
1 parent ad75d90 commit 376bed5
Show file tree
Hide file tree
Showing 31 changed files with 520 additions and 127 deletions.
18 changes: 7 additions & 11 deletions dev_docs/operations/packages_idm.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -51,19 +51,16 @@ Package types allow us to have many different packages, pre-defined build tasks,
: These packages can be imported from all other packages.

`shared-browser`
: These packages can be imported from `shared-browser` and `plugin-browser` packages. `shared-browser` packages may include Storybooks.
: These packages can be imported from `shared-browser` and `public` directories within `plugin` packages. `shared-browser` packages may include Storybooks.

`shared-server`
: These packages can be imported from `shared-server` and `plugin-server` packages.
: These packages can be imported from `shared-server` and `server` directories within `plugin` packages.

`shared-scss`
: These packages can be imported by `shared-browser` and `plugin-browser` packages, and expose an `index.scss` file to consumers instead of an `index.ts` file.
: These packages can be imported by `shared-browser` and `public` directories within `plugin` packages, and expose an `index.scss` file to consumers instead of an `index.ts` file.

`plugin-browser`
: These packages expose types to other packages via a root `types.ts` file. Module IDs must end with `-plugin-browser`. Consumers must use `import type` statements.

`plugin-server`
: These packages expose types to other packages via a root `types.ts` file. Module IDs must end with `-plugin-server`. Consumers must use `import type` statements.
`plugin`
: These packages were automatically created from the existing plugins at the time we switched everything over to packages. Module IDs must end with `-plugin`. Consumers must use `import type` statements.

`functional-test`
: These packages expose one or more functional testing configurations, including API integration tests, and can not be imported by other packages. Separating functional and integration tests allows us to iterate on tests without rebuilding the application. Similarly, iterating and updating the application should mostly mean the tests don't need to rebuild.
Expand Down Expand Up @@ -160,8 +157,7 @@ The solution to resolving a circular dependency has, thus far, been to break out

There are a few package naming rules:
- all packages must use the `@kbn/` namespace
- `plugin-browser`-type packages must end with `-plugin-browser`
- `plugin-server- type packages must end with `-plugin-server`
- `plugin`-type packages must end with `-plugin`
- considering that we operate in a global namespace, avoid overly generic names

Other than these rules, it's up to you and your team to decide on an appropriate name for your package.
Expand Down Expand Up @@ -205,4 +201,4 @@ We're now entering Phase 2 of the plan, more details about the phases of our pla

[status]: #what-works-now
[idm-rfc]: https://docs.google.com/document/d/1Bhg601MoGQjqGMGdLWSLnkopRexwrcbf_0MNcUkhx3I "Internal Dependency Management RFC on Google Docs"
[pkgDirs]: https://github.com/elastic/kibana/blob/main/packages/kbn-bazel-packages/src/bazel_package_dirs.ts#L22
[pkgDirs]: https://github.com/elastic/kibana/blob/main/packages/kbn-repo-packages/src/repo_package_dirs.js#L19
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ function create({
logger?: jest.Mocked<LoggerFactory>;
configService?: jest.Mocked<IConfigService>;
} = {}): DeeplyMockedKeys<CoreContext> {
return { coreId: Symbol(), env, logger, configService };
return { coreId: Symbol(), env: env as DeeplyMockedKeys<typeof env>, logger, configService };
}

export const mockCoreContext = {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { PluginPackageManifest } from '@kbn/repo-packages';
import { PluginType } from '@kbn/core-base-common';
import { pluginManifestFromPluginPackage } from './plugin_manifest_from_plugin_package';

const kibanaVersion = `1.${Math.round(10 * Math.random())}.1`;
const minimal: PluginPackageManifest = {
type: 'plugin',
id: '@kbn/some-legacy-plugin',
owner: ['@elastic/team-a', '@elastic/team-b'],
plugin: {
id: 'someLegacyPluginId',
browser: true,
server: true,
},
};
const basic: PluginPackageManifest = {
...minimal,
plugin: {
...minimal.plugin,
type: 'preboot',
configPath: ['some', 'legacy'],
enabledOnAnonymousPages: false,
extraPublicDirs: ['foo', 'bar'],
optionalPlugins: ['someOtherPlugin'],
requiredBundles: ['someRequiresBundlePlugin'],
requiredPlugins: ['someRequiredPlugin'],
},
serviceFolders: ['foo', 'bar'],
};

describe('pluginManifestFromPluginPackage()', () => {
it('consumes correct values from plugin package manifest', () => {
expect(pluginManifestFromPluginPackage('static', basic)).toMatchInlineSnapshot(`
Object {
"configPath": Array [
"some",
"legacy",
],
"enabledOnAnonymousPages": false,
"id": "someLegacyPluginId",
"kibanaVersion": "static",
"optionalPlugins": Array [
"someOtherPlugin",
],
"owner": Object {
"name": "@elastic/team-a & @elastic/team-b",
},
"requiredBundles": Array [
"someRequiresBundlePlugin",
],
"requiredPlugins": Array [
"someRequiredPlugin",
],
"server": true,
"serviceFolders": Array [
"foo",
"bar",
],
"type": "preboot",
"ui": true,
"version": "1.0.0",
}
`);
});

it('applies correct defaults', () => {
const pm = pluginManifestFromPluginPackage(kibanaVersion, minimal);
expect(pm).toHaveProperty('type', PluginType.standard);
expect(pm.enabledOnAnonymousPages).toBeUndefined();
expect(pm.serviceFolders).toBeUndefined();
expect(pm).toHaveProperty('kibanaVersion', kibanaVersion);
expect(pm).toHaveProperty('optionalPlugins', []);
expect(pm).toHaveProperty('requiredBundles', []);
expect(pm).toHaveProperty('requiredPlugins', []);
expect(pm).toHaveProperty('owner', {
name: '@elastic/team-a & @elastic/team-b',
});
expect(pm).toHaveProperty('server', true);
expect(pm).toHaveProperty('ui', true);
expect(pm).toHaveProperty('configPath', 'some_legacy_plugin_id');
});

it('reflects plugin.server', () => {
expect(
pluginManifestFromPluginPackage(kibanaVersion, {
...minimal,
plugin: { ...minimal.plugin, server: false },
})
).toHaveProperty('server', false);
expect(
pluginManifestFromPluginPackage(kibanaVersion, {
...minimal,
plugin: { ...minimal.plugin, server: true },
})
).toHaveProperty('server', true);
});

it('reflects plugin.browser', () => {
expect(
pluginManifestFromPluginPackage(kibanaVersion, {
...minimal,
plugin: { ...minimal.plugin, browser: false },
})
).toHaveProperty('ui', false);
expect(
pluginManifestFromPluginPackage(kibanaVersion, {
...minimal,
plugin: { ...minimal.plugin, browser: true },
})
).toHaveProperty('ui', true);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { snakeCase } from 'lodash';
import { PluginPackageManifest } from '@kbn/repo-packages';
import { PluginManifest } from '@kbn/core-plugins-server';
import { PluginType } from '@kbn/core-base-common';

export function pluginManifestFromPluginPackage(
kibanaVersion: string,
manifest: PluginPackageManifest
): PluginManifest {
return {
type: manifest.plugin.type === 'preboot' ? PluginType.preboot : PluginType.standard,
id: manifest.plugin.id,
version: '1.0.0',
enabledOnAnonymousPages: manifest.plugin.enabledOnAnonymousPages,
serviceFolders: manifest.serviceFolders,
kibanaVersion,
optionalPlugins: manifest.plugin.optionalPlugins ?? [],
requiredBundles: manifest.plugin.requiredBundles ?? [],
requiredPlugins: manifest.plugin.requiredPlugins ?? [],
owner: {
name: manifest.owner.join(' & '),
},
server: manifest.plugin.server,
ui: manifest.plugin.browser,
configPath: manifest.plugin.configPath ?? snakeCase(manifest.plugin.id),
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { mockPackage, scanPluginSearchPathsMock } from './plugins_discovery.test
import mockFs from 'mock-fs';
import { getEnvOptions, rawConfigServiceMock } from '@kbn/config-mocks';
import { loggingSystemMock } from '@kbn/core-logging-server-mocks';
import type { Package } from '@kbn/repo-packages';

import { firstValueFrom, from } from 'rxjs';
import { map, toArray } from 'rxjs/operators';
Expand All @@ -25,6 +26,35 @@ import { discover } from './plugins_discovery';
import { PluginType } from '@kbn/core-base-common';

const KIBANA_ROOT = process.cwd();
jest.mock('@kbn/repo-packages', () => ({
...jest.requireActual('@kbn/repo-packages'),
getPackages: jest.fn().mockReturnValue([]),
getPluginPackagesFilter: jest.fn().mockReturnValue(() => true),
}));

jest.mock('./plugin_manifest_from_plugin_package', () => ({
pluginManifestFromPluginPackage: jest.fn((version, pkgManifest) => ({
version,
...pkgManifest,
})),
}));

const getPluginPackagesFilterMock: jest.Mock =
jest.requireMock('@kbn/repo-packages').getPluginPackagesFilter;
const pluginManifestFromPluginPackageMock: jest.Mock = jest.requireMock(
'./plugin_manifest_from_plugin_package'
).pluginManifestFromPluginPackage;

function getMockPackage(id: string) {
return {
id,
manifest: {
id,
type: 'plugin',
},
directory: resolve(REPO_ROOT, `packages/${id}`),
} as Package;
}

const Plugins = {
invalid: () => ({
Expand Down Expand Up @@ -180,6 +210,8 @@ describe('plugins discovery system', () => {
jest.spyOn(console, 'log').mockImplementation((...args) => {
process.stdout.write(args + '\n');
});

jest.clearAllMocks();
});

afterEach(() => {
Expand Down Expand Up @@ -542,9 +574,61 @@ describe('plugins discovery system', () => {
expect(loggingSystemMock.collect(logger).warn).toEqual([]);
});

describe('plugin packages', () => {
it('filters repoPackages in the env and converts them to PluginWrappers', async () => {
const foo = getMockPackage('foo');
const bar = getMockPackage('bar');
coreContext.env = {
...env,
pluginSearchPaths: [],
repoPackages: [foo, bar],
};
const filterFn = jest.fn((p: Package) => p === foo);
getPluginPackagesFilterMock.mockReturnValue(filterFn);

const { plugin$ } = discover({
config: new PluginsConfig(pluginConfig, coreContext.env),
coreContext,
instanceInfo,
nodeInfo,
});

const [plugin, ...empty] = await firstValueFrom(plugin$.pipe(toArray()));
expect(empty).toHaveLength(0);

expect(getPluginPackagesFilterMock).toHaveBeenCalledTimes(1);
const filterArgs = getPluginPackagesFilterMock.mock.calls[0];
expect(filterArgs).toEqual([
{
examples: false,
oss: false,
parentDirs: [],
paths: [],
},
]);

expect(filterFn).toHaveBeenCalledTimes(2);
expect(filterFn.mock.calls[0]).toEqual([foo, 0]);
expect(filterFn.mock.calls[1]).toEqual([bar, 1]);
expect(filterFn.mock.results).toEqual([
{ type: 'return', value: true },
{ type: 'return', value: false },
]);

expect(pluginManifestFromPluginPackageMock).toHaveBeenCalledTimes(1);
const manifestArgs = pluginManifestFromPluginPackageMock.mock.calls[0];
expect(manifestArgs).toEqual([coreContext.env.packageInfo.version, foo.manifest]);
expect(pluginManifestFromPluginPackageMock.mock.results[0]).toEqual({
type: 'return',
value: plugin.manifest,
});
});
});

describe('discovery order', () => {
beforeEach(() => {
scanPluginSearchPathsMock.mockClear();
getPluginPackagesFilterMock.mockReturnValue(() => true);
});

it('returns the plugins in a deterministic order', async () => {
Expand All @@ -565,8 +649,13 @@ describe('plugins discovery system', () => {
])
);

coreContext.env = {
...env,
repoPackages: [getMockPackage('foo'), getMockPackage('bar')],
};

let { plugin$ } = discover({
config: new PluginsConfig(pluginConfig, env),
config: new PluginsConfig(pluginConfig, coreContext.env),
coreContext,
instanceInfo,
nodeInfo,
Expand All @@ -576,9 +665,8 @@ describe('plugins discovery system', () => {
let plugins = await firstValueFrom(plugin$.pipe(toArray()));
let pluginNames = plugins.map((plugin) => plugin.name);

expect(pluginNames).toHaveLength(3);
// order coming from `ROOT/plugin` -> `ROOT/src/plugins` -> // ROOT/x-pack
expect(pluginNames).toEqual(['pluginB', 'pluginA', 'pluginC']);
// order coming from `ROOT/packages` -> `ROOT/plugin` -> `ROOT/src/plugins` -> // ROOT/x-pack
expect(pluginNames).toEqual(['bar', 'foo', 'pluginB', 'pluginA', 'pluginC']);

// second pass
scanPluginSearchPathsMock.mockReturnValue(
Expand All @@ -589,6 +677,11 @@ describe('plugins discovery system', () => {
])
);

coreContext.env = {
...env,
repoPackages: [getMockPackage('bar'), getMockPackage('foo')],
};

plugin$ = discover({
config: new PluginsConfig(pluginConfig, env),
coreContext,
Expand All @@ -600,9 +693,8 @@ describe('plugins discovery system', () => {
plugins = await firstValueFrom(plugin$.pipe(toArray()));
pluginNames = plugins.map((plugin) => plugin.name);

expect(pluginNames).toHaveLength(3);
// order coming from `ROOT/plugin` -> `ROOT/src/plugins` -> // ROOT/x-pack
expect(pluginNames).toEqual(['pluginB', 'pluginA', 'pluginC']);
// order coming from `ROOT/packages` -> `ROOT/plugin` -> `ROOT/src/plugins` -> // ROOT/x-pack
expect(pluginNames).toEqual(['bar', 'foo', 'pluginB', 'pluginA', 'pluginC']);
});
});
});
Loading

0 comments on commit 376bed5

Please sign in to comment.