Allow optional OSS to X-Pack dependencies by jportner · Pull Request #107432 · elastic/kibana

jportner · 2021-08-02T17:49:34Z

This is no longer necessary now that we validate OSS to X-Pack plugin dependencies in the plugins service. Since there is no way to differentiate "optional" and "required" dependencies in ESLint rules, this rule needs to be removed to allow such optional dependencies.

afharo · 2021-08-03T11:25:40Z

src/core/server/plugins/plugins_service.ts

+          const requiredPlugin = pluginEnableStatuses.get(id);
+          if (requiredPlugin && XPACK_PATH_REGEX.test(requiredPlugin.plugin.path)) {
+            throw new Error(
+              `X-Pack plugin or bundle with id "${id}" is required by OSS plugin "${pluginName}", which is prohibited.`


Should we extend the error message to suggest making it optional?

Done in daebba1 😄

joshdover · 2021-08-03T11:38:47Z

src/core/server/plugins/plugins_service.ts

+const OSS_PATH_REGEX = /[\/|\\]src[\/|\\]plugins[\/|\\]/; // Matches src/plugins directory on POSIX and Windows
+const XPACK_PATH_REGEX = /[\/|\\]x-pack[\/|\\]plugins[\/|\\]/; // Matches x-pack/plugins directory on POSIX and Windows
+


I was thinking it'd be nice if we added a source property to the PluginWrapper class inside src/core/server/plugins/plugin.ts and populated it in the constructor. Then we could use this information in other places as well rather than only inside the service.

I'm fairly confident that it would be a small change to this PR, WDYT?

Just to clarify: the PluginWrapper class already has a path field.

Do you envision the source field as a string literal such as 'x-pack' | 'oss'?

Edit: done in daebba1.

src/core/server/plugins/plugins_service.ts

…cies

* Improve error message * Add source field to PluginWrapper

joshdover

LGTM, thanks for making those changes

mshustov · 2021-08-04T09:03:03Z

src/core/server/plugins/plugins_service.ts

      }

+      // validate that OSS plugins do not have required dependencies on X-Pack plugins
+      if (plugin.source === 'oss') {


optional: the validation path is growing too large. @elastic/kibana-core we need to move it under a dedicated method to simplify reading.

Done in b6a38a7

mshustov · 2021-08-04T09:03:55Z

src/core/server/plugins/plugins_service.test.ts

+        await expectError();
+      });
+
+      it('throws if an OSS plugin requires an X-Pack bundle', async () => {


should we add an explicit test when an external plugin depends on oss and x-pack?

Done in 07bb8ec

I'll merge this now, if you have any additional concerns about tests I'd be happy to address them in a follow up PR!

afharo

LGTM! Thanks for making those changes

…cies

kibanamachine · 2021-08-04T16:15:41Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #142676 succeeded daebba1
💚 Build #142402 succeeded 774ae1b

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

src/core/server/plugins/plugins_service.ts

kibanamachine · 2021-08-05T18:01:59Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

…-png-pdf-report-type * 'master' of github.com:elastic/kibana: (392 commits) update linting doc (elastic#105748) [APM] Various improvements from elastic#104851 (elastic#107726) Update dependency @elastic/charts to v33.2.0 (master) (elastic#107842) Fix default route link on kibana homepage (elastic#107809) [APM] Invalidate trackPageview on route change (elastic#107741) Service map backend links (elastic#107317) [index patterns] index pattern create modal (elastic#101853) [RAC] integrating rbac search strategy with alert table (elastic#107242) [Security Solution] Siem signals -> alerts as data field and index aliases (elastic#106049) [Metrics UI] Add checkbox to optionally drop partial buckets (elastic#107676) [Metrics UI] Fix metric threshold preview regression (elastic#107674) Disable Product check in @elastic/elasticsearch-js (elastic#107642) [App Search] Migrate Crawler Status Indicator, Crawler Status Banner, and Crawl Request polling (elastic#107603) [Security Solution, Lists] Replace legacy imports from 'elasticsearch' package (elastic#107226) [maps] asset tracking tutorial (elastic#104552) [scripts/build_ts_refs] when using `--clean` initialize caches (elastic#107777) Upgrade EUI to v36.1.0 (elastic#107231) [RAC] [TGrid] Implements cell actions in the TGrid (elastic#107771) Realign cypress/ccs_integration with cypress/integration (elastic#107743) Allow optional OSS to X-Pack dependencies (elastic#107432) ... # Conflicts: # x-pack/examples/reporting_example/public/application.tsx # x-pack/examples/reporting_example/public/components/app.tsx # x-pack/plugins/canvas/public/services/legacy/stubs/reporting.ts # x-pack/plugins/reporting/common/types.ts # x-pack/plugins/reporting/public/lib/reporting_api_client/context.tsx # x-pack/plugins/reporting/public/management/mount_management_section.tsx # x-pack/plugins/reporting/public/management/report_listing.test.tsx # x-pack/plugins/reporting/public/plugin.ts # x-pack/plugins/reporting/public/share_context_menu/register_pdf_png_reporting.tsx # x-pack/plugins/reporting/server/export_types/printable_pdf/execute_job/index.ts

jportner added 2 commits August 2, 2021 13:45

Validate OSS to X-pack dependencies in plugins service

0792111

jportner requested a review from a team as a code owner August 2, 2021 17:49

jportner added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 v7.15.0 labels Aug 2, 2021

afharo reviewed Aug 3, 2021

View reviewed changes

joshdover reviewed Aug 3, 2021

View reviewed changes

mshustov reviewed Aug 3, 2021

View reviewed changes

src/core/server/plugins/plugins_service.ts Outdated Show resolved Hide resolved

jportner added 2 commits August 3, 2021 10:11

Merge branch 'master' into issue-106485-allow-optional-xpack-dependen…

c54c2b5

…cies

PR review feedback

daebba1

* Improve error message * Add source field to PluginWrapper

jportner requested review from afharo, joshdover and mshustov August 3, 2021 19:04

joshdover approved these changes Aug 3, 2021

View reviewed changes

mshustov reviewed Aug 4, 2021

View reviewed changes

afharo approved these changes Aug 4, 2021

View reviewed changes

jportner added 3 commits August 4, 2021 09:29

Merge branch 'master' into issue-106485-allow-optional-xpack-dependen…

89e3cc8

…cies

Refactor plugin dependency validation into separate method

b6a38a7

Add extra unit test cases for X-Pack and external plugin dependencies

07bb8ec

jportner requested a review from mshustov August 4, 2021 13:55

lukeelmers approved these changes Aug 5, 2021

View reviewed changes

src/core/server/plugins/plugins_service.ts Show resolved Hide resolved

jportner added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 5, 2021

jportner merged commit 66dbb88 into elastic:master Aug 5, 2021

jportner deleted the issue-106485-allow-optional-xpack-dependencies branch August 5, 2021 17:58

kibanamachine mentioned this pull request Aug 5, 2021

[7.x] Allow optional OSS to X-Pack dependencies (#107432) #107794

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 5, 2021

Allow optional OSS to X-Pack dependencies (elastic#107432)

7f96de0

kibanamachine added a commit that referenced this pull request Aug 5, 2021

Allow optional OSS to X-Pack dependencies (#107432) (#107794)

025b209

Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

streamich pushed a commit to vadimkibana/kibana that referenced this pull request Aug 8, 2021

Allow optional OSS to X-Pack dependencies (elastic#107432)

73afaff

lukasolson mentioned this pull request Oct 11, 2021

[Search] Unify data.search plugin #92802

Closed

10 tasks

Dosant mentioned this pull request Jan 25, 2022

data dependent on security breaks tests #123720

Closed

		const OSS_PATH_REGEX = /[\/\|\\]src[\/\|\\]plugins[\/\|\\]/; // Matches src/plugins directory on POSIX and Windows
		const XPACK_PATH_REGEX = /[\/\|\\]x-pack[\/\|\\]plugins[\/\|\\]/; // Matches x-pack/plugins directory on POSIX and Windows

Conversation

jportner commented Aug 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jportner Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joshdover left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afharo left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Aug 4, 2021

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

Uh oh!

kibanamachine commented Aug 5, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jportner Aug 3, 2021 •

edited

Loading