Allow custom NP plugin paths in production #53562

pgayvallet · 2019-12-19T08:51:13Z

Summary

PR does the following:

enable plugins added via --plugin-path or config's plugin.path property in production.
only log the plugin-path related warning when in production mode
remove env.staticDirFile as it was unused
moves the example plugins activation to pluginSearchPaths instead of pluginPath to avoid having to list them explicitly.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~

Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-12-19T10:47:25Z

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet · 2019-12-19T10:48:41Z

src/core/server/plugins/discovery/plugins_discovery.test.ts

-  expect(loggingServiceMock.collect(logger).warn).toMatchInlineSnapshot(`
-Array [
-  Array [
-    "Explicit plugin paths [${TEST_EXTRA_PLUGIN_PATH}] are only supported in development. Relative imports will not work in production.",
-  ],
-]
-`);


I let you imagine what this was doing when --update-snapshot....

Past me takes full responsibility 😧

pgayvallet · 2019-12-19T10:50:51Z

src/core/server/plugins/discovery/plugins_discovery.ts

-      `Explicit plugin paths [${config.additionalPluginPaths}] are only supported in development. Relative imports will not work in production.`
+      `Explicit plugin paths [${config.additionalPluginPaths}] should only be used in development. Relative imports may not work properly in production.`


Not sure if we should be even mode explicit by explaining that relative path used for the plugin should be the same in dev than in prod. But the root issue should be addressed by #40446 anyway.

I feel like this warning in production is pretty useless. If the plugin is working with --plugin-path this warning doesn't tell you anything useful that an admin can or should do.

I think we should either remove this completely or have a different warning only in dev that warns about relative paths will break if the plugin is installed in a different location in production. Though that warning would also be less relevant after #40446.

You are right, logging the warning in dev probably makes more sense than in production

elasticmachine · 2019-12-19T11:28:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 431e3f0

History

💚 Build #16649 succeeded 3f78c81
💔 Build #16645 failed 346b1ce

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

joshdover · 2019-12-20T17:32:33Z

src/cli/serve/serve.js

-              fromRoot('examples/embeddable_examples'),
-              fromRoot('examples/embeddable_explorer'),
-            ]
-          : [],


@stacey-gammon FYI you won't have to manually add these anymore

awesome, thanks!!

joshdover · 2019-12-20T17:36:16Z

src/core/server/plugins/discovery/plugins_discovery.test.ts

-  expect(loggingServiceMock.collect(logger).warn).toMatchInlineSnapshot(`
-Array [
-  Array [
-    "Explicit plugin paths [${TEST_EXTRA_PLUGIN_PATH}] are only supported in development. Relative imports will not work in production.",
-  ],
-]
-`);


Past me takes full responsibility 😧

joshdover · 2019-12-20T17:40:59Z

src/core/server/plugins/discovery/plugins_discovery.ts

-      `Explicit plugin paths [${config.additionalPluginPaths}] are only supported in development. Relative imports will not work in production.`
+      `Explicit plugin paths [${config.additionalPluginPaths}] should only be used in development. Relative imports may not work properly in production.`


I feel like this warning in production is pretty useless. If the plugin is working with --plugin-path this warning doesn't tell you anything useful that an admin can or should do.

I think we should either remove this completely or have a different warning only in dev that warns about relative paths will break if the plugin is installed in a different location in production. Though that warning would also be less relevant after #40446.

mshustov · 2019-12-24T08:17:53Z

src/core/server/plugins/discovery/plugins_discovery.test.ts

+  expect(loggingServiceMock.collect(logger).warn).toEqual([]);
+});
+
+test('logs a warning about --plugin-paths when used in production', async () => {


Suggested change

test('logs a warning about --plugin-paths when used in production', async () => {

test('logs a warning about --plugin-path when used in production', async () => {

…lugin-path-in-prod

kibanamachine · 2020-01-06T08:10:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: f61398b

History

💚 Build #16661 succeeded 431e3f0
💚 Build #16649 succeeded 3f78c81
💔 Build #16645 failed 346b1ce

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

* allow NP plugins --plugin-path in production, logs a warning * remove env.staticFilesDir (unused) * only show warning in development * update rendering tests snapshots * fix typo

* master: increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798)

* master: increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798) [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793)

* master: increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798) [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793) [SR] Enable component integration tests (elastic#53893)

* allow NP plugins --plugin-path in production, logs a warning * remove env.staticFilesDir (unused) * only show warning in development * update rendering tests snapshots * fix typo

…nsole-dependencies * 'master' of github.com:elastic/kibana: (33 commits) adds strict types to Alerting Client (elastic#53821) [Dashboard] Empty screen redesign (elastic#53681) Migrate config deprecations and `ShieldUser` functionality to the New Platform (elastic#53768) increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798) [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793) [SR] Enable component integration tests (elastic#53893) Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777) Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) ... # Conflicts: # yarn.lock

allow NP plugins --plugin-path in production, logs a warning

3f78c81

pgayvallet force-pushed the kbn-49927-allow-plugin-path-in-prod branch from 346b1ce to 3f78c81 Compare December 19, 2019 09:22

remove env.staticFilesDir (unused)

431e3f0

pgayvallet added Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.6.0 v8.0.0 labels Dec 19, 2019

pgayvallet added the release_note:skip Skip the PR/issue when compiling release notes label Dec 19, 2019

pgayvallet commented Dec 19, 2019

View reviewed changes

pgayvallet marked this pull request as ready for review December 19, 2019 11:29

pgayvallet requested a review from a team as a code owner December 19, 2019 11:29

joshdover approved these changes Dec 20, 2019

View reviewed changes

mshustov reviewed Dec 24, 2019

View reviewed changes

pgayvallet added 4 commits January 6, 2020 07:22

Merge remote-tracking branch 'upstream/master' into kbn-49927-allow-p…

e8977d6

…lugin-path-in-prod

only show warning in development

d1a7579

update rendering tests snapshots

a8953ef

fix typo

f61398b

pgayvallet merged commit 24542c3 into elastic:master Jan 6, 2020

pgayvallet mentioned this pull request Jan 6, 2020

[7.x] Allow custom NP plugin paths in production (#53562) #53999

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow custom NP plugin paths in production #53562

Allow custom NP plugin paths in production #53562

pgayvallet commented Dec 19, 2019 •

edited

Loading

elasticmachine commented Dec 19, 2019

pgayvallet Dec 19, 2019

joshdover Dec 20, 2019

pgayvallet Dec 19, 2019

joshdover Dec 20, 2019

pgayvallet Jan 6, 2020

elasticmachine commented Dec 19, 2019

joshdover Dec 20, 2019

stacey-gammon Dec 20, 2019

joshdover Dec 20, 2019

joshdover Dec 20, 2019

mshustov Dec 24, 2019

kibanamachine commented Jan 6, 2020

		`Explicit plugin paths [${config.additionalPluginPaths}] are only supported in development. Relative imports will not work in production.`
		`Explicit plugin paths [${config.additionalPluginPaths}] should only be used in development. Relative imports may not work properly in production.`

	test('logs a warning about --plugin-paths when used in production', async () => {
	test('logs a warning about --plugin-path when used in production', async () => {

Allow custom NP plugin paths in production #53562

Allow custom NP plugin paths in production #53562

Conversation

pgayvallet commented Dec 19, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Dec 19, 2019

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

elasticmachine commented Dec 19, 2019

💚 Build Succeeded

History

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

kibanamachine commented Jan 6, 2020

💚 Build Succeeded

History

pgayvallet commented Dec 19, 2019 •

edited

Loading