Skip to content

Commit

Permalink
allow NP plugins --plugin-path in production, logs a warning
Browse files Browse the repository at this point in the history
  • Loading branch information
pgayvallet committed Dec 19, 2019
1 parent ecf8657 commit 3f78c81
Show file tree
Hide file tree
Showing 10 changed files with 129 additions and 27 deletions.
12 changes: 1 addition & 11 deletions src/cli/serve/serve.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,23 +140,12 @@ function applyConfigOverrides(rawConfig, opts, extraCliOptions) {
}

set('plugins.scanDirs', _.compact([].concat(get('plugins.scanDirs'), opts.pluginDir)));

set(
'plugins.paths',
_.compact(
[].concat(
get('plugins.paths'),
opts.pluginPath,
opts.runExamples
? [
// Ideally this would automatically include all plugins in the examples dir
fromRoot('examples/demo_search'),
fromRoot('examples/search_explorer'),
fromRoot('examples/embeddable_examples'),
fromRoot('examples/embeddable_explorer'),
]
: [],

XPACK_INSTALLED && !opts.oss ? [XPACK_DIR] : []
)
)
Expand Down Expand Up @@ -253,6 +242,7 @@ export default function(program) {
silent: !!opts.silent,
watch: !!opts.watch,
repl: !!opts.repl,
runExamples: !!opts.runExamples,
// We want to run without base path when the `--run-examples` flag is given so that we can use local
// links in other documentation sources, like "View this tutorial [here](http://localhost:5601/app/tutorial/xyz)".
// We can tell users they only have to run with `yarn start --run-examples` to get those
Expand Down
1 change: 1 addition & 0 deletions src/core/server/config/__mocks__/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export function getEnvOptions(options: DeepPartial<EnvOptions> = {}): EnvOptions
basePath: false,
optimize: false,
oss: false,
runExamples: false,
...(options.cliArgs || {}),
},
isDevClusterMaster:
Expand Down
6 changes: 6 additions & 0 deletions src/core/server/config/__snapshots__/env.test.ts.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions src/core/server/config/env.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,3 +152,25 @@ test('pluginSearchPaths does not contains x-pack plugins path if --oss flag is t

expect(env.pluginSearchPaths).not.toContain('/some/home/dir/x-pack/plugins');
});

test('pluginSearchPaths contains examples plugins path if --run-examples flag is true', () => {
const env = new Env(
'/some/home/dir',
getEnvOptions({
cliArgs: { runExamples: true },
})
);

expect(env.pluginSearchPaths).toContain('/some/home/dir/examples');
});

test('pluginSearchPaths does not contains examples plugins path if --run-examples flag is false', () => {
const env = new Env(
'/some/home/dir',
getEnvOptions({
cliArgs: { runExamples: false },
})
);

expect(env.pluginSearchPaths).not.toContain('/some/home/dir/examples');
});
6 changes: 4 additions & 2 deletions src/core/server/config/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export interface CliArgs {
optimize: boolean;
open: boolean;
oss: boolean;
runExamples: boolean;
}

export class Env {
Expand Down Expand Up @@ -104,10 +105,11 @@ export class Env {

this.pluginSearchPaths = [
resolve(this.homeDir, 'src', 'plugins'),
options.cliArgs.oss ? '' : resolve(this.homeDir, 'x-pack', 'plugins'),
...(options.cliArgs.oss ? [] : [resolve(this.homeDir, 'x-pack', 'plugins')]),
resolve(this.homeDir, 'plugins'),
...(options.cliArgs.runExamples ? [resolve(this.homeDir, 'examples')] : []),
resolve(this.homeDir, '..', 'kibana-extra'),
].filter(Boolean);
];

this.cliArgs = Object.freeze(options.cliArgs);
this.configs = Object.freeze(options.configs);
Expand Down
56 changes: 47 additions & 9 deletions src/core/server/plugins/discovery/plugins_discovery.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,14 @@
*/

import { mockPackage, mockReaddir, mockReadFile, mockStat } from './plugins_discovery.test.mocks';
import { rawConfigServiceMock } from '../../config/raw_config_service.mock';
import { loggingServiceMock } from '../../logging/logging_service.mock';

import { resolve } from 'path';
import { first, map, toArray } from 'rxjs/operators';

import { ConfigService, Env } from '../../config';
import { rawConfigServiceMock } from '../../config/raw_config_service.mock';
import { getEnvOptions } from '../../config/__mocks__/env';
import { loggingServiceMock } from '../../logging/logging_service.mock';
import { PluginWrapper } from '../plugin';
import { PluginsConfig, PluginsConfigType, config } from '../plugins_config';
import { discover } from './plugins_discovery';
Expand All @@ -37,6 +38,7 @@ const TEST_PLUGIN_SEARCH_PATHS = {
const TEST_EXTRA_PLUGIN_PATH = resolve(process.cwd(), 'my-extra-plugin');

const logger = loggingServiceMock.create();

beforeEach(() => {
mockReaddir.mockImplementation((path, cb) => {
if (path === TEST_PLUGIN_SEARCH_PATHS.nonEmptySrcPlugins) {
Expand Down Expand Up @@ -183,11 +185,47 @@ test('properly iterates through plugin search locations', async () => {
)})`,
]);

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.",
],
]
`);
expect(loggingServiceMock.collect(logger).warn).toEqual([]);
});

test('logs a warning about --plugin-paths when used in production', async () => {
mockPackage.raw = {
branch: 'master',
version: '1.2.3',
build: {
distributable: true,
number: 1,
sha: '',
},
};

const env = Env.createDefault(
getEnvOptions({
cliArgs: { dev: false, envName: 'production' },
})
);
const configService = new ConfigService(
rawConfigServiceMock.create({ rawConfig: { plugins: { paths: [TEST_EXTRA_PLUGIN_PATH] } } }),
env,
logger
);
await configService.setSchema(config.path, config.schema);

const rawConfig = await configService
.atPath<PluginsConfigType>('plugins')
.pipe(first())
.toPromise();

discover(new PluginsConfig(rawConfig, env), {
coreId: Symbol(),
configService,
env,
logger,
});

expect(loggingServiceMock.collect(logger).warn).toEqual([
[
`Explicit plugin paths [${TEST_EXTRA_PLUGIN_PATH}] should only be used in development. Relative imports may not work properly in production.`,
],
]);
});
4 changes: 2 additions & 2 deletions src/core/server/plugins/discovery/plugins_discovery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ export function discover(config: PluginsConfig, coreContext: CoreContext) {
const log = coreContext.logger.get('plugins-discovery');
log.debug('Discovering plugins...');

if (config.additionalPluginPaths.length) {
if (config.additionalPluginPaths.length && coreContext.env.mode.prod) {
log.warn(
`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.`
);
}

Expand Down
44 changes: 44 additions & 0 deletions src/core/server/plugins/plugins_config.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { PluginsConfig, PluginsConfigType } from './plugins_config';
import { Env } from '../config';
import { getEnvOptions } from '../config/__mocks__/env';

describe('PluginsConfig', () => {
it('retrieves additionalPluginPaths from config.paths when in production mode', () => {
const env = Env.createDefault(getEnvOptions({ cliArgs: { dev: false } }));
const rawConfig: PluginsConfigType = {
initialize: true,
paths: ['some-path', 'another-path'],
};
const config = new PluginsConfig(rawConfig, env);
expect(config.additionalPluginPaths).toEqual(['some-path', 'another-path']);
});

it('retrieves additionalPluginPaths from config.paths when in development mode', () => {
const env = Env.createDefault(getEnvOptions({ cliArgs: { dev: true } }));
const rawConfig: PluginsConfigType = {
initialize: true,
paths: ['some-path', 'another-path'],
};
const config = new PluginsConfig(rawConfig, env);
expect(config.additionalPluginPaths).toEqual(['some-path', 'another-path']);
});
});
4 changes: 1 addition & 3 deletions src/core/server/plugins/plugins_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ export const config = {

/**
* Defines an array of directories where another plugin should be loaded from.
* Should only be used in a development environment.
*/
paths: schema.arrayOf(schema.string(), { defaultValue: [] }),
}),
Expand All @@ -55,7 +54,6 @@ export class PluginsConfig {
constructor(rawConfig: PluginsConfigType, env: Env) {
this.initialize = rawConfig.initialize;
this.pluginSearchPaths = env.pluginSearchPaths;
// Only allow custom plugin paths in dev.
this.additionalPluginPaths = env.mode.dev ? rawConfig.paths : [];
this.additionalPluginPaths = rawConfig.paths;
}
}
1 change: 1 addition & 0 deletions src/test_utils/kbn_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export function createRootWithSettings(
repl: false,
basePath: false,
optimize: false,
runExamples: false,
oss: true,
...cliArgs,
},
Expand Down

0 comments on commit 3f78c81

Please sign in to comment.