[eslint] fix and skip violations for cross-boundary imports

[spalger]

⚠️ Dearest reviewers

Sorry this isn't short, but there's lots of context here.

You're probably familiar with the @kbn/eslint/no-restricted-paths rule. This rule attempted to prevent the problems caused by importing server code in public code (and vice-versa) but it was implemented in a very generic way which made it difficult to understand and harder to make work for everything. Now it's very common for people to use // eslint-disable comments to disable this rule, but the new packaging project relies on these assertions so we need to make them actually work and then break our habits of disabling the rule. This PR is a step in that direction by implementing a new rule, @kbn/imports/no_boundary_crossing, that's described below and removing the @kbn/eslint/no-restricted-paths rule.

In short, the purpose of this rule is similar to @kbn/eslint/no-restricted-paths except that:

• import type statements can reach into any package, including from the server to the browser and back (though circular deps are still not supported)
• All of the patterns which seemed valid in the repo are handled by this rule (there are a few exceptions, but the plan is to remove these edge cases because supporting them would introduce too much ambiguity)
• Once this rule is fully implemented and all disables are removed it will become protected (can't be disabled).

For now, any instance of // eslint-disable comments for the @kbn/imports/no_boundary_crossing rule is an example of an import that won't be supported in the new package system and is likely unintentional and either should be removed or moved out to a different location.

We're starting by disabling the rule in these locations and once this is merged I'll be working with teams and creating issues, providing details about the steps necessary to get rid of these disables and pave the way for the relevant code to be migrated to packages.

A recently opened RFC will reduce some of the complexity of this rule. It's currently easy to know when you're looking at a Jest unit test file, we want similar clarity for other types of files.

What's going on here?

An early component of the internal dependency management project (package all the things) is getting a handle on the purpose of each file in the repository. This will ease the process of migrating the code to packages, and help identify likely problem areas for migration.

To accomplish this I've added a new ESLint rule @kbn/imports/no_boundary_crossing which classifies the linted source file with @kbn/repo-source-classifier, traverses the AST to find all imports, resolves those imports, and then classifies the imported file. It then makes sure that the source file is allowed to import each thing it does import and report errors when we're breaking the rules.

The types of code that can exist are:

• server package: plugin code in the root server/ directory, eventually this will include packages of type server-plugin or server-shared
• browser package: plugin code in the root public/ directory (and a few others in specific plugins), eventually this will include packages of type browser-plugin or browser-shared
• common packages: includes any existing package, plugin code in root common/ directories, (and a few others in specific plugins), Eventually this will include common-shared packages
• tests or mocks: code that is loaded by jest/storybook and mocks/helpers intended for use by that code. These files usually live along side package code but will have a separate dependency tree and are pieces of code which should never end up in the product.
• static: static files, currently any .json file or things loaded via raw-loader in browser code
• tooling: scripts, config files for tools like eslint, webpack, etc.
• non-package: code that lives outside of packages/plugins or doesn't fit into other more specific categories. Once the package project is complete this category should be limited to just @kbn/pm

This is a map of types to the types they are allowed to import:

• NOTE: anything can be import type'd
• non-package: non-package, server package, browser package, common package or static
• server package: common package, server package, or static
• browser package: common package, browser package, or static
• common package: common package orstatic
• static: static files are not allowed to have dependencies
• tests or mocks: anything
• tooling: anything

To inspect how files are being classified, you can use the node scripts/classify_source --help CLI.

Here is a detailed description of how files are classified by this PR. These rules are applied in order and the first rule to match the path in question defines the type of the file:

• If the file has a .json extension it is of type static
• If the file meets any of the following conditions it is test or mocks:
  • starts with mock_*
  • has a name (excluding the extension) that we've identified as only used for test helpers/mocks/stories (see RANDOM_TEST_FILE_NAMES list)
  • starts with _ and includes any well known "test tag" (see TEST_TAG list)
  • has a "tag" (eg. name.tag.ts) that matches a well known "test tag" (see TEST_TAG list)
  • is in a directory that we have identified is only used for test code (see TEST_DIR list)
• If the file meets any of the following conditions it is tooling:
  • is within a scripts directory (excluding src/plugins/data/server/scripts)
  • has the filename webpack.config and is not in the @kbn/optimizer package
• If the file is not identified as being within a package or "synthetic package" (aka plugin) then the type is non-package
• If the package id of the package includes the word mocks, storybook or test then the type is test or mocks
• If the package is a bazel-managed package the code is common package
• If the file is in the @kbn/core package and within the root types directory then the type is common package
• If the file is in the @kbn/canvas-plugin package then a set of custom rules is applied for canvas, as agreed upon with that team
• If the file is in the root public or static directory of a package/plugin then it is of type browser package
• If the file is in the root server directory of a package/plugin then it is of type server package
• All other files are of type common package

These rules are complicated, but they are only necessary during the transition to packages. Once all of the code is migrated to package (and packages have types) the code will describe it's own type. The only exception to this rule is test code, which lives along-side implementation code in packages and needs to be properly removed from package before they are used for Kibana, or included in the distributable. Because of this we recently opened an RFC to choose standard naming we can use across the repository for test-specific code, tooling, and other specific types of code.

In the cases where the classifier was identified to be making the right decision, but the code was still violating the rules, an // eslint-disable-next-line @kbn/imports/no_boundary_crossing comment was used to disable the ESLint violation in that spot. I will be opening issues for each team which has these comments, as they need to be resolved ASAP so that we can protect this ESLint rule and prevent additional violations from being committed to the repository (which will simply make the migration to packages more challenging for teams).

[spalger]

"scripts" directories are used to store tooling files, which are only necessary for local development and not a part of package source code. I hope this rename is acceptable

[spalger]

helpers is a pretty ambiguous name, so I renamed the directory to test_helpers to be more clear about the purpose of the code and allow the classifier to assert that this is test code which shouldn't be imported by non-test code.

[marshallmain]

Detection alerts changes LGTM

[efegurkan]

Enterprise Search look good

[rshen91]

Shared UX services changes LGTM thanks!

[tsullivan]

app services / reporting services changes are LGTM

[ThomThomson]

Presentation team import changes LGTM!

[mistic]

Changes on files under operations team code owners LGTM

[dominiqueclarke]

Uptime changes lgtm. Thanks!

[dominiqueclarke]

LGTM

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: b8eda14
• Storybooks Preview

Failed CI Steps

• Security Solution Tests #3
• APM Cypress Tests
• APM Cypress Tests
• Osquery Cypress Tests

Test Failures

• [job] [logs] Security Solution Tests #3 / Inspect Hosts stats and tables "before all" hook for "inspects the All Hosts Table"

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id	before	after	diff
cases	16	15	-1
licenseManagement	1	0	-1
securitySolution	78	76	-2
triggersActionsUi	5	3	-2
total			-6

ESLint disabled line counts

id	before	after	diff
@kbn/core-config-server-internal	0	1	+1
@kbn/eslint-plugin-eslint	3	1	-2
@kbn/home-sample-data-card	0	1	+1
@kbn/securitysolution-io-ts-utils	0	1	+1
@kbn/securitysolution-t-grid	0	1	+1
@kbn/shared-ux-services	1	3	+2
alerting	60	59	-1
apm	87	81	-6
canvas	33	34	+1
cases	73	68	-5
controls	1	2	+1
data	60	46	-14
dataVisualizer	39	38	-1
embeddable	6	5	-1
expressionGauge	1	2	+1
expressionHeatmap	1	2	+1
expressionLegacyMetricVis	4	5	+1
expressionMetricVis	1	2	+1
expressions	18	16	-2
expressionTagcloud	1	2	+1
expressionXY	0	1	+1
fleet	52	51	-1
indexManagement	16	14	-2
infra	129	94	-35
ingestPipelines	16	15	-1
interactiveSetup	11	10	-1
kibanaUtils	11	12	+1
licenseApiGuard	0	1	+1
monitoring	25	18	-7
observability	47	44	-3
presentationUtil	9	11	+2
reporting	6	3	-3
savedObjectsManagement	5	4	-1
screenshotting	5	6	+1
security	26	20	-6
securitySolution	420	419	-1
share	10	7	-3
synthetics	61	58	-3
timelines	37	36	-1
triggersActionsUi	190	183	-7
ux	11	10	-1
total			-89

Total ESLint disabled count

id	before	after	diff
@kbn/core-config-server-internal	0	1	+1
@kbn/eslint-plugin-eslint	12	10	-2
@kbn/home-sample-data-card	0	1	+1
@kbn/securitysolution-io-ts-utils	0	1	+1
@kbn/securitysolution-t-grid	0	1	+1
@kbn/shared-ux-services	1	3	+2
alerting	61	60	-1
apm	101	95	-6
canvas	37	38	+1
cases	89	83	-6
controls	1	2	+1
data	62	48	-14
dataVisualizer	39	38	-1
embeddable	8	7	-1
expressionGauge	1	2	+1
expressionHeatmap	1	2	+1
expressionLegacyMetricVis	4	5	+1
expressionMetricVis	1	2	+1
expressions	23	21	-2
expressionTagcloud	2	3	+1
expressionXY	0	1	+1
fleet	58	57	-1
indexManagement	20	18	-2
infra	136	101	-35
ingestPipelines	16	15	-1
interactiveSetup	11	10	-1
kibanaUtils	13	14	+1
licenseApiGuard	0	1	+1
licenseManagement	2	1	-1
monitoring	32	25	-7
observability	53	50	-3
presentationUtil	11	13	+2
reporting	7	4	-3
savedObjectsManagement	5	4	-1
screenshotting	8	9	+1
security	28	22	-6
securitySolution	498	495	-3
share	12	9	-3
synthetics	67	64	-3
timelines	39	38	-1
triggersActionsUi	195	186	-9
ux	14	13	-1
total			-95

History

• 💛 Build #61785 was flaky 4faf899
• 💚 Build #61741 succeeded c9c30ef
• 💚 Build #61595 succeeded d5589a8
• 💛 Build #61570 was flaky 2e813bc
• 💔 Build #61548 failed a80fe93

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[drewdaemon]

Vis editors changes LGTM!

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

[simianhacker]

LGTM

[TinaHeiligers]

@spalger how would we resolve this?

[spalger]

The problem here is that test_utils are assumed to be code only intended for use by tests, but it's imported from the index.ts file of the package, meaning that it's included in the distributable and these test helpers are exposed to everyone who imports this package. If these helpers are actually used in other packages then we will need to move them to a new test-helpers package (though we can't define the type of the package yet, that would be it's purpose and it's name should reflect that somehow). Then we can import the @kbn/core-config-server-test-helpers package in the local tests and in the other packages which use them.

If these test helpers are not actually needed outside this package then this export ... from statement can just be removed.