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

implement "plugin" package type #149370

Merged
merged 11 commits into from
Jan 30, 2023
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`;
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: rand in tests is bad, m'kay?

Copy link
Contributor Author

@spalger spalger Jan 25, 2023

Choose a reason for hiding this comment

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

lol, fine, want me to just hard-code a version here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, given we had surprises in the past when using the current version, having an hardcoded one seems the best compromise to me

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,
Copy link
Contributor

@pgayvallet pgayvallet Jan 25, 2023

Choose a reason for hiding this comment

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

What are serviceFolders exactly? I saw the documentation on this property later in the PR, but still not sure to get it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty sure they were added to the documentation system for core specifically so that core/server and core/public were separate documentation sections. I'm not totally clear what it actually does, but I'm just maintaining support for it.

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 @@ -6,12 +6,14 @@
* Side Public License, v 1.
*/

import { from, merge } from 'rxjs';
import { from, merge, EMPTY } from 'rxjs';
import { catchError, filter, map, mergeMap, concatMap, shareReplay, toArray } from 'rxjs/operators';
import { Logger } from '@kbn/logging';
import { getPluginPackagesFilter } from '@kbn/repo-packages';
import type { CoreContext } from '@kbn/core-base-server-internal';
import type { NodeInfo } from '@kbn/core-node-server';
import { PluginWrapper } from '../plugin';
import { pluginManifestFromPluginPackage } from './plugin_manifest_from_plugin_package';
import { createPluginInitializerContext, InstanceInfo } from '../plugin_context';
import { PluginsConfig } from '../plugins_config';
import { PluginDiscoveryError } from './plugin_discovery_error';
Expand Down Expand Up @@ -48,7 +50,7 @@ export function discover({
);
}

const discoveryResults$ = merge(
const fsDiscovery$ = merge(
from(config.additionalPluginPaths),
scanPluginSearchPaths(config.pluginSearchPaths, log)
).pipe(
Expand All @@ -64,10 +66,43 @@ export function discover({
return typeof pluginPathOrError === 'string'
? createPlugin$(pluginPathOrError, log, coreContext, instanceInfo, nodeInfo)
: [pluginPathOrError];
}),
shareReplay()
})
);

const pluginPkgDiscovery$ = from(coreContext.env.repoPackages ?? EMPTY).pipe(
filter(
getPluginPackagesFilter({
oss: coreContext.env.cliArgs.oss,
examples: coreContext.env.cliArgs.runExamples,
paths: config.additionalPluginPaths,
parentDirs: config.pluginSearchPaths,
})
),
Comment on lines +64 to +72
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a fundamental change. The associated test file will have to be adapted accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

map((pkg) => {
log.debug(`Successfully discovered plugin package "${pkg.id}"`);
const manifest = pluginManifestFromPluginPackage(
coreContext.env.packageInfo.version,
pkg.manifest
);
Comment on lines +75 to +78
Copy link
Contributor

@pgayvallet pgayvallet Jan 25, 2023

Choose a reason for hiding this comment

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

It's still unclear to me how to define multi-zone (browser+server) plugins with this package approach? Is it as simple as creating a package with a server and client sub folders? I assume so, given the PR remove the distinction with a single plugin package type, but I wanted to confirm it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's based on the plugin.server and plugin.browser properties in kibana.jsonc. They're both required properties in the manifest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if my question was unclear, let me rephrase: can we still define plugins having both server and client parts with the new system?

const initializerContext = createPluginInitializerContext({
coreContext,
opaqueId: Symbol(pkg.id),
manifest,
instanceInfo,
nodeInfo,
});

return new PluginWrapper({
path: pkg.directory,
manifest,
opaqueId: initializerContext.opaqueId,
initializerContext,
});
})
);

const discoveryResults$ = merge(fsDiscovery$, pluginPkgDiscovery$).pipe(shareReplay());
Copy link
Contributor

Choose a reason for hiding this comment

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

merge(fsDiscovery$, pluginPkgDiscovery$)

I feel like we should be checking for duplicated from the two sources somehow here, unless we're adamant there's no risk of plugins being scanned by both (fs+package), wdyt?

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 filesystem scanning only finds legacy plugins with kibana.json files, and the package scanning only finds packages with kibana.jsonc files. fsDiscovery$ won't ever produce duplicate values, though I'm find adding a distinct operator here.

Copy link
Contributor

Choose a reason for hiding this comment

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

My naive question was: what about a folder with both kibana.json and kibana.jsonc for the same plugin?


return {
plugin$: discoveryResults$.pipe(
filter((entry): entry is PluginWrapper => entry instanceof PluginWrapper)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"@kbn/core-environment-server-internal",
"@kbn/core-node-server-internal",
"@kbn/core-plugins-base-server-internal",
"@kbn/repo-packages",
],
"exclude": [
"target/**/*",
Expand Down
2 changes: 2 additions & 0 deletions packages/core/root/core-root-server-internal/src/bootstrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

import chalk from 'chalk';
import { getPackages } from '@kbn/repo-packages';
import { CliArgs, Env, RawConfigService } from '@kbn/config';
import { CriticalError } from '@kbn/core-base-server-internal';
import { Root } from './root';
Expand Down Expand Up @@ -39,6 +40,7 @@ export async function bootstrap({ configs, cliArgs, applyConfigOverrides }: Boot
const env = Env.createDefault(REPO_ROOT, {
configs,
cliArgs,
repoPackages: getPackages(REPO_ROOT),
});

const rawConfigService = new RawConfigService(env.configs, applyConfigOverrides);
Expand Down
1 change: 1 addition & 0 deletions packages/core/root/core-root-server-internal/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
"@kbn/apm-config-loader",
"@kbn/core-custom-branding-server-internal",
"@kbn/core-custom-branding-server-mocks",
"@kbn/repo-packages",
],
"exclude": [
"target/**/*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { defaultsDeep } from 'lodash';
import { BehaviorSubject } from 'rxjs';
import supertest from 'supertest';

import { getPackages } from '@kbn/repo-packages';
import { ToolingLog } from '@kbn/tooling-log';
import { REPO_ROOT } from '@kbn/repo-info';
import {
Expand Down Expand Up @@ -71,6 +72,7 @@ export function createRootWithSettings(
dist: false,
...cliArgs,
},
repoPackages: getPackages(REPO_ROOT),
},
pkg
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
"@kbn/core-lifecycle-server-internal",
"@kbn/core-root-server-internal",
"@kbn/repo-info",
"@kbn/repo-packages",
],
"exclude": [
"target/**/*",
Expand Down
2 changes: 2 additions & 0 deletions packages/kbn-cli-dev-mode/src/bootstrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

import { REPO_ROOT } from '@kbn/repo-info';
import { getPackages } from '@kbn/repo-packages';
import { CliArgs, Env, RawConfigAdapter } from '@kbn/config';
import { CliDevMode } from './cli_dev_mode';
import { CliLog } from './log';
Expand All @@ -25,6 +26,7 @@ export async function bootstrapDevMode({ configs, cliArgs, applyConfigOverrides
const env = Env.createDefault(REPO_ROOT, {
configs,
cliArgs,
repoPackages: getPackages(REPO_ROOT),
});

const config = await loadConfig({
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-cli-dev-mode/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"@kbn/repo-source-classifier",
"@kbn/import-resolver",
"@kbn/picomatcher",
"@kbn/repo-packages",
],
"exclude": [
"target/**/*",
Expand Down
2 changes: 2 additions & 0 deletions packages/kbn-config-mocks/src/env.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

import { REPO_ROOT } from '@kbn/repo-info';
import { getPackages } from '@kbn/repo-packages';
import { Env, type RawPackageInfo, type EnvOptions } from '@kbn/config';

type DeepPartial<T> = {
Expand All @@ -28,6 +29,7 @@ export function getEnvOptions(options: DeepPartial<EnvOptions> = {}): EnvOptions
runExamples: false,
...(options.cliArgs || {}),
},
repoPackages: getPackages(REPO_ROOT),
};
}

Expand Down
1 change: 1 addition & 0 deletions packages/kbn-config-mocks/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"@kbn/utility-types",
"@kbn/doc-links",
"@kbn/repo-info",
"@kbn/repo-packages",
],
"exclude": [
"target/**/*",
Expand Down
Loading