implement "plugin" package type #149370

spalger · 2023-01-24T00:35:46Z

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.

elasticmachine · 2023-01-24T14:56:02Z

Pinging @elastic/kibana-operations (Team:Operations)

TinaHeiligers · 2023-01-24T19:36:58Z

I'm going to need some help identifying what we need to get test coverage for.

@lukeelmers your input here would help a lot!
I can take a guess but don't have all the historical knowledge that other folks on the @elastic/kibana-core might have.

TinaHeiligers

I left a few questions and recommend getting input from other team members on test coverage but it seems to be good. I'll give it a tentative LGTM. There are still a few days to follow up with changes if needed.

...re/plugins/core-plugins-server-internal/src/discovery/plugin_manifest_from_plugin_package.ts

packages/core/base/core-base-server-mocks/src/core_context.mock.ts

TinaHeiligers

Tentative LGTM pending questions and test-coverage input from other team members.

pgayvallet

To be perfectly honest, I'm still not sure to understand why exactly we want to change the plugin scanning system to be based on our generated plugin list instead of just filesystem scan as it's done today.

I get the fact that it's more performant, given that packages can be 'anywhere' and that therefore the plugin discovery system would take more time if it was only based on FS scan, but still, having the implementation diverging between 3rd parties (and test?) plugins and our internal ones feels somewhat wrong to me.

Now, this is just my gut feeling, I don't have any strong argument against this PR's changes.

A few questions and remarks that I'd like covered before the merge though:

pgayvallet · 2023-01-25T07:35:03Z

...ugins/core-plugins-server-internal/src/discovery/plugin_manifest_from_plugin_package.test.ts

+import { PluginType } from '@kbn/core-base-common';
+import { pluginManifestFromPluginPackage } from './plugin_manifest_from_plugin_package';
+
+const kibanaVersion = `1.${Math.round(10 * Math.random())}.1`;


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

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

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

pgayvallet · 2023-01-25T07:36:47Z

...re/plugins/core-plugins-server-internal/src/discovery/plugin_manifest_from_plugin_package.ts

+    id: manifest.plugin.id,
+    version: '1.0.0',
+    enabledOnAnonymousPages: manifest.plugin.enabledOnAnonymousPages,
+    serviceFolders: manifest.serviceFolders,


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

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.

pgayvallet · 2023-01-25T07:40:13Z

packages/core/plugins/core-plugins-server-internal/src/discovery/plugins_discovery.ts

+  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,
+      })
+    ),


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

See 6e7fcff (#149370)

pgayvallet · 2023-01-25T07:41:52Z

packages/core/plugins/core-plugins-server-internal/src/discovery/plugins_discovery.ts

+      const manifest = pluginManifestFromPluginPackage(
+        coreContext.env.packageInfo.version,
+        pkg.manifest
+      );


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.

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

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

pgayvallet · 2023-01-25T07:43:51Z

packages/core/plugins/core-plugins-server-internal/src/discovery/plugins_discovery.ts

+    })
+  );
+
+  const discoveryResults$ = merge(fsDiscovery$, pluginPkgDiscovery$).pipe(shareReplay());


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?

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.

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

pgayvallet · 2023-01-25T07:47:09Z

packages/kbn-config/src/env.ts

@@ -64,6 +66,8 @@ export class Env {
  public readonly logDir: string;
  /** @internal */
  public readonly pluginSearchPaths: readonly string[];
+  /** @internal */
+  public readonly repoPackages?: readonly Package[];


NIT: update unit tests accordingly

See 352c68a (#149370)

spalger · 2023-01-25T14:08:56Z

I get the fact that it's more performant, given that packages can be 'anywhere' and that therefore the plugin discovery system would take more time if it was only based on FS scan, but still, having the implementation diverging between 3rd parties (and test?) plugins and our internal ones feels somewhat wrong to me.

Yeah, the locations of packages is flexible, and the manifest approach to package discovery (listing dirs to search somewhere) is limiting and annoying for normal devs. Being able to put packages anywhere is a really good DX and worth it IMO, but we can't just scan every file in the repo to find all the kibana.jsonc files. We do this in bootstrap because we know we're in a git repo and can list all the files very efficiently, and we do it in the build process because we have the list of packages we are adding to the build. Either way, this approach brings a lot of consistency to many other operations, which is why this feels so good to me. Third-party plugins might also get a similar treatment, but we just haven't solved that problem yet. It should be really simple to add third-party packages to the standard package list, we just have to figure out the semantics around that, and then we can deprecate the old file-scanning code and remove it at some point.

…n-package-type

…bana into implement/plugin-package-type

kibana-ci · 2023-01-30T17:47:54Z

💚 Build Succeeded

Buildkite Build
Commit: b4349da

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/kibana-manifest-schema`	85	107	+22

Unknown metric groups

API count

id	before	after	diff
`@kbn/kibana-manifest-schema`	86	108	+22

History

💚 Build #102683 succeeded 16d5438
💔 Build #102676 failed 6e7fcff
💚 Build #102203 succeeded fc2b0c4
💛 Build #102015 was flaky eea7254

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

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 elastic#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.

Fixes #149344 This PR migrates all plugins to packages automatically. It does this using `node scripts/lint_packages` to automatically migrate `kibana.json` files to `kibana.jsonc` files. By doing this automatically we can simplify many build and testing procedures to only support packages, and not both "packages" and "synthetic packages" (basically pointers to plugins). The majority of changes are in operations related code, so we'll be having operations review this before marking it ready for review. The vast majority of the code owners are simply pinged because we deleted all `kibana.json` files and replaced them with `kibana.jsonc` files, so we plan on leaving the PR ready-for-review for about 24 hours before merging (after feature freeze), assuming we don't have any blockers (especially from @elastic/kibana-core since there are a few core specific changes, though the majority were handled in #149370). --------- Co-authored-by: kibanamachine <[email protected]>

Fixes elastic#149344 This PR migrates all plugins to packages automatically. It does this using `node scripts/lint_packages` to automatically migrate `kibana.json` files to `kibana.jsonc` files. By doing this automatically we can simplify many build and testing procedures to only support packages, and not both "packages" and "synthetic packages" (basically pointers to plugins). The majority of changes are in operations related code, so we'll be having operations review this before marking it ready for review. The vast majority of the code owners are simply pinged because we deleted all `kibana.json` files and replaced them with `kibana.jsonc` files, so we plan on leaving the PR ready-for-review for about 24 hours before merging (after feature freeze), assuming we don't have any blockers (especially from @elastic/kibana-core since there are a few core specific changes, though the majority were handled in elastic#149370). --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit 1b85815) # Conflicts: # .github/CODEOWNERS # src/dev/build/tasks/bundle_fleet_packages.ts # src/dev/tsconfig.json # yarn.lock

# Backport This will backport the following commits from `main` to `8.7`: - [[packages] migrate all plugins to packages (#148130)](#148130)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  --------- Co-authored-by: Spencer <[email protected]>

spalger added 4 commits January 23, 2023 18:33

implement "plugin" package type

04d5557

expand on serviceFolders desc

f421c94

update jest snapshots

fdec39c

extract some logic to get some unit test coverage

eea7254

spalger added Team:Operations Team label for Operations Team release_note:skip Skip the PR/issue when compiling release notes labels Jan 24, 2023

spalger marked this pull request as ready for review January 24, 2023 14:55

spalger requested review from a team as code owners January 24, 2023 14:55

Merge branch 'main' into implement/plugin-package-type

fc2b0c4

jbudz approved these changes Jan 24, 2023

View reviewed changes

TinaHeiligers reviewed Jan 24, 2023

View reviewed changes

TinaHeiligers approved these changes Jan 24, 2023

View reviewed changes

pgayvallet reviewed Jan 25, 2023

View reviewed changes

spalger added 4 commits January 25, 2023 10:19

add tests for repoPackages property in env

352c68a

Merge branch 'main' of github.com:elastic/kibana into implement/plugi…

c5f27b6

…n-package-type

Merge branch 'implement/plugin-package-type' of github.com:spalger/ki…

39198ff

…bana into implement/plugin-package-type

add tests about filtering and mapping plugin packages to PluginWrappers

6e7fcff

spalger requested a review from pgayvallet January 25, 2023 16:23

fix tests after cherry-pick

16d5438

pgayvallet approved these changes Jan 27, 2023

View reviewed changes

Merge branch 'main' into implement/plugin-package-type

b4349da

spalger enabled auto-merge (squash) January 30, 2023 16:35

spalger merged commit 376bed5 into elastic:main Jan 30, 2023

spalger deleted the implement/plugin-package-type branch January 30, 2023 17:47

kibanamachine added the v8.7.0 label Jan 30, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jan 30, 2023

spalger mentioned this pull request Feb 1, 2023

[packages] migrate all plugins to packages #148130

Merged

pgayvallet mentioned this pull request May 16, 2023

Plugins do not compile #157440

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement "plugin" package type #149370

implement "plugin" package type #149370

spalger commented Jan 24, 2023 •

edited

Loading

elasticmachine commented Jan 24, 2023

TinaHeiligers commented Jan 24, 2023 •

edited by spalger

Loading

TinaHeiligers left a comment •

edited

Loading

TinaHeiligers left a comment

pgayvallet left a comment

pgayvallet Jan 25, 2023

spalger Jan 25, 2023 •

edited

Loading

pgayvallet Jan 26, 2023

pgayvallet Jan 25, 2023 •

edited

Loading

spalger Jan 25, 2023

pgayvallet Jan 25, 2023

spalger Jan 25, 2023

pgayvallet Jan 25, 2023 •

edited

Loading

spalger Jan 25, 2023

pgayvallet Jan 26, 2023

pgayvallet Jan 25, 2023

spalger Jan 25, 2023

pgayvallet Jan 26, 2023

pgayvallet Jan 25, 2023

spalger Jan 25, 2023

spalger commented Jan 25, 2023 •

edited

Loading

kibana-ci commented Jan 30, 2023

API count

implement "plugin" package type #149370

implement "plugin" package type #149370

Conversation

spalger commented Jan 24, 2023 • edited Loading

elasticmachine commented Jan 24, 2023

TinaHeiligers commented Jan 24, 2023 • edited by spalger Loading

TinaHeiligers left a comment • edited Loading

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Jan 25, 2023 • edited Loading

kibana-ci commented Jan 30, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

History

spalger commented Jan 24, 2023 •

edited

Loading

TinaHeiligers commented Jan 24, 2023 •

edited by spalger

Loading

TinaHeiligers left a comment •

edited

Loading

spalger Jan 25, 2023 •

edited

Loading

pgayvallet Jan 25, 2023 •

edited

Loading

pgayvallet Jan 25, 2023 •

edited

Loading

spalger commented Jan 25, 2023 •

edited

Loading