Skip to content

Commit

Permalink
[dx] Dependency check script for plugins (#171483)
Browse files Browse the repository at this point in the history
## Summary

Dealing with circular dependencies between plugins has become a sharp
pain point for anyone developing plugins in Kibana.

### Providing dependencies to a plugin

First, a plugin defines its dependencies in its `kibana.jsonc` file as
one of three types:

- `required` - the dependency must be present and enabled -- will be
guaranteed in the lifecycle
- `optional` - the dependency can be missing or disabled -- will be
`undefined` in the lifecycle
- `requiredBundle` - the dependency is required as static code only --
will not be present in the lifecycle

Missing or circular dependencies are detected by the Kibana platform
when it starts.

### Providing dependencies in code

Our plugins are written in and type-checked by Typescript. As such, each
plugin needs to maintain Typescript types defining what the platform is
providing. This is done manually, and there is no enforcement mechanism
between that and the plugin Typescript types. If these dependency
definitions are inconsistent or stale, it can lead to host of issues:

- optional plugins typed as required that are disabled crash the plugin
at runtime;
- plugins that are no longer used still included in dependency checks;
- plugins marked as required or optional that are actually required
bundles.
- etc.

### Dependencies with side-effects

One of the interesting things that has come out of this has been
identifying plugins that provide dependent logic through side-effects,
rather than lifecycles.

As an example, `licensing` provides a lifecycle contracts, but also a
[route handler
context](https://github.com/elastic/kibana/blob/main/x-pack/plugins/licensing/server/licensing_route_handler_context.ts)
as middleware for a dependent plugin. Unfortunately, while this
dependency can be stated as `required` in a dependent plugin's
`kibana.jsonc`, the fact that this is a side-effect makes it incredible
difficult to understand the dependency without searching the code.

<img width="735" alt="Screenshot 2023-12-13 at 10 08 00 AM"
src="https://github.com/elastic/kibana/assets/297604/b4201c86-4811-4506-b2d0-be5bf8c372b0">

So the side-effect is more or less hidden from developers. This is
likely why we see other plugins using the lifecycle
[logic](https://github.com/elastic/kibana/blob/main/src/plugins/maps_ems/public/kibana_services.ts#L33-L37),
or copy-pasting licensing check code
[[1](https://github.com/elastic/kibana/blob/main/x-pack/plugins/actions/server/lib/license_state.ts),
[2](https://github.com/elastic/kibana/blob/main/x-pack/plugins/alerting/server/lib/license_state.ts)],
or relying on the route context side-effect.

## Proposed (initial) solution

This script is an initial attempt to both identify these problems and
surface a plugin's dependencies in a useful way. In addition, the script
will warn if the classes aren't typed well, not typed at all, or even
don't extend the `core` `Plugin` class.

<img width="1426" alt="Screenshot 2023-12-13 at 12 37 25 AM"
src="https://github.com/elastic/kibana/assets/297604/e044afb7-26f5-4d96-92db-d2eb0a3dfc6e">
<img width="1413" alt="Screenshot 2023-12-13 at 12 38 07 AM"
src="https://github.com/elastic/kibana/assets/297604/69217a34-9840-4d32-98de-eeeb863d4a50">
<img width="1071" alt="Screenshot 2023-12-13 at 12 38 35 AM"
src="https://github.com/elastic/kibana/assets/297604/57736027-2d10-44bf-8230-29fdb8b77cb2">

For side-effects, identifying them is key, and then refactoring the
plugins to provide appropriate logic in the `start` or `setup`
contracts.

## Next steps

- [x] refine the logic
- [ ] write tests
- [ ] add `--fix` option

I'm also considering (in another PR), outputting a consistent type
definition file-- perhaps `kibana.d.ts`-- to the plugin from which the
implementing classes could `Omit<>` or `Pick<>` the relevant contracts.
  • Loading branch information
clintandrewhall authored Jan 17, 2024
1 parent 64ebaff commit 2f92ce1
Show file tree
Hide file tree
Showing 30 changed files with 1,414 additions and 18 deletions.
1 change: 1 addition & 0 deletions .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,7 @@ packages/kbn-peggy @elastic/kibana-operations
packages/kbn-peggy-loader @elastic/kibana-operations
packages/kbn-performance-testing-dataset-extractor @elastic/kibana-performance-testing
packages/kbn-picomatcher @elastic/kibana-operations
packages/kbn-plugin-check @elastic/appex-sharedux
packages/kbn-plugin-generator @elastic/kibana-operations
packages/kbn-plugin-helpers @elastic/kibana-operations
examples/portable_dashboards_example @elastic/kibana-presentation
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,7 @@
"@kbn/paertial-results-example-plugin": "link:examples/partial_results_example",
"@kbn/painless-lab-plugin": "link:x-pack/plugins/painless_lab",
"@kbn/panel-loader": "link:packages/kbn-panel-loader",
"@kbn/plugin-check": "link:packages/kbn-plugin-check",
"@kbn/portable-dashboards-example": "link:examples/portable_dashboards_example",
"@kbn/preboot-example-plugin": "link:examples/preboot_example",
"@kbn/presentation-containers": "link:packages/presentation/presentation_containers",
Expand Down Expand Up @@ -1659,7 +1660,7 @@
"terser-webpack-plugin": "^4.2.3",
"tough-cookie": "^4.1.3",
"tree-kill": "^1.2.2",
"ts-morph": "^13.0.2",
"ts-morph": "^15.1.0",
"tsd": "^0.20.0",
"typescript": "4.7.4",
"url-loader": "^2.2.0",
Expand Down
2 changes: 2 additions & 0 deletions packages/kbn-docs-utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,5 @@
*/

export { runBuildApiDocsCli } from './src';

export { findPlugins, findTeamPlugins } from './src/find_plugins';
28 changes: 27 additions & 1 deletion packages/kbn-docs-utils/src/find_plugins.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ function toApiScope(pkg: Package): ApiScope {
}

function toPluginOrPackage(pkg: Package): PluginOrPackage {
return {
const result = {
id: pkg.isPlugin() ? pkg.manifest.plugin.id : pkg.manifest.id,
directory: Path.resolve(REPO_ROOT, pkg.normalizedRepoRelativeDir),
manifestPath: Path.resolve(REPO_ROOT, pkg.normalizedRepoRelativeDir, 'kibana.jsonc'),
Expand All @@ -50,6 +50,20 @@ function toPluginOrPackage(pkg: Package): PluginOrPackage {
},
scope: toApiScope(pkg),
};

if (pkg.isPlugin()) {
return {
...result,
manifest: {
...result.manifest,
requiredPlugins: pkg.manifest.plugin.requiredPlugins || [],
optionalPlugins: pkg.manifest.plugin.optionalPlugins || [],
requiredBundles: pkg.manifest.plugin.requiredBundles || [],
},
};
}

return result;
}

export function findPlugins(pluginOrPackageFilter?: string[]): PluginOrPackage[] {
Expand Down Expand Up @@ -78,6 +92,18 @@ export function findPlugins(pluginOrPackageFilter?: string[]): PluginOrPackage[]
}
}

export function findTeamPlugins(team: string): PluginOrPackage[] {
const packages = getPackages(REPO_ROOT);
const plugins = packages.filter(
getPluginPackagesFilter({
examples: false,
testPlugins: false,
})
);

return [...plugins.filter((p) => p.manifest.owner.includes(team)).map(toPluginOrPackage)];
}

/**
* Helper to find packages.
*/
Expand Down
3 changes: 3 additions & 0 deletions packages/kbn-docs-utils/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ export interface PluginOrPackage {
description?: string;
owner: { name: string; githubTeam?: string };
serviceFolders: readonly string[];
requiredBundles?: readonly string[];
requiredPlugins?: readonly string[];
optionalPlugins?: readonly string[];
};
isPlugin: boolean;
directory: string;
Expand Down
5 changes: 5 additions & 0 deletions packages/kbn-plugin-check/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"rules": {
"import/no-extraneous-dependencies": "off"
}
}
17 changes: 17 additions & 0 deletions packages/kbn-plugin-check/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# @kbn/plugin-check

This package contains a CLI to detect inconsistencies between the manifest and Typescript types of a Kibana plugin. Future work will include automatically fixing these inconsistencies.

## Usage

To check a single plugin, run the following command from the root of the Kibana repo:

```sh
node scripts/plugin_check --plugin pluginName
```

To check all plugins owned by a team, run the following:

```sh
node scripts/plugin_check --team @elastic/team_name
```
34 changes: 34 additions & 0 deletions packages/kbn-plugin-check/const.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* 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.
*/

/** Type types of plugin classes within a single plugin. */
export const PLUGIN_LAYERS = ['server', 'client'] as const;

/** The lifecycles a plugin class implements. */
export const PLUGIN_LIFECYCLES = ['setup', 'start'] as const;

/** An enum representing the dependency requirements for a plugin. */
export const PLUGIN_REQUIREMENTS = ['required', 'optional'] as const;

/** An enum representing the manifest requirements for a plugin. */
export const MANIFEST_REQUIREMENTS = ['required', 'optional', 'bundle'] as const;

/** The state of a particular dependency as it relates to the plugin manifest. */
export const MANIFEST_STATES = ['required', 'optional', 'bundle', 'missing'] as const;

/**
* The state of a particular dependency as it relates to a plugin class. Includes states where the
* plugin is missing properties to determine that state.
*/
export const PLUGIN_STATES = ['required', 'optional', 'missing', 'no class', 'unknown'] as const;

/** The state of the dependency for the entire plugin. */
export const DEPENDENCY_STATES = ['required', 'optional', 'mismatch'] as const;

/** An enum representing how the dependency status was derived from the plugin class. */
export const SOURCE_OF_TYPE = ['implements', 'method', 'none'] as const;
217 changes: 217 additions & 0 deletions packages/kbn-plugin-check/dependencies/create_table.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,217 @@
/*
* 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 Table, { Table as TableType } from 'cli-table3';

import colors from 'colors/safe';

import { ToolingLog } from '@kbn/tooling-log';

import { PluginLayer, PluginLifecycle, PluginInfo, PluginStatuses, PluginState } from '../types';
import { PLUGIN_LAYERS, PLUGIN_LIFECYCLES } from '../const';
import { borders } from './table_borders';

// A lot of this logic is brute-force and ugly. It's a quick and dirty way to get the
// proof-of-concept working.
export const createTable = (
pluginInfo: PluginInfo,
statuses: PluginStatuses,
_log: ToolingLog
): TableType => {
const table = new Table({
colAligns: ['left', 'center', 'center', 'center', 'center', 'center', 'center'],
style: {
'padding-left': 2,
'padding-right': 2,
},
chars: borders.table,
});

const noDependencies = Object.keys(statuses).length === 0;
const noServerPlugin = pluginInfo.classes.server === null;
const noClientPlugin = pluginInfo.classes.client === null;
const noPlugins = noServerPlugin && noClientPlugin;

if (noDependencies || noPlugins) {
table.push([pluginInfo.name]);

if (noDependencies) {
table.push(['Plugin has no dependencies.']);
}

if (noPlugins)
table.push([
'Plugin has no client or server implementation.\nIt should be migrated to a package, or only be a requiredBundle.',
]);

return table;
}

/**
* Build and format the header cell for the plugin lifecycle column.
*/
const getLifecycleColumnHeader = (layer: PluginLayer, lifecycle: PluginLifecycle) =>
Object.entries(statuses).some(
([_name, statusObj]) => statusObj[layer][lifecycle].source === 'none'
)
? colors.red(lifecycle.toUpperCase())
: lifecycle.toUpperCase();

/**
* Build and format the header cell for the plugin layer column.
*/
const getLayerColumnHeader = (layer: PluginLayer) => {
if (!pluginInfo.classes[layer]) {
return [
{
colSpan: 2,
content: 'NO CLASS',
chars: borders.subheader,
},
];
}

return PLUGIN_LIFECYCLES.map((lifecycle) => ({
content: getLifecycleColumnHeader(layer, lifecycle),
chars: borders.subheader,
}));
};

/**
* True if the `PluginState` is one of the states that should be excluded from a
* mismatch check.
*/
const isExcludedState = (state: PluginState) =>
state === 'no class' || state === 'unknown' || state === 'missing';

const entries = Object.entries(statuses);
let hasPass = false;
let hasFail = false;
let hasWarn = false;

// Table Header
table.push([
{
colSpan: 3,
content: pluginInfo.name,
chars: borders.header,
},
{
colSpan: 2,
content: 'SERVER',
chars: borders.header,
},
{
colSpan: 2,
content: 'PUBLIC',
chars: borders.header,
},
]);

// Table Subheader
table.push([
{
content: '',
chars: borders.subheader,
},
{
content: 'DEPENDENCY',
chars: borders.subheader,
},
{
content: 'MANIFEST',
chars: borders.subheader,
},
...getLayerColumnHeader('server'),
...getLayerColumnHeader('client'),
]);

// Dependency Rows
entries
.sort(([nameA], [nameB]) => {
return nameA.localeCompare(nameB);
})
.forEach(([name, statusObj], index) => {
const { manifestState /* server, client*/ } = statusObj;
const chars = index === entries.length - 1 ? borders.lastDependency : {};
const states = PLUGIN_LAYERS.flatMap((layer) =>
PLUGIN_LIFECYCLES.flatMap((lifecycle) => statusObj[layer][lifecycle].pluginState)
);

// TODO: Clean all of this brute-force stuff up.
const getLifecycleCellContent = (state: string) => {
if (state === 'no class' || (manifestState === 'bundle' && state === 'missing')) {
return '';
} else if (manifestState === 'bundle' || (manifestState !== state && state !== 'missing')) {
return colors.red(state === 'missing' ? '' : state);
}

return state === 'missing' ? '' : state;
};

const hasNoMismatch = () =>
states.some((state) => state === manifestState) &&
states.filter((state) => state !== manifestState).every(isExcludedState);

const isValidBundle = () => manifestState === 'bundle' && states.every(isExcludedState);

const getStateLabel = () => {
if (hasNoMismatch() || isValidBundle()) {
hasPass = true;
return '✅';
} else if (!hasNoMismatch()) {
hasFail = true;
return '❌';
}

hasWarn = true;
return '❓';
};

const getLifecycleColumns = () => {
if (noClientPlugin && noServerPlugin) {
return [{ colSpan: 4, content: '' }];
}

return PLUGIN_LAYERS.flatMap((layer) => {
if (!pluginInfo.classes[layer]) {
return { colSpan: 2, content: '', chars };
}

return PLUGIN_LIFECYCLES.flatMap((lifecycle) => ({
content: getLifecycleCellContent(statusObj[layer][lifecycle].pluginState),
chars,
}));
});
};

table.push({
[getStateLabel()]: [
{ content: name, chars },
{
content:
manifestState === 'missing' ? colors.red(manifestState.toUpperCase()) : manifestState,
chars,
},
...getLifecycleColumns(),
],
});
});

table.push([
{
colSpan: 7,
content: `${hasWarn ? '❓ - dependency is entirely missing or unknown.\n' : ''}${
hasFail ? '❌ - dependency differs from the manifest.\n' : ''
}${hasPass ? '✅ - dependency matches the manifest.' : ''}`,
chars: borders.footer,
},
]);

return table;
};
Loading

0 comments on commit 2f92ce1

Please sign in to comment.