From 6afb473631047689ee8e2711668662e4a7b11ee2 Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Fri, 5 Oct 2018 19:10:41 +0200
Subject: [PATCH 01/15] Implement new platform plugin discovery.

---
 .../config/__snapshots__/env.test.ts.snap     |   6 -
 src/core/server/config/env.ts                 |   2 -
 src/core/server/index.test.ts                 |  11 +
 src/core/server/index.ts                      |   6 +
 src/core/server/plugins/index.ts              |  30 +++
 .../server/plugins/plugin_discovery.test.ts   | 139 +++++++++++
 .../server/plugins/plugin_discovery_error.ts  |  43 ++++
 src/core/server/plugins/plugins_config.ts     |  58 +++++
 src/core/server/plugins/plugins_discovery.ts  | 230 ++++++++++++++++++
 src/core/server/plugins/plugins_service.ts    |  68 ++++++
 10 files changed, 585 insertions(+), 8 deletions(-)
 create mode 100644 src/core/server/plugins/index.ts
 create mode 100644 src/core/server/plugins/plugin_discovery.test.ts
 create mode 100644 src/core/server/plugins/plugin_discovery_error.ts
 create mode 100644 src/core/server/plugins/plugins_config.ts
 create mode 100644 src/core/server/plugins/plugins_discovery.ts
 create mode 100644 src/core/server/plugins/plugins_service.ts

diff --git a/src/core/server/config/__snapshots__/env.test.ts.snap b/src/core/server/config/__snapshots__/env.test.ts.snap
index ebe6c4ea83470..e1cc8df1bed2a 100644
--- a/src/core/server/config/__snapshots__/env.test.ts.snap
+++ b/src/core/server/config/__snapshots__/env.test.ts.snap
@@ -17,7 +17,6 @@ Env {
   "configs": Array [
     "/some/other/path/some-kibana.yml",
   ],
-  "corePluginsDir": "/test/cwd/core_plugins",
   "homeDir": "/test/cwd",
   "isDevClusterMaster": false,
   "logDir": "/test/cwd/log",
@@ -53,7 +52,6 @@ Env {
   "configs": Array [
     "/some/other/path/some-kibana.yml",
   ],
-  "corePluginsDir": "/test/cwd/core_plugins",
   "homeDir": "/test/cwd",
   "isDevClusterMaster": false,
   "logDir": "/test/cwd/log",
@@ -88,7 +86,6 @@ Env {
   "configs": Array [
     "/test/cwd/config/kibana.yml",
   ],
-  "corePluginsDir": "/test/cwd/core_plugins",
   "homeDir": "/test/cwd",
   "isDevClusterMaster": true,
   "logDir": "/test/cwd/log",
@@ -123,7 +120,6 @@ Env {
   "configs": Array [
     "/some/other/path/some-kibana.yml",
   ],
-  "corePluginsDir": "/test/cwd/core_plugins",
   "homeDir": "/test/cwd",
   "isDevClusterMaster": false,
   "logDir": "/test/cwd/log",
@@ -158,7 +154,6 @@ Env {
   "configs": Array [
     "/some/other/path/some-kibana.yml",
   ],
-  "corePluginsDir": "/test/cwd/core_plugins",
   "homeDir": "/test/cwd",
   "isDevClusterMaster": false,
   "logDir": "/test/cwd/log",
@@ -193,7 +188,6 @@ Env {
   "configs": Array [
     "/some/other/path/some-kibana.yml",
   ],
-  "corePluginsDir": "/some/home/dir/core_plugins",
   "homeDir": "/some/home/dir",
   "isDevClusterMaster": false,
   "logDir": "/some/home/dir/log",
diff --git a/src/core/server/config/env.ts b/src/core/server/config/env.ts
index b5a1092724b84..3c20c126f33fe 100644
--- a/src/core/server/config/env.ts
+++ b/src/core/server/config/env.ts
@@ -61,7 +61,6 @@ export class Env {
   }
 
   public readonly configDir: string;
-  public readonly corePluginsDir: string;
   public readonly binDir: string;
   public readonly logDir: string;
   public readonly staticFilesDir: string;
@@ -96,7 +95,6 @@ export class Env {
    */
   constructor(readonly homeDir: string, options: EnvOptions) {
     this.configDir = resolve(this.homeDir, 'config');
-    this.corePluginsDir = resolve(this.homeDir, 'core_plugins');
     this.binDir = resolve(this.homeDir, 'bin');
     this.logDir = resolve(this.homeDir, 'log');
     this.staticFilesDir = resolve(this.homeDir, 'ui');
diff --git a/src/core/server/index.test.ts b/src/core/server/index.test.ts
index d6bd19d36ce3c..7d604efcb98e8 100644
--- a/src/core/server/index.test.ts
+++ b/src/core/server/index.test.ts
@@ -22,6 +22,11 @@ jest.mock('./http/http_service', () => ({
   HttpService: jest.fn(() => mockHttpService),
 }));
 
+const mockPluginsService = { start: jest.fn(), stop: jest.fn() };
+jest.mock('./plugins/plugins_service', () => ({
+  PluginsService: jest.fn(() => mockPluginsService),
+}));
+
 const mockLegacyService = { start: jest.fn(), stop: jest.fn() };
 jest.mock('./legacy_compat/legacy_service', () => ({
   LegacyService: jest.fn(() => mockLegacyService),
@@ -45,6 +50,8 @@ afterEach(() => {
   mockConfigService.atPath.mockReset();
   mockHttpService.start.mockReset();
   mockHttpService.stop.mockReset();
+  mockPluginsService.start.mockReset();
+  mockPluginsService.stop.mockReset();
   mockLegacyService.start.mockReset();
   mockLegacyService.stop.mockReset();
 });
@@ -56,11 +63,13 @@ test('starts services on "start"', async () => {
   const server = new Server(mockConfigService as any, logger, env);
 
   expect(mockHttpService.start).not.toHaveBeenCalled();
+  expect(mockPluginsService.start).not.toHaveBeenCalled();
   expect(mockLegacyService.start).not.toHaveBeenCalled();
 
   await server.start();
 
   expect(mockHttpService.start).toHaveBeenCalledTimes(1);
+  expect(mockPluginsService.start).toHaveBeenCalledTimes(1);
   expect(mockLegacyService.start).toHaveBeenCalledTimes(1);
   expect(mockLegacyService.start).toHaveBeenCalledWith(mockHttpServiceStartContract);
 });
@@ -112,10 +121,12 @@ test('stops services on "stop"', async () => {
   await server.start();
 
   expect(mockHttpService.stop).not.toHaveBeenCalled();
+  expect(mockPluginsService.stop).not.toHaveBeenCalled();
   expect(mockLegacyService.stop).not.toHaveBeenCalled();
 
   await server.stop();
 
   expect(mockHttpService.stop).toHaveBeenCalledTimes(1);
+  expect(mockPluginsService.stop).toHaveBeenCalledTimes(1);
   expect(mockLegacyService.stop).toHaveBeenCalledTimes(1);
 });
diff --git a/src/core/server/index.ts b/src/core/server/index.ts
index ac645b2280041..0c9518f87e010 100644
--- a/src/core/server/index.ts
+++ b/src/core/server/index.ts
@@ -17,6 +17,8 @@
  * under the License.
  */
 
+import { PluginsModule } from './plugins';
+
 export { bootstrap } from './bootstrap';
 
 import { first } from 'rxjs/operators';
@@ -27,6 +29,7 @@ import { Logger, LoggerFactory } from './logging';
 
 export class Server {
   private readonly http: HttpModule;
+  private readonly plugins: PluginsModule;
   private readonly legacy: LegacyCompatModule;
   private readonly log: Logger;
 
@@ -38,6 +41,7 @@ export class Server {
     this.log = logger.get('server');
 
     this.http = new HttpModule(configService.atPath('server', HttpConfig), logger);
+    this.plugins = new PluginsModule(configService, logger, env);
     this.legacy = new LegacyCompatModule(configService, logger, env);
   }
 
@@ -54,6 +58,7 @@ export class Server {
       httpServerInfo = await this.http.service.start();
     }
 
+    await this.plugins.service.start();
     await this.legacy.service.start(httpServerInfo);
 
     const unhandledConfigPaths = await this.configService.getUnusedPaths();
@@ -70,6 +75,7 @@ export class Server {
     this.log.debug('stopping server');
 
     await this.legacy.service.stop();
+    await this.plugins.service.stop();
     await this.http.service.stop();
   }
 }
diff --git a/src/core/server/plugins/index.ts b/src/core/server/plugins/index.ts
new file mode 100644
index 0000000000000..6ed7870b86c6d
--- /dev/null
+++ b/src/core/server/plugins/index.ts
@@ -0,0 +1,30 @@
+/*
+ * 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 { ConfigService, Env } from '../config';
+import { LoggerFactory } from '../logging';
+import { PluginsService } from './plugins_service';
+
+export class PluginsModule {
+  public readonly service: PluginsService;
+
+  constructor(private readonly configService: ConfigService, logger: LoggerFactory, env: Env) {
+    this.service = new PluginsService(env, logger, this.configService);
+  }
+}
diff --git a/src/core/server/plugins/plugin_discovery.test.ts b/src/core/server/plugins/plugin_discovery.test.ts
new file mode 100644
index 0000000000000..b9b3af4832f7f
--- /dev/null
+++ b/src/core/server/plugins/plugin_discovery.test.ts
@@ -0,0 +1,139 @@
+/*
+ * 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.
+ */
+
+const mockReaddir = jest.fn();
+const mockReadFile = jest.fn();
+const mockStat = jest.fn();
+jest.mock('fs', () => ({
+  readdir: mockReaddir,
+  readFile: mockReadFile,
+  stat: mockStat,
+}));
+
+import { of } from 'rxjs';
+import { map, toArray } from 'rxjs/operators';
+import { logger } from '../logging/__mocks__';
+import { PluginsDiscovery } from './plugins_discovery';
+
+beforeEach(() => {
+  mockReaddir.mockImplementation((path, cb) => {
+    if (path === '/scan/non-empty/') {
+      cb(null, ['1', '2-no-manifest', '3', '4-incomplete-manifest']);
+    } else if (path === '/scan/non-empty-2/') {
+      cb(null, ['5-invalid-manifest', '6', '7-non-dir']);
+    } else if (path.includes('non-existent')) {
+      cb(new Error('ENOENT'));
+    } else {
+      cb(null, []);
+    }
+  });
+
+  mockStat.mockImplementation((path, cb) => {
+    if (path.includes('non-existent')) {
+      cb(new Error('ENOENT'));
+    } else {
+      cb(null, { isDirectory: () => !path.includes('non-dir') });
+    }
+  });
+
+  mockReadFile.mockImplementation((path, cb) => {
+    if (path.endsWith('no-manifest/kibana.json')) {
+      cb(new Error('ENOENT'));
+    } else if (path.endsWith('invalid-manifest/kibana.json')) {
+      cb(null, Buffer.from('not-json'));
+    } else if (path.endsWith('incomplete-manifest/kibana.json')) {
+      cb(null, Buffer.from(JSON.stringify({ version: '1' })));
+    } else {
+      cb(null, Buffer.from(JSON.stringify({ id: 'plugin', version: '1' })));
+    }
+  });
+});
+
+afterEach(() => {
+  jest.clearAllMocks();
+});
+
+test('properly scans folders and paths', async () => {
+  const discovery = new PluginsDiscovery(logger.get());
+  const { plugins$, errors$ } = discovery.discover(
+    of({
+      initialize: true,
+      scanDirs: ['/scan/non-empty/', '/scan/non-existent/', '/scan/empty/', '/scan/non-empty-2/'],
+      paths: ['/path/existent-dir', '/path/non-dir', '/path/non-existent', '/path/existent-dir-2'],
+    })
+  );
+
+  await expect(plugins$.pipe(toArray()).toPromise()).resolves.toMatchInlineSnapshot(`
+Array [
+  Object {
+    "manifest": Object {
+      "id": "plugin",
+      "version": "1",
+    },
+    "path": "/scan/non-empty/1",
+  },
+  Object {
+    "manifest": Object {
+      "id": "plugin",
+      "version": "1",
+    },
+    "path": "/scan/non-empty/3",
+  },
+  Object {
+    "manifest": Object {
+      "id": "plugin",
+      "version": "1",
+    },
+    "path": "/scan/non-empty-2/6",
+  },
+  Object {
+    "manifest": Object {
+      "id": "plugin",
+      "version": "1",
+    },
+    "path": "/path/existent-dir",
+  },
+  Object {
+    "manifest": Object {
+      "id": "plugin",
+      "version": "1",
+    },
+    "path": "/path/existent-dir-2",
+  },
+]
+`);
+
+  await expect(
+    errors$
+      .pipe(
+        map(error => error.toString()),
+        toArray()
+      )
+      .toPromise()
+  ).resolves.toMatchInlineSnapshot(`
+Array [
+  "Error: ENOENT (missing-manifest, /scan/non-empty/2-no-manifest/kibana.json)",
+  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, /scan/non-empty/4-incomplete-manifest/kibana.json)",
+  "Error: ENOENT (invalid-scan-dir, /scan/non-existent/)",
+  "Error: Unexpected token o in JSON at position 1 (invalid-manifest, /scan/non-empty-2/5-invalid-manifest/kibana.json)",
+  "Error: /path/non-dir is not a directory. (invalid-plugin-dir, /path/non-dir)",
+  "Error: ENOENT (invalid-plugin-dir, /path/non-existent)",
+]
+`);
+});
diff --git a/src/core/server/plugins/plugin_discovery_error.ts b/src/core/server/plugins/plugin_discovery_error.ts
new file mode 100644
index 0000000000000..924dec5d8d3d4
--- /dev/null
+++ b/src/core/server/plugins/plugin_discovery_error.ts
@@ -0,0 +1,43 @@
+/*
+ * 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.
+ */
+
+export enum PluginDiscoveryErrorType {
+  InvalidScanDirectory = 'invalid-scan-dir',
+  InvalidPluginDirectory = 'invalid-plugin-dir',
+  InvalidManifest = 'invalid-manifest',
+  MissingManifest = 'missing-manifest',
+}
+
+export class PluginDiscoveryError extends Error {
+  constructor(
+    public readonly type: PluginDiscoveryErrorType,
+    public readonly path: string,
+    public readonly cause?: Error
+  ) {
+    super(cause && cause.message);
+
+    // Set the prototype explicitly, see:
+    // https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
+    Object.setPrototypeOf(this, PluginDiscoveryError.prototype);
+  }
+
+  public toString() {
+    return `${super.toString()} (${this.type}, ${this.path})`;
+  }
+}
diff --git a/src/core/server/plugins/plugins_config.ts b/src/core/server/plugins/plugins_config.ts
new file mode 100644
index 0000000000000..ad86581653310
--- /dev/null
+++ b/src/core/server/plugins/plugins_config.ts
@@ -0,0 +1,58 @@
+/*
+ * 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 { schema, TypeOf } from '@kbn/config-schema';
+
+const pluginsSchema = schema.object({
+  initialize: schema.boolean({ defaultValue: true }),
+  scanDirs: schema.arrayOf(schema.string(), {
+    defaultValue: [],
+  }),
+  paths: schema.arrayOf(schema.string(), {
+    defaultValue: [],
+  }),
+});
+
+type PluginsConfigType = TypeOf<typeof pluginsSchema>;
+
+/** @internal */
+export class PluginsConfig {
+  public static schema = pluginsSchema;
+
+  /**
+   * Indicates whether or not plugins should be initialized.
+   */
+  public readonly initialize: boolean;
+
+  /**
+   * Defines directories that we should scan for the plugin subdirectories.
+   */
+  public readonly scanDirs: string[];
+
+  /**
+   * Defines direct paths to specific plugin directories that we should initialize.
+   */
+  public readonly paths: string[];
+
+  constructor(config: PluginsConfigType) {
+    this.initialize = config.initialize;
+    this.scanDirs = config.scanDirs;
+    this.paths = config.paths;
+  }
+}
diff --git a/src/core/server/plugins/plugins_discovery.ts b/src/core/server/plugins/plugins_discovery.ts
new file mode 100644
index 0000000000000..5e1e2e60908e8
--- /dev/null
+++ b/src/core/server/plugins/plugins_discovery.ts
@@ -0,0 +1,230 @@
+/*
+ * 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 { readdir, readFile, stat } from 'fs';
+import { resolve } from 'path';
+import { bindNodeCallback, from, merge, Observable, throwError } from 'rxjs';
+import { catchError, mergeMap, shareReplay } from 'rxjs/operators';
+import { Logger } from '../logging';
+import { PluginDiscoveryError, PluginDiscoveryErrorType } from './plugin_discovery_error';
+import { PluginsConfig } from './plugins_config';
+
+const fsReadDir$ = bindNodeCallback(readdir);
+const fsStat$ = bindNodeCallback(stat);
+const fsReadFile$ = bindNodeCallback(readFile);
+
+/**
+ * Describes the set of required and optional properties plugin can define in its
+ * mandatory JSON manifest file.
+ */
+interface PluginManifest {
+  /**
+   * Identifier of the plugin.
+   */
+  id: string;
+
+  /**
+   * Version of the plugin.
+   */
+  version: string;
+
+  /**
+   * The version of Kibana the plugin is compatible with, defaults to "version".
+   */
+  kibanaVersion?: string;
+
+  /**
+   * An optional list of the other plugins that **must be** installed and enabled
+   * for this plugin to function properly.
+   */
+  requiredPlugins?: string[];
+
+  /**
+   * An optional list of the other plugins that if installed and enabled **may be**
+   * leveraged by this plugin for some additional functionality but otherwise are
+   * not required for this plugin to work properly.
+   */
+  optionalPlugins?: string[];
+
+  /**
+   * Specifies whether plugin includes some client/browser specific functionality
+   * that should be included into client bundle via `public/ui_plugin.js` file.
+   */
+  ui?: boolean;
+}
+
+interface DiscoveryResult {
+  plugin?: { path: string; manifest: PluginManifest };
+  error?: PluginDiscoveryError;
+}
+
+/**
+ * Name of the JSON manifest file that should be located in the plugin directory.
+ */
+const MANIFEST_FILE_NAME = 'kibana.json';
+
+/**
+ * This class is responsible for discovering all recognizable plugins based on the
+ * provided plugin config that defines top-level directories for groups of plugins
+ * or/and direct paths to specific plugins.
+ */
+export class PluginsDiscovery {
+  constructor(private readonly log: Logger) {}
+
+  /**
+   * Tries to discover all possible plugins based on the provided plugin config.
+   * Discovery result consists of two separate streams, the one (`plugins$`) is
+   * for the successfully discovered plugins and the other one (`errors$`) is for
+   * all the errors that occurred during discovery process.
+   * @param config$ Plugin config instance.
+   */
+  public discover(config$: Observable<PluginsConfig>) {
+    this.log.debug('Discovering plugins...');
+
+    const discoveryResults$ = config$.pipe(
+      mergeMap(config =>
+        merge(this.processScanDirs$(config.scanDirs), this.processPaths$(config.paths))
+      ),
+      mergeMap(pluginPathOrError => {
+        return typeof pluginPathOrError === 'string'
+          ? this.createPlugin$(pluginPathOrError)
+          : [pluginPathOrError];
+      }),
+      shareReplay()
+    );
+
+    return {
+      plugins$: discoveryResults$.pipe(
+        mergeMap(entry => (entry.plugin !== undefined ? [entry.plugin] : []))
+      ),
+      errors$: discoveryResults$.pipe(
+        mergeMap(entry => (entry.error !== undefined ? [entry.error] : []))
+      ),
+    };
+  }
+
+  /**
+   * Iterates over every entry in `scanDirs` and returns a merged stream of all
+   * sub-directories. If directory cannot be read or it's impossible to get stat
+   * for any of the nested entries then error is added into the stream instead.
+   * @param scanDirs List of the top-level directories to process.
+   */
+  private processScanDirs$(scanDirs: string[]) {
+    return from(scanDirs).pipe(
+      mergeMap(dir => {
+        this.log.debug(`Scanning "${dir}" for plugin sub-directories...`);
+
+        return fsReadDir$(dir).pipe(
+          mergeMap(subDirs => subDirs.map(subDir => resolve(dir, subDir))),
+          mergeMap(path =>
+            fsStat$(path).pipe(
+              // Filter out non-directory entries from target directories, it's expected that
+              // these directories may contain files (e.g. `README.md` or `package.json`).
+              // We shouldn't silently ignore the entries we couldn't get stat for though.
+              mergeMap(pathStat => (pathStat.isDirectory() ? [path] : [])),
+              catchError(err => [
+                wrapError(PluginDiscoveryErrorType.InvalidPluginDirectory, path, err),
+              ])
+            )
+          ),
+          catchError(err => [wrapError(PluginDiscoveryErrorType.InvalidScanDirectory, dir, err)])
+        );
+      })
+    );
+  }
+
+  /**
+   * Iterates over every entry in `paths` and returns a stream of all paths that
+   * are directories. If path is not a directory or it's impossible to get stat
+   * for this path then error is added into the stream instead.
+   * @param paths List of paths to process.
+   */
+  private processPaths$(paths: string[]) {
+    return from(paths).pipe(
+      mergeMap(path => {
+        this.log.debug(`Including "${path}" into the plugin path list.`);
+
+        return fsStat$(path).pipe(
+          // Since every path is specifically provided we should treat non-directory
+          // entries as mistakes we should report of.
+          mergeMap(pathStat => {
+            return pathStat.isDirectory()
+              ? [path]
+              : throwError(new Error(`${path} is not a directory.`));
+          }),
+          catchError(err => [wrapError(PluginDiscoveryErrorType.InvalidPluginDirectory, path, err)])
+        );
+      })
+    );
+  }
+
+  /**
+   * Tries to load and parse the plugin manifest file located at the provided
+   * plugin directory path and throws if it fails to do so or plugin manifest
+   * isn't valid.
+   * @param path Path to the plugin directory where manifest should be loaded from.
+   */
+  private createPlugin$(path: string) {
+    const manifestPath = resolve(path, MANIFEST_FILE_NAME);
+    return fsReadFile$(manifestPath).pipe(
+      mergeMap(manifestContent => {
+        try {
+          const plugin = { path, manifest: parseManifest(manifestContent) };
+
+          this.log.debug(`Successfully discovered plugin "${plugin.manifest.id}" at "${path}"`);
+
+          return [{ plugin }];
+        } catch (err) {
+          return [wrapError(PluginDiscoveryErrorType.InvalidManifest, manifestPath, err)];
+        }
+      }),
+      catchError(err => [wrapError(PluginDiscoveryErrorType.MissingManifest, manifestPath, err)])
+    );
+  }
+}
+
+/**
+ * Parses raw buffer content into plugin manifest with the preliminary checks.
+ * @param rawManifest Buffer containing plugin manifest JSON.
+ */
+function parseManifest(rawManifest: Buffer): PluginManifest {
+  const manifest = JSON.parse(rawManifest.toString());
+  if (
+    manifest.id &&
+    manifest.version &&
+    typeof manifest.id === 'string' &&
+    typeof manifest.version === 'string'
+  ) {
+    return manifest;
+  }
+
+  throw new Error('The "id" or/and "version" is missing in the plugin manifest.');
+}
+
+/**
+ * Wraps any error instance into `PluginDiscoveryError` tagging it with specific
+ * type that can be used later to distinguish between different types of errors
+ * that can occur during plugin discovery process.
+ * @param type Type of the discovery error (invalid directory, invalid manifest etc.)
+ * @param path Path at which discovery error occurred.
+ * @param error Instance of the "raw" error that occurred during discovery.
+ */
+function wrapError(type: PluginDiscoveryErrorType, path: string, error: Error): DiscoveryResult {
+  return { error: new PluginDiscoveryError(type, path, error) };
+}
diff --git a/src/core/server/plugins/plugins_service.ts b/src/core/server/plugins/plugins_service.ts
new file mode 100644
index 0000000000000..1b70c9c8d36d3
--- /dev/null
+++ b/src/core/server/plugins/plugins_service.ts
@@ -0,0 +1,68 @@
+/*
+ * 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 { filter, first, tap, toArray } from 'rxjs/operators';
+import { CoreService } from '../../types/core_service';
+import { ConfigService, Env } from '../config';
+import { Logger, LoggerFactory } from '../logging';
+import { PluginDiscoveryErrorType } from './plugin_discovery_error';
+import { PluginsConfig } from './plugins_config';
+import { PluginsDiscovery } from './plugins_discovery';
+
+export class PluginsService implements CoreService {
+  private readonly log: Logger;
+  private readonly discovery: PluginsDiscovery;
+
+  constructor(
+    readonly env: Env,
+    readonly logger: LoggerFactory,
+    private readonly configService: ConfigService
+  ) {
+    this.log = logger.get('plugins', 'service');
+    this.discovery = new PluginsDiscovery(logger.get('plugins', 'discovery'));
+  }
+
+  public async start() {
+    this.log.debug('starting plugins service');
+
+    const { plugins$, errors$ } = this.discovery.discover(
+      this.configService.atPath('plugins', PluginsConfig).pipe(first())
+    );
+
+    await plugins$
+      .pipe(
+        toArray(),
+        tap(plugins => this.log.debug(`Discovered ${plugins.length} plugins.`))
+      )
+      .toPromise();
+
+    await errors$
+      .pipe(
+        filter(error => error.type === PluginDiscoveryErrorType.InvalidManifest),
+        tap(invalidManifestError => {
+          this.log.error(invalidManifestError);
+        })
+      )
+      .toPromise();
+  }
+
+  public async stop() {
+    this.log.debug('stopping plugins service');
+  }
+}

From ae8d61bbc82462a505c53e8face6840957cc913a Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Mon, 22 Oct 2018 11:34:22 +0200
Subject: [PATCH 02/15] Review#1: Make `discover` to accept single
 `PluginsConfig` instance instead of config stream, set `PluginManifest`
 defaults, add more tests for the plugin manifest parsing and get rid of `env`
 argument for now.

---
 src/core/server/index.ts                      |   2 +-
 src/core/server/plugins/index.ts              |   6 +-
 .../server/plugins/plugin_discovery.test.ts   | 186 +++++++++++++++++-
 src/core/server/plugins/plugins_discovery.ts  |  37 ++--
 src/core/server/plugins/plugins_service.ts    |  24 ++-
 5 files changed, 215 insertions(+), 40 deletions(-)

diff --git a/src/core/server/index.ts b/src/core/server/index.ts
index 0c9518f87e010..f1d7d29dd9f6c 100644
--- a/src/core/server/index.ts
+++ b/src/core/server/index.ts
@@ -41,7 +41,7 @@ export class Server {
     this.log = logger.get('server');
 
     this.http = new HttpModule(configService.atPath('server', HttpConfig), logger);
-    this.plugins = new PluginsModule(configService, logger, env);
+    this.plugins = new PluginsModule(configService, logger);
     this.legacy = new LegacyCompatModule(configService, logger, env);
   }
 
diff --git a/src/core/server/plugins/index.ts b/src/core/server/plugins/index.ts
index 6ed7870b86c6d..01c2721e6172e 100644
--- a/src/core/server/plugins/index.ts
+++ b/src/core/server/plugins/index.ts
@@ -17,14 +17,14 @@
  * under the License.
  */
 
-import { ConfigService, Env } from '../config';
+import { ConfigService } from '../config';
 import { LoggerFactory } from '../logging';
 import { PluginsService } from './plugins_service';
 
 export class PluginsModule {
   public readonly service: PluginsService;
 
-  constructor(private readonly configService: ConfigService, logger: LoggerFactory, env: Env) {
-    this.service = new PluginsService(env, logger, this.configService);
+  constructor(private readonly configService: ConfigService, logger: LoggerFactory) {
+    this.service = new PluginsService(logger, this.configService);
   }
 }
diff --git a/src/core/server/plugins/plugin_discovery.test.ts b/src/core/server/plugins/plugin_discovery.test.ts
index b9b3af4832f7f..a29ec3e0ceeff 100644
--- a/src/core/server/plugins/plugin_discovery.test.ts
+++ b/src/core/server/plugins/plugin_discovery.test.ts
@@ -26,11 +26,12 @@ jest.mock('fs', () => ({
   stat: mockStat,
 }));
 
-import { of } from 'rxjs';
+import { Observable } from 'rxjs';
 import { map, toArray } from 'rxjs/operators';
 import { logger } from '../logging/__mocks__';
-import { PluginsDiscovery } from './plugins_discovery';
+import { PluginManifest, PluginsDiscovery } from './plugins_discovery';
 
+let discovery: PluginsDiscovery;
 beforeEach(() => {
   mockReaddir.mockImplementation((path, cb) => {
     if (path === '/scan/non-empty/') {
@@ -63,6 +64,8 @@ beforeEach(() => {
       cb(null, Buffer.from(JSON.stringify({ id: 'plugin', version: '1' })));
     }
   });
+
+  discovery = new PluginsDiscovery(logger.get());
 });
 
 afterEach(() => {
@@ -70,20 +73,21 @@ afterEach(() => {
 });
 
 test('properly scans folders and paths', async () => {
-  const discovery = new PluginsDiscovery(logger.get());
-  const { plugins$, errors$ } = discovery.discover(
-    of({
-      initialize: true,
-      scanDirs: ['/scan/non-empty/', '/scan/non-existent/', '/scan/empty/', '/scan/non-empty-2/'],
-      paths: ['/path/existent-dir', '/path/non-dir', '/path/non-existent', '/path/existent-dir-2'],
-    })
-  );
+  const { plugins$, errors$ } = discovery.discover({
+    initialize: true,
+    scanDirs: ['/scan/non-empty/', '/scan/non-existent/', '/scan/empty/', '/scan/non-empty-2/'],
+    paths: ['/path/existent-dir', '/path/non-dir', '/path/non-existent', '/path/existent-dir-2'],
+  });
 
   await expect(plugins$.pipe(toArray()).toPromise()).resolves.toMatchInlineSnapshot(`
 Array [
   Object {
     "manifest": Object {
       "id": "plugin",
+      "kibanaVersion": "1",
+      "optionalPlugins": Array [],
+      "requiredPlugins": Array [],
+      "ui": false,
       "version": "1",
     },
     "path": "/scan/non-empty/1",
@@ -91,6 +95,10 @@ Array [
   Object {
     "manifest": Object {
       "id": "plugin",
+      "kibanaVersion": "1",
+      "optionalPlugins": Array [],
+      "requiredPlugins": Array [],
+      "ui": false,
       "version": "1",
     },
     "path": "/scan/non-empty/3",
@@ -98,6 +106,10 @@ Array [
   Object {
     "manifest": Object {
       "id": "plugin",
+      "kibanaVersion": "1",
+      "optionalPlugins": Array [],
+      "requiredPlugins": Array [],
+      "ui": false,
       "version": "1",
     },
     "path": "/scan/non-empty-2/6",
@@ -105,6 +117,10 @@ Array [
   Object {
     "manifest": Object {
       "id": "plugin",
+      "kibanaVersion": "1",
+      "optionalPlugins": Array [],
+      "requiredPlugins": Array [],
+      "ui": false,
       "version": "1",
     },
     "path": "/path/existent-dir",
@@ -112,6 +128,10 @@ Array [
   Object {
     "manifest": Object {
       "id": "plugin",
+      "kibanaVersion": "1",
+      "optionalPlugins": Array [],
+      "requiredPlugins": Array [],
+      "ui": false,
       "version": "1",
     },
     "path": "/path/existent-dir-2",
@@ -137,3 +157,149 @@ Array [
 ]
 `);
 });
+
+describe('parsing plugin manifest', () => {
+  let plugins$: Observable<Array<{ path: string; manifest: PluginManifest }>>;
+  let errors$: Observable<string[]>;
+  beforeEach(async () => {
+    const discoveryResult = discovery.discover({
+      initialize: true,
+      scanDirs: [],
+      paths: ['/path/existent-dir'],
+    });
+
+    plugins$ = discoveryResult.plugins$.pipe(toArray());
+    errors$ = discoveryResult.errors$.pipe(
+      map(error => error.toString()),
+      toArray()
+    );
+  });
+
+  test('return error when manifest is empty', async () => {
+    mockReadFile.mockImplementation((path, cb) => {
+      cb(null, Buffer.from(''));
+    });
+
+    await expect(plugins$.toPromise()).resolves.toEqual([]);
+    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
+Array [
+  "Error: Unexpected end of JSON input (invalid-manifest, /path/existent-dir/kibana.json)",
+]
+`);
+  });
+
+  test('return error when manifest content is null', async () => {
+    mockReadFile.mockImplementation((path, cb) => {
+      cb(null, Buffer.from('null'));
+    });
+
+    await expect(plugins$.toPromise()).resolves.toEqual([]);
+    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
+Array [
+  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, /path/existent-dir/kibana.json)",
+]
+`);
+  });
+
+  test('return error when manifest content is not a valid JSON', async () => {
+    mockReadFile.mockImplementation((path, cb) => {
+      cb(null, Buffer.from('not-json'));
+    });
+
+    await expect(plugins$.toPromise()).resolves.toEqual([]);
+    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
+Array [
+  "Error: Unexpected token o in JSON at position 1 (invalid-manifest, /path/existent-dir/kibana.json)",
+]
+`);
+  });
+
+  test('return error when plugin id is missing', async () => {
+    mockReadFile.mockImplementation((path, cb) => {
+      cb(null, Buffer.from(JSON.stringify({ version: 'some-version' })));
+    });
+
+    await expect(plugins$.toPromise()).resolves.toEqual([]);
+    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
+Array [
+  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, /path/existent-dir/kibana.json)",
+]
+`);
+  });
+
+  test('return error when plugin version is missing', async () => {
+    mockReadFile.mockImplementation((path, cb) => {
+      cb(null, Buffer.from(JSON.stringify({ id: 'some-id' })));
+    });
+
+    await expect(plugins$.toPromise()).resolves.toEqual([]);
+    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
+Array [
+  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, /path/existent-dir/kibana.json)",
+]
+`);
+  });
+
+  test('set defaults for all missing optional fields', async () => {
+    mockReadFile.mockImplementation((path, cb) => {
+      cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: 'some-version' })));
+    });
+
+    await expect(plugins$.toPromise()).resolves.toMatchInlineSnapshot(`
+Array [
+  Object {
+    "manifest": Object {
+      "id": "some-id",
+      "kibanaVersion": "some-version",
+      "optionalPlugins": Array [],
+      "requiredPlugins": Array [],
+      "ui": false,
+      "version": "some-version",
+    },
+    "path": "/path/existent-dir",
+  },
+]
+`);
+    await expect(errors$.toPromise()).resolves.toEqual([]);
+  });
+
+  test('return all set optional fields as they are in manifest', async () => {
+    mockReadFile.mockImplementation((path, cb) => {
+      cb(
+        null,
+        Buffer.from(
+          JSON.stringify({
+            id: 'some-id',
+            version: 'some-version',
+            kibanaVersion: 'some-kibana-version',
+            requiredPlugins: ['some-required-plugin', 'some-required-plugin-2'],
+            optionalPlugins: ['some-optional-plugin'],
+            ui: true,
+          })
+        )
+      );
+    });
+
+    await expect(plugins$.toPromise()).resolves.toMatchInlineSnapshot(`
+Array [
+  Object {
+    "manifest": Object {
+      "id": "some-id",
+      "kibanaVersion": "some-kibana-version",
+      "optionalPlugins": Array [
+        "some-optional-plugin",
+      ],
+      "requiredPlugins": Array [
+        "some-required-plugin",
+        "some-required-plugin-2",
+      ],
+      "ui": true,
+      "version": "some-version",
+    },
+    "path": "/path/existent-dir",
+  },
+]
+`);
+    await expect(errors$.toPromise()).resolves.toEqual([]);
+  });
+});
diff --git a/src/core/server/plugins/plugins_discovery.ts b/src/core/server/plugins/plugins_discovery.ts
index 5e1e2e60908e8..cdaed5b0bea67 100644
--- a/src/core/server/plugins/plugins_discovery.ts
+++ b/src/core/server/plugins/plugins_discovery.ts
@@ -19,7 +19,7 @@
 
 import { readdir, readFile, stat } from 'fs';
 import { resolve } from 'path';
-import { bindNodeCallback, from, merge, Observable, throwError } from 'rxjs';
+import { bindNodeCallback, from, merge, throwError } from 'rxjs';
 import { catchError, mergeMap, shareReplay } from 'rxjs/operators';
 import { Logger } from '../logging';
 import { PluginDiscoveryError, PluginDiscoveryErrorType } from './plugin_discovery_error';
@@ -33,7 +33,7 @@ const fsReadFile$ = bindNodeCallback(readFile);
  * Describes the set of required and optional properties plugin can define in its
  * mandatory JSON manifest file.
  */
-interface PluginManifest {
+export interface PluginManifest {
   /**
    * Identifier of the plugin.
    */
@@ -53,20 +53,20 @@ interface PluginManifest {
    * An optional list of the other plugins that **must be** installed and enabled
    * for this plugin to function properly.
    */
-  requiredPlugins?: string[];
+  requiredPlugins: string[];
 
   /**
    * An optional list of the other plugins that if installed and enabled **may be**
    * leveraged by this plugin for some additional functionality but otherwise are
    * not required for this plugin to work properly.
    */
-  optionalPlugins?: string[];
+  optionalPlugins: string[];
 
   /**
    * Specifies whether plugin includes some client/browser specific functionality
    * that should be included into client bundle via `public/ui_plugin.js` file.
    */
-  ui?: boolean;
+  ui: boolean;
 }
 
 interface DiscoveryResult {
@@ -92,15 +92,15 @@ export class PluginsDiscovery {
    * Discovery result consists of two separate streams, the one (`plugins$`) is
    * for the successfully discovered plugins and the other one (`errors$`) is for
    * all the errors that occurred during discovery process.
-   * @param config$ Plugin config instance.
+   * @param config Plugin config instance.
    */
-  public discover(config$: Observable<PluginsConfig>) {
+  public discover(config: PluginsConfig) {
     this.log.debug('Discovering plugins...');
 
-    const discoveryResults$ = config$.pipe(
-      mergeMap(config =>
-        merge(this.processScanDirs$(config.scanDirs), this.processPaths$(config.paths))
-      ),
+    const discoveryResults$ = merge(
+      this.processScanDirs$(config.scanDirs),
+      this.processPaths$(config.paths)
+    ).pipe(
       mergeMap(pluginPathOrError => {
         return typeof pluginPathOrError === 'string'
           ? this.createPlugin$(pluginPathOrError)
@@ -204,14 +204,25 @@ export class PluginsDiscovery {
  * @param rawManifest Buffer containing plugin manifest JSON.
  */
 function parseManifest(rawManifest: Buffer): PluginManifest {
-  const manifest = JSON.parse(rawManifest.toString());
+  const manifest: Partial<PluginManifest> = JSON.parse(rawManifest.toString());
   if (
+    manifest &&
     manifest.id &&
     manifest.version &&
     typeof manifest.id === 'string' &&
     typeof manifest.version === 'string'
   ) {
-    return manifest;
+    return {
+      id: manifest.id,
+      version: manifest.version,
+      kibanaVersion:
+        typeof manifest.kibanaVersion === 'string' && manifest.kibanaVersion
+          ? manifest.kibanaVersion
+          : manifest.version,
+      requiredPlugins: Array.isArray(manifest.requiredPlugins) ? manifest.requiredPlugins : [],
+      optionalPlugins: Array.isArray(manifest.optionalPlugins) ? manifest.optionalPlugins : [],
+      ui: typeof manifest.ui === 'boolean' ? manifest.ui : false,
+    };
   }
 
   throw new Error('The "id" or/and "version" is missing in the plugin manifest.');
diff --git a/src/core/server/plugins/plugins_service.ts b/src/core/server/plugins/plugins_service.ts
index 1b70c9c8d36d3..612122cc71725 100644
--- a/src/core/server/plugins/plugins_service.ts
+++ b/src/core/server/plugins/plugins_service.ts
@@ -17,9 +17,9 @@
  * under the License.
  */
 
-import { filter, first, tap, toArray } from 'rxjs/operators';
+import { filter, first, map, tap, toArray } from 'rxjs/operators';
 import { CoreService } from '../../types/core_service';
-import { ConfigService, Env } from '../config';
+import { ConfigService } from '../config';
 import { Logger, LoggerFactory } from '../logging';
 import { PluginDiscoveryErrorType } from './plugin_discovery_error';
 import { PluginsConfig } from './plugins_config';
@@ -29,11 +29,7 @@ export class PluginsService implements CoreService {
   private readonly log: Logger;
   private readonly discovery: PluginsDiscovery;
 
-  constructor(
-    readonly env: Env,
-    readonly logger: LoggerFactory,
-    private readonly configService: ConfigService
-  ) {
+  constructor(logger: LoggerFactory, private readonly configService: ConfigService) {
     this.log = logger.get('plugins', 'service');
     this.discovery = new PluginsDiscovery(logger.get('plugins', 'discovery'));
   }
@@ -41,9 +37,13 @@ export class PluginsService implements CoreService {
   public async start() {
     this.log.debug('starting plugins service');
 
-    const { plugins$, errors$ } = this.discovery.discover(
-      this.configService.atPath('plugins', PluginsConfig).pipe(first())
-    );
+    const { errors$, plugins$ } = await this.configService
+      .atPath('plugins', PluginsConfig)
+      .pipe(
+        first(),
+        map(config => this.discovery.discover(config))
+      )
+      .toPromise();
 
     await plugins$
       .pipe(
@@ -55,9 +55,7 @@ export class PluginsService implements CoreService {
     await errors$
       .pipe(
         filter(error => error.type === PluginDiscoveryErrorType.InvalidManifest),
-        tap(invalidManifestError => {
-          this.log.error(invalidManifestError);
-        })
+        tap(invalidManifestError => this.log.error(invalidManifestError))
       )
       .toPromise();
   }

From 1a047e5b3040888a90b3bf15cd7dd116a3863a7b Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Fri, 26 Oct 2018 12:39:09 +0200
Subject: [PATCH 03/15] Make plugin manifest interface "immutable".

---
 src/core/server/plugins/plugins_discovery.ts | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/core/server/plugins/plugins_discovery.ts b/src/core/server/plugins/plugins_discovery.ts
index cdaed5b0bea67..c24acf0c78ab2 100644
--- a/src/core/server/plugins/plugins_discovery.ts
+++ b/src/core/server/plugins/plugins_discovery.ts
@@ -37,36 +37,36 @@ export interface PluginManifest {
   /**
    * Identifier of the plugin.
    */
-  id: string;
+  readonly id: string;
 
   /**
    * Version of the plugin.
    */
-  version: string;
+  readonly version: string;
 
   /**
    * The version of Kibana the plugin is compatible with, defaults to "version".
    */
-  kibanaVersion?: string;
+  readonly kibanaVersion: string;
 
   /**
    * An optional list of the other plugins that **must be** installed and enabled
    * for this plugin to function properly.
    */
-  requiredPlugins: string[];
+  readonly requiredPlugins: ReadonlyArray<string>;
 
   /**
    * An optional list of the other plugins that if installed and enabled **may be**
    * leveraged by this plugin for some additional functionality but otherwise are
    * not required for this plugin to work properly.
    */
-  optionalPlugins: string[];
+  readonly optionalPlugins: ReadonlyArray<string>;
 
   /**
    * Specifies whether plugin includes some client/browser specific functionality
    * that should be included into client bundle via `public/ui_plugin.js` file.
    */
-  ui: boolean;
+  readonly ui: boolean;
 }
 
 interface DiscoveryResult {

From 8dfe3bef0b3c8c7cae70370b7d0c2d4c1bdde948 Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Fri, 2 Nov 2018 14:27:19 +0100
Subject: [PATCH 04/15] =?UTF-8?q?Review=20=E2=84=962:=20make=20tests=20pla?=
 =?UTF-8?q?tform=20agnostic.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../server/plugins/plugin_discovery.test.ts   | 107 +++++++++++++-----
 1 file changed, 80 insertions(+), 27 deletions(-)

diff --git a/src/core/server/plugins/plugin_discovery.test.ts b/src/core/server/plugins/plugin_discovery.test.ts
index a29ec3e0ceeff..dc446988c5155 100644
--- a/src/core/server/plugins/plugin_discovery.test.ts
+++ b/src/core/server/plugins/plugin_discovery.test.ts
@@ -26,19 +26,43 @@ jest.mock('fs', () => ({
   stat: mockStat,
 }));
 
+import { resolve } from 'path';
 import { Observable } from 'rxjs';
 import { map, toArray } from 'rxjs/operators';
 import { logger } from '../logging/__mocks__';
 import { PluginManifest, PluginsDiscovery } from './plugins_discovery';
 
+/**
+ * Resolves absolute path and escapes backslashes (used on windows systems).
+ * @param pathSegments Test path segments.
+ */
+function resolveForSnapshot(...pathSegments: string[]) {
+  return resolve(...pathSegments).replace(/\\/g, '\\\\');
+}
+
+const TEST_PATHS = {
+  scanDirs: {
+    nonEmpty: resolve('scan', 'non-empty'),
+    nonEmpty2: resolve('scan', 'non-empty-2'),
+    nonExistent: resolve('scan', 'non-existent'),
+    empty: resolve('scan', 'empty'),
+  },
+  paths: {
+    existentDir: resolve('path', 'existent-dir'),
+    existentDir2: resolve('path', 'existent-dir-2'),
+    nonDir: resolve('path', 'non-dir'),
+    nonExistent: resolve('path', 'non-existent'),
+  },
+};
+
 let discovery: PluginsDiscovery;
 beforeEach(() => {
   mockReaddir.mockImplementation((path, cb) => {
-    if (path === '/scan/non-empty/') {
+    if (path === TEST_PATHS.scanDirs.nonEmpty) {
       cb(null, ['1', '2-no-manifest', '3', '4-incomplete-manifest']);
-    } else if (path === '/scan/non-empty-2/') {
+    } else if (path === TEST_PATHS.scanDirs.nonEmpty2) {
       cb(null, ['5-invalid-manifest', '6', '7-non-dir']);
-    } else if (path.includes('non-existent')) {
+    } else if (path === TEST_PATHS.scanDirs.nonExistent) {
       cb(new Error('ENOENT'));
     } else {
       cb(null, []);
@@ -54,11 +78,11 @@ beforeEach(() => {
   });
 
   mockReadFile.mockImplementation((path, cb) => {
-    if (path.endsWith('no-manifest/kibana.json')) {
+    if (path.includes('no-manifest')) {
       cb(new Error('ENOENT'));
-    } else if (path.endsWith('invalid-manifest/kibana.json')) {
+    } else if (path.includes('invalid-manifest')) {
       cb(null, Buffer.from('not-json'));
-    } else if (path.endsWith('incomplete-manifest/kibana.json')) {
+    } else if (path.includes('incomplete-manifest')) {
       cb(null, Buffer.from(JSON.stringify({ version: '1' })));
     } else {
       cb(null, Buffer.from(JSON.stringify({ id: 'plugin', version: '1' })));
@@ -75,8 +99,8 @@ afterEach(() => {
 test('properly scans folders and paths', async () => {
   const { plugins$, errors$ } = discovery.discover({
     initialize: true,
-    scanDirs: ['/scan/non-empty/', '/scan/non-existent/', '/scan/empty/', '/scan/non-empty-2/'],
-    paths: ['/path/existent-dir', '/path/non-dir', '/path/non-existent', '/path/existent-dir-2'],
+    scanDirs: Object.values(TEST_PATHS.scanDirs),
+    paths: Object.values(TEST_PATHS.paths),
   });
 
   await expect(plugins$.pipe(toArray()).toPromise()).resolves.toMatchInlineSnapshot(`
@@ -90,7 +114,7 @@ Array [
       "ui": false,
       "version": "1",
     },
-    "path": "/scan/non-empty/1",
+    "path": "${resolveForSnapshot(TEST_PATHS.scanDirs.nonEmpty, '1')}",
   },
   Object {
     "manifest": Object {
@@ -101,7 +125,7 @@ Array [
       "ui": false,
       "version": "1",
     },
-    "path": "/scan/non-empty/3",
+    "path": "${resolveForSnapshot(TEST_PATHS.scanDirs.nonEmpty, '3')}",
   },
   Object {
     "manifest": Object {
@@ -112,7 +136,7 @@ Array [
       "ui": false,
       "version": "1",
     },
-    "path": "/scan/non-empty-2/6",
+    "path": "${resolveForSnapshot(TEST_PATHS.scanDirs.nonEmpty2, '6')}",
   },
   Object {
     "manifest": Object {
@@ -123,7 +147,7 @@ Array [
       "ui": false,
       "version": "1",
     },
-    "path": "/path/existent-dir",
+    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir)}",
   },
   Object {
     "manifest": Object {
@@ -134,7 +158,7 @@ Array [
       "ui": false,
       "version": "1",
     },
-    "path": "/path/existent-dir-2",
+    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir2)}",
   },
 ]
 `);
@@ -148,12 +172,26 @@ Array [
       .toPromise()
   ).resolves.toMatchInlineSnapshot(`
 Array [
-  "Error: ENOENT (missing-manifest, /scan/non-empty/2-no-manifest/kibana.json)",
-  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, /scan/non-empty/4-incomplete-manifest/kibana.json)",
-  "Error: ENOENT (invalid-scan-dir, /scan/non-existent/)",
-  "Error: Unexpected token o in JSON at position 1 (invalid-manifest, /scan/non-empty-2/5-invalid-manifest/kibana.json)",
-  "Error: /path/non-dir is not a directory. (invalid-plugin-dir, /path/non-dir)",
-  "Error: ENOENT (invalid-plugin-dir, /path/non-existent)",
+  "Error: ENOENT (missing-manifest, ${resolveForSnapshot(
+    TEST_PATHS.scanDirs.nonEmpty,
+    '2-no-manifest',
+    'kibana.json'
+  )})",
+  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, ${resolveForSnapshot(
+    TEST_PATHS.scanDirs.nonEmpty,
+    '4-incomplete-manifest',
+    'kibana.json'
+  )})",
+  "Error: Unexpected token o in JSON at position 1 (invalid-manifest, ${resolveForSnapshot(
+    TEST_PATHS.scanDirs.nonEmpty2,
+    '5-invalid-manifest',
+    'kibana.json'
+  )})",
+  "Error: ENOENT (invalid-scan-dir, ${resolveForSnapshot(TEST_PATHS.scanDirs.nonExistent)})",
+  "Error: ${resolveForSnapshot(
+    TEST_PATHS.paths.nonDir
+  )} is not a directory. (invalid-plugin-dir, ${resolveForSnapshot(TEST_PATHS.paths.nonDir)})",
+  "Error: ENOENT (invalid-plugin-dir, ${resolveForSnapshot(TEST_PATHS.paths.nonExistent)})",
 ]
 `);
 });
@@ -165,7 +203,7 @@ describe('parsing plugin manifest', () => {
     const discoveryResult = discovery.discover({
       initialize: true,
       scanDirs: [],
-      paths: ['/path/existent-dir'],
+      paths: [TEST_PATHS.paths.existentDir],
     });
 
     plugins$ = discoveryResult.plugins$.pipe(toArray());
@@ -183,7 +221,10 @@ describe('parsing plugin manifest', () => {
     await expect(plugins$.toPromise()).resolves.toEqual([]);
     await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
 Array [
-  "Error: Unexpected end of JSON input (invalid-manifest, /path/existent-dir/kibana.json)",
+  "Error: Unexpected end of JSON input (invalid-manifest, ${resolveForSnapshot(
+    TEST_PATHS.paths.existentDir,
+    'kibana.json'
+  )})",
 ]
 `);
   });
@@ -196,7 +237,10 @@ Array [
     await expect(plugins$.toPromise()).resolves.toEqual([]);
     await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
 Array [
-  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, /path/existent-dir/kibana.json)",
+  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, ${resolveForSnapshot(
+    TEST_PATHS.paths.existentDir,
+    'kibana.json'
+  )})",
 ]
 `);
   });
@@ -209,7 +253,10 @@ Array [
     await expect(plugins$.toPromise()).resolves.toEqual([]);
     await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
 Array [
-  "Error: Unexpected token o in JSON at position 1 (invalid-manifest, /path/existent-dir/kibana.json)",
+  "Error: Unexpected token o in JSON at position 1 (invalid-manifest, ${resolveForSnapshot(
+    TEST_PATHS.paths.existentDir,
+    'kibana.json'
+  )})",
 ]
 `);
   });
@@ -222,7 +269,10 @@ Array [
     await expect(plugins$.toPromise()).resolves.toEqual([]);
     await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
 Array [
-  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, /path/existent-dir/kibana.json)",
+  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, ${resolveForSnapshot(
+    TEST_PATHS.paths.existentDir,
+    'kibana.json'
+  )})",
 ]
 `);
   });
@@ -235,7 +285,10 @@ Array [
     await expect(plugins$.toPromise()).resolves.toEqual([]);
     await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
 Array [
-  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, /path/existent-dir/kibana.json)",
+  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, ${resolveForSnapshot(
+    TEST_PATHS.paths.existentDir,
+    'kibana.json'
+  )})",
 ]
 `);
   });
@@ -256,7 +309,7 @@ Array [
       "ui": false,
       "version": "some-version",
     },
-    "path": "/path/existent-dir",
+    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir)}",
   },
 ]
 `);
@@ -296,7 +349,7 @@ Array [
       "ui": true,
       "version": "some-version",
     },
-    "path": "/path/existent-dir",
+    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir)}",
   },
 ]
 `);

From aff6a9710a4c899fc594b495d3045452187afd79 Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Fri, 2 Nov 2018 16:57:23 +0100
Subject: [PATCH 05/15] =?UTF-8?q?Review=20=E2=84=962:=20make=20`cause`=20r?=
 =?UTF-8?q?equired=20parameter=20for=20`PluginDiscoveryError`.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 src/core/server/plugins/plugin_discovery_error.ts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/core/server/plugins/plugin_discovery_error.ts b/src/core/server/plugins/plugin_discovery_error.ts
index 924dec5d8d3d4..857725f1d9755 100644
--- a/src/core/server/plugins/plugin_discovery_error.ts
+++ b/src/core/server/plugins/plugin_discovery_error.ts
@@ -28,9 +28,9 @@ export class PluginDiscoveryError extends Error {
   constructor(
     public readonly type: PluginDiscoveryErrorType,
     public readonly path: string,
-    public readonly cause?: Error
+    public readonly cause: Error
   ) {
-    super(cause && cause.message);
+    super(cause.message);
 
     // Set the prototype explicitly, see:
     // https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work

From 6257e41085b93bc0761b5ea3a4ecdb936c48858a Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Fri, 2 Nov 2018 17:09:51 +0100
Subject: [PATCH 06/15] =?UTF-8?q?Review=20=E2=84=962:=20use=20`discover`?=
 =?UTF-8?q?=20function=20instead=20of=20`PluginDiscovery`=20class.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../server/plugins/plugin_discovery.test.ts   |   9 +-
 src/core/server/plugins/plugins_discovery.ts  | 221 +++++++++---------
 src/core/server/plugins/plugins_service.ts    |  11 +-
 3 files changed, 117 insertions(+), 124 deletions(-)

diff --git a/src/core/server/plugins/plugin_discovery.test.ts b/src/core/server/plugins/plugin_discovery.test.ts
index dc446988c5155..91a54b6bc3198 100644
--- a/src/core/server/plugins/plugin_discovery.test.ts
+++ b/src/core/server/plugins/plugin_discovery.test.ts
@@ -30,7 +30,7 @@ import { resolve } from 'path';
 import { Observable } from 'rxjs';
 import { map, toArray } from 'rxjs/operators';
 import { logger } from '../logging/__mocks__';
-import { PluginManifest, PluginsDiscovery } from './plugins_discovery';
+import { discover, PluginManifest } from './plugins_discovery';
 
 /**
  * Resolves absolute path and escapes backslashes (used on windows systems).
@@ -55,7 +55,6 @@ const TEST_PATHS = {
   },
 };
 
-let discovery: PluginsDiscovery;
 beforeEach(() => {
   mockReaddir.mockImplementation((path, cb) => {
     if (path === TEST_PATHS.scanDirs.nonEmpty) {
@@ -88,8 +87,6 @@ beforeEach(() => {
       cb(null, Buffer.from(JSON.stringify({ id: 'plugin', version: '1' })));
     }
   });
-
-  discovery = new PluginsDiscovery(logger.get());
 });
 
 afterEach(() => {
@@ -97,7 +94,7 @@ afterEach(() => {
 });
 
 test('properly scans folders and paths', async () => {
-  const { plugins$, errors$ } = discovery.discover({
+  const { plugins$, errors$ } = discover(logger.get(), {
     initialize: true,
     scanDirs: Object.values(TEST_PATHS.scanDirs),
     paths: Object.values(TEST_PATHS.paths),
@@ -200,7 +197,7 @@ describe('parsing plugin manifest', () => {
   let plugins$: Observable<Array<{ path: string; manifest: PluginManifest }>>;
   let errors$: Observable<string[]>;
   beforeEach(async () => {
-    const discoveryResult = discovery.discover({
+    const discoveryResult = discover(logger.get(), {
       initialize: true,
       scanDirs: [],
       paths: [TEST_PATHS.paths.existentDir],
diff --git a/src/core/server/plugins/plugins_discovery.ts b/src/core/server/plugins/plugins_discovery.ts
index c24acf0c78ab2..ac7c0555074e1 100644
--- a/src/core/server/plugins/plugins_discovery.ts
+++ b/src/core/server/plugins/plugins_discovery.ts
@@ -80,123 +80,118 @@ interface DiscoveryResult {
 const MANIFEST_FILE_NAME = 'kibana.json';
 
 /**
- * This class is responsible for discovering all recognizable plugins based on the
- * provided plugin config that defines top-level directories for groups of plugins
- * or/and direct paths to specific plugins.
+ * Tries to discover all possible plugins based on the provided plugin config.
+ * Discovery result consists of two separate streams, the one (`plugins$`) is
+ * for the successfully discovered plugins and the other one (`errors$`) is for
+ * all the errors that occurred during discovery process.
+ * @param log Plugin discovery logger instance.
+ * @param config Plugin config instance.
  */
-export class PluginsDiscovery {
-  constructor(private readonly log: Logger) {}
-
-  /**
-   * Tries to discover all possible plugins based on the provided plugin config.
-   * Discovery result consists of two separate streams, the one (`plugins$`) is
-   * for the successfully discovered plugins and the other one (`errors$`) is for
-   * all the errors that occurred during discovery process.
-   * @param config Plugin config instance.
-   */
-  public discover(config: PluginsConfig) {
-    this.log.debug('Discovering plugins...');
-
-    const discoveryResults$ = merge(
-      this.processScanDirs$(config.scanDirs),
-      this.processPaths$(config.paths)
-    ).pipe(
-      mergeMap(pluginPathOrError => {
-        return typeof pluginPathOrError === 'string'
-          ? this.createPlugin$(pluginPathOrError)
-          : [pluginPathOrError];
-      }),
-      shareReplay()
-    );
-
-    return {
-      plugins$: discoveryResults$.pipe(
-        mergeMap(entry => (entry.plugin !== undefined ? [entry.plugin] : []))
-      ),
-      errors$: discoveryResults$.pipe(
-        mergeMap(entry => (entry.error !== undefined ? [entry.error] : []))
-      ),
-    };
-  }
+export function discover(log: Logger, config: PluginsConfig) {
+  log.debug('Discovering plugins...');
+
+  const discoveryResults$ = merge(
+    processScanDirs$(log, config.scanDirs),
+    processPaths$(log, config.paths)
+  ).pipe(
+    mergeMap(pluginPathOrError => {
+      return typeof pluginPathOrError === 'string'
+        ? createPlugin$(log, pluginPathOrError)
+        : [pluginPathOrError];
+    }),
+    shareReplay()
+  );
+
+  return {
+    plugins$: discoveryResults$.pipe(
+      mergeMap(entry => (entry.plugin !== undefined ? [entry.plugin] : []))
+    ),
+    errors$: discoveryResults$.pipe(
+      mergeMap(entry => (entry.error !== undefined ? [entry.error] : []))
+    ),
+  };
+}
 
-  /**
-   * Iterates over every entry in `scanDirs` and returns a merged stream of all
-   * sub-directories. If directory cannot be read or it's impossible to get stat
-   * for any of the nested entries then error is added into the stream instead.
-   * @param scanDirs List of the top-level directories to process.
-   */
-  private processScanDirs$(scanDirs: string[]) {
-    return from(scanDirs).pipe(
-      mergeMap(dir => {
-        this.log.debug(`Scanning "${dir}" for plugin sub-directories...`);
-
-        return fsReadDir$(dir).pipe(
-          mergeMap(subDirs => subDirs.map(subDir => resolve(dir, subDir))),
-          mergeMap(path =>
-            fsStat$(path).pipe(
-              // Filter out non-directory entries from target directories, it's expected that
-              // these directories may contain files (e.g. `README.md` or `package.json`).
-              // We shouldn't silently ignore the entries we couldn't get stat for though.
-              mergeMap(pathStat => (pathStat.isDirectory() ? [path] : [])),
-              catchError(err => [
-                wrapError(PluginDiscoveryErrorType.InvalidPluginDirectory, path, err),
-              ])
-            )
-          ),
-          catchError(err => [wrapError(PluginDiscoveryErrorType.InvalidScanDirectory, dir, err)])
-        );
-      })
-    );
-  }
+/**
+ * Iterates over every entry in `scanDirs` and returns a merged stream of all
+ * sub-directories. If directory cannot be read or it's impossible to get stat
+ * for any of the nested entries then error is added into the stream instead.
+ * @param log Plugin discovery logger instance.
+ * @param scanDirs List of the top-level directories to process.
+ */
+function processScanDirs$(log: Logger, scanDirs: string[]) {
+  return from(scanDirs).pipe(
+    mergeMap(dir => {
+      log.debug(`Scanning "${dir}" for plugin sub-directories...`);
+
+      return fsReadDir$(dir).pipe(
+        mergeMap(subDirs => subDirs.map(subDir => resolve(dir, subDir))),
+        mergeMap(path =>
+          fsStat$(path).pipe(
+            // Filter out non-directory entries from target directories, it's expected that
+            // these directories may contain files (e.g. `README.md` or `package.json`).
+            // We shouldn't silently ignore the entries we couldn't get stat for though.
+            mergeMap(pathStat => (pathStat.isDirectory() ? [path] : [])),
+            catchError(err => [
+              wrapError(PluginDiscoveryErrorType.InvalidPluginDirectory, path, err),
+            ])
+          )
+        ),
+        catchError(err => [wrapError(PluginDiscoveryErrorType.InvalidScanDirectory, dir, err)])
+      );
+    })
+  );
+}
 
-  /**
-   * Iterates over every entry in `paths` and returns a stream of all paths that
-   * are directories. If path is not a directory or it's impossible to get stat
-   * for this path then error is added into the stream instead.
-   * @param paths List of paths to process.
-   */
-  private processPaths$(paths: string[]) {
-    return from(paths).pipe(
-      mergeMap(path => {
-        this.log.debug(`Including "${path}" into the plugin path list.`);
-
-        return fsStat$(path).pipe(
-          // Since every path is specifically provided we should treat non-directory
-          // entries as mistakes we should report of.
-          mergeMap(pathStat => {
-            return pathStat.isDirectory()
-              ? [path]
-              : throwError(new Error(`${path} is not a directory.`));
-          }),
-          catchError(err => [wrapError(PluginDiscoveryErrorType.InvalidPluginDirectory, path, err)])
-        );
-      })
-    );
-  }
+/**
+ * Iterates over every entry in `paths` and returns a stream of all paths that
+ * are directories. If path is not a directory or it's impossible to get stat
+ * for this path then error is added into the stream instead.
+ * @param log Plugin discovery logger instance.
+ * @param paths List of paths to process.
+ */
+function processPaths$(log: Logger, paths: string[]) {
+  return from(paths).pipe(
+    mergeMap(path => {
+      log.debug(`Including "${path}" into the plugin path list.`);
+
+      return fsStat$(path).pipe(
+        // Since every path is specifically provided we should treat non-directory
+        // entries as mistakes we should report of.
+        mergeMap(pathStat => {
+          return pathStat.isDirectory()
+            ? [path]
+            : throwError(new Error(`${path} is not a directory.`));
+        }),
+        catchError(err => [wrapError(PluginDiscoveryErrorType.InvalidPluginDirectory, path, err)])
+      );
+    })
+  );
+}
 
-  /**
-   * Tries to load and parse the plugin manifest file located at the provided
-   * plugin directory path and throws if it fails to do so or plugin manifest
-   * isn't valid.
-   * @param path Path to the plugin directory where manifest should be loaded from.
-   */
-  private createPlugin$(path: string) {
-    const manifestPath = resolve(path, MANIFEST_FILE_NAME);
-    return fsReadFile$(manifestPath).pipe(
-      mergeMap(manifestContent => {
-        try {
-          const plugin = { path, manifest: parseManifest(manifestContent) };
-
-          this.log.debug(`Successfully discovered plugin "${plugin.manifest.id}" at "${path}"`);
-
-          return [{ plugin }];
-        } catch (err) {
-          return [wrapError(PluginDiscoveryErrorType.InvalidManifest, manifestPath, err)];
-        }
-      }),
-      catchError(err => [wrapError(PluginDiscoveryErrorType.MissingManifest, manifestPath, err)])
-    );
-  }
+/**
+ * Tries to load and parse the plugin manifest file located at the provided
+ * plugin directory path and throws if it fails to do so or plugin manifest
+ * isn't valid.
+ * @param log Plugin discovery logger instance.
+ * @param path Path to the plugin directory where manifest should be loaded from.
+ */
+function createPlugin$(log: Logger, path: string) {
+  const manifestPath = resolve(path, MANIFEST_FILE_NAME);
+  return fsReadFile$(manifestPath).pipe(
+    mergeMap(manifestContent => {
+      try {
+        const plugin = { path, manifest: parseManifest(manifestContent) };
+
+        log.debug(`Successfully discovered plugin "${plugin.manifest.id}" at "${path}"`);
+
+        return [{ plugin }];
+      } catch (err) {
+        return [wrapError(PluginDiscoveryErrorType.InvalidManifest, manifestPath, err)];
+      }
+    }),
+    catchError(err => [wrapError(PluginDiscoveryErrorType.MissingManifest, manifestPath, err)])
+  );
 }
 
 /**
diff --git a/src/core/server/plugins/plugins_service.ts b/src/core/server/plugins/plugins_service.ts
index 612122cc71725..6f6fc705f7d20 100644
--- a/src/core/server/plugins/plugins_service.ts
+++ b/src/core/server/plugins/plugins_service.ts
@@ -23,15 +23,16 @@ import { ConfigService } from '../config';
 import { Logger, LoggerFactory } from '../logging';
 import { PluginDiscoveryErrorType } from './plugin_discovery_error';
 import { PluginsConfig } from './plugins_config';
-import { PluginsDiscovery } from './plugins_discovery';
+import { discover } from './plugins_discovery';
 
 export class PluginsService implements CoreService {
   private readonly log: Logger;
-  private readonly discovery: PluginsDiscovery;
 
-  constructor(logger: LoggerFactory, private readonly configService: ConfigService) {
+  constructor(
+    private readonly logger: LoggerFactory,
+    private readonly configService: ConfigService
+  ) {
     this.log = logger.get('plugins', 'service');
-    this.discovery = new PluginsDiscovery(logger.get('plugins', 'discovery'));
   }
 
   public async start() {
@@ -41,7 +42,7 @@ export class PluginsService implements CoreService {
       .atPath('plugins', PluginsConfig)
       .pipe(
         first(),
-        map(config => this.discovery.discover(config))
+        map(config => discover(this.logger.get('plugins', 'discovery'), config))
       )
       .toPromise();
 

From b2e1ea18fbb7574c61b7613c655c336594648413 Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Fri, 2 Nov 2018 17:31:40 +0100
Subject: [PATCH 07/15] =?UTF-8?q?Review=20=E2=84=962:=20rename=20`plugins$?=
 =?UTF-8?q?/errors$`=20to=20`plugin$/error$`.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 src/core/server/plugins/plugin_discovery.test.ts | 10 +++++-----
 src/core/server/plugins/plugins_discovery.ts     |  8 ++++----
 src/core/server/plugins/plugins_service.ts       |  6 +++---
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/core/server/plugins/plugin_discovery.test.ts b/src/core/server/plugins/plugin_discovery.test.ts
index 91a54b6bc3198..f6260ea30ab7d 100644
--- a/src/core/server/plugins/plugin_discovery.test.ts
+++ b/src/core/server/plugins/plugin_discovery.test.ts
@@ -94,13 +94,13 @@ afterEach(() => {
 });
 
 test('properly scans folders and paths', async () => {
-  const { plugins$, errors$ } = discover(logger.get(), {
+  const { plugin$, error$ } = discover(logger.get(), {
     initialize: true,
     scanDirs: Object.values(TEST_PATHS.scanDirs),
     paths: Object.values(TEST_PATHS.paths),
   });
 
-  await expect(plugins$.pipe(toArray()).toPromise()).resolves.toMatchInlineSnapshot(`
+  await expect(plugin$.pipe(toArray()).toPromise()).resolves.toMatchInlineSnapshot(`
 Array [
   Object {
     "manifest": Object {
@@ -161,7 +161,7 @@ Array [
 `);
 
   await expect(
-    errors$
+    error$
       .pipe(
         map(error => error.toString()),
         toArray()
@@ -203,8 +203,8 @@ describe('parsing plugin manifest', () => {
       paths: [TEST_PATHS.paths.existentDir],
     });
 
-    plugins$ = discoveryResult.plugins$.pipe(toArray());
-    errors$ = discoveryResult.errors$.pipe(
+    plugins$ = discoveryResult.plugin$.pipe(toArray());
+    errors$ = discoveryResult.error$.pipe(
       map(error => error.toString()),
       toArray()
     );
diff --git a/src/core/server/plugins/plugins_discovery.ts b/src/core/server/plugins/plugins_discovery.ts
index ac7c0555074e1..5e17709093828 100644
--- a/src/core/server/plugins/plugins_discovery.ts
+++ b/src/core/server/plugins/plugins_discovery.ts
@@ -81,8 +81,8 @@ const MANIFEST_FILE_NAME = 'kibana.json';
 
 /**
  * Tries to discover all possible plugins based on the provided plugin config.
- * Discovery result consists of two separate streams, the one (`plugins$`) is
- * for the successfully discovered plugins and the other one (`errors$`) is for
+ * Discovery result consists of two separate streams, the one (`plugin$`) is
+ * for the successfully discovered plugins and the other one (`error$`) is for
  * all the errors that occurred during discovery process.
  * @param log Plugin discovery logger instance.
  * @param config Plugin config instance.
@@ -103,10 +103,10 @@ export function discover(log: Logger, config: PluginsConfig) {
   );
 
   return {
-    plugins$: discoveryResults$.pipe(
+    plugin$: discoveryResults$.pipe(
       mergeMap(entry => (entry.plugin !== undefined ? [entry.plugin] : []))
     ),
-    errors$: discoveryResults$.pipe(
+    error$: discoveryResults$.pipe(
       mergeMap(entry => (entry.error !== undefined ? [entry.error] : []))
     ),
   };
diff --git a/src/core/server/plugins/plugins_service.ts b/src/core/server/plugins/plugins_service.ts
index 6f6fc705f7d20..1bedfc512c281 100644
--- a/src/core/server/plugins/plugins_service.ts
+++ b/src/core/server/plugins/plugins_service.ts
@@ -38,7 +38,7 @@ export class PluginsService implements CoreService {
   public async start() {
     this.log.debug('starting plugins service');
 
-    const { errors$, plugins$ } = await this.configService
+    const { error$, plugin$ } = await this.configService
       .atPath('plugins', PluginsConfig)
       .pipe(
         first(),
@@ -46,14 +46,14 @@ export class PluginsService implements CoreService {
       )
       .toPromise();
 
-    await plugins$
+    await plugin$
       .pipe(
         toArray(),
         tap(plugins => this.log.debug(`Discovered ${plugins.length} plugins.`))
       )
       .toPromise();
 
-    await errors$
+    await error$
       .pipe(
         filter(error => error.type === PluginDiscoveryErrorType.InvalidManifest),
         tap(invalidManifestError => this.log.error(invalidManifestError))

From dd5536bf555f1321b60621e9622028506081b57e Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Fri, 2 Nov 2018 20:08:13 +0100
Subject: [PATCH 08/15] =?UTF-8?q?Review=20=E2=84=962:=20improve=20`createP?=
 =?UTF-8?q?lugin$`=20JSDoc.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 src/core/server/plugins/plugins_discovery.ts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/core/server/plugins/plugins_discovery.ts b/src/core/server/plugins/plugins_discovery.ts
index 5e17709093828..f35b38e451728 100644
--- a/src/core/server/plugins/plugins_discovery.ts
+++ b/src/core/server/plugins/plugins_discovery.ts
@@ -170,8 +170,8 @@ function processPaths$(log: Logger, paths: string[]) {
 }
 
 /**
- * Tries to load and parse the plugin manifest file located at the provided
- * plugin directory path and throws if it fails to do so or plugin manifest
+ * Tries to load and parse the plugin manifest file located at the provided plugin
+ * directory path and produces an error result if it fails to do so or plugin manifest
  * isn't valid.
  * @param log Plugin discovery logger instance.
  * @param path Path to the plugin directory where manifest should be loaded from.

From 8e836b95823155c1ec3ebf3d2d5fc830af5fa9a8 Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Fri, 2 Nov 2018 20:28:26 +0100
Subject: [PATCH 09/15] =?UTF-8?q?Review=20=E2=84=962:=20throw=20more=20spe?=
 =?UTF-8?q?cific=20errors=20while=20parsing=20plugin=20manifest.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../server/plugins/plugin_discovery.test.ts   |  8 ++--
 src/core/server/plugins/plugins_discovery.ts  | 40 ++++++++++---------
 2 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/src/core/server/plugins/plugin_discovery.test.ts b/src/core/server/plugins/plugin_discovery.test.ts
index f6260ea30ab7d..54e4fb13095e5 100644
--- a/src/core/server/plugins/plugin_discovery.test.ts
+++ b/src/core/server/plugins/plugin_discovery.test.ts
@@ -174,7 +174,7 @@ Array [
     '2-no-manifest',
     'kibana.json'
   )})",
-  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, ${resolveForSnapshot(
+  "Error: Plugin manifest must contain an \\"id\\" property. (invalid-manifest, ${resolveForSnapshot(
     TEST_PATHS.scanDirs.nonEmpty,
     '4-incomplete-manifest',
     'kibana.json'
@@ -234,7 +234,7 @@ Array [
     await expect(plugins$.toPromise()).resolves.toEqual([]);
     await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
 Array [
-  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, ${resolveForSnapshot(
+  "Error: Plugin manifest must contain a JSON encoded object. (invalid-manifest, ${resolveForSnapshot(
     TEST_PATHS.paths.existentDir,
     'kibana.json'
   )})",
@@ -266,7 +266,7 @@ Array [
     await expect(plugins$.toPromise()).resolves.toEqual([]);
     await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
 Array [
-  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, ${resolveForSnapshot(
+  "Error: Plugin manifest must contain an \\"id\\" property. (invalid-manifest, ${resolveForSnapshot(
     TEST_PATHS.paths.existentDir,
     'kibana.json'
   )})",
@@ -282,7 +282,7 @@ Array [
     await expect(plugins$.toPromise()).resolves.toEqual([]);
     await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
 Array [
-  "Error: The \\"id\\" or/and \\"version\\" is missing in the plugin manifest. (invalid-manifest, ${resolveForSnapshot(
+  "Error: Plugin manifest for \\"some-id\\" must contain a \\"version\\" property. (invalid-manifest, ${resolveForSnapshot(
     TEST_PATHS.paths.existentDir,
     'kibana.json'
   )})",
diff --git a/src/core/server/plugins/plugins_discovery.ts b/src/core/server/plugins/plugins_discovery.ts
index f35b38e451728..e5db2f4426aad 100644
--- a/src/core/server/plugins/plugins_discovery.ts
+++ b/src/core/server/plugins/plugins_discovery.ts
@@ -200,27 +200,29 @@ function createPlugin$(log: Logger, path: string) {
  */
 function parseManifest(rawManifest: Buffer): PluginManifest {
   const manifest: Partial<PluginManifest> = JSON.parse(rawManifest.toString());
-  if (
-    manifest &&
-    manifest.id &&
-    manifest.version &&
-    typeof manifest.id === 'string' &&
-    typeof manifest.version === 'string'
-  ) {
-    return {
-      id: manifest.id,
-      version: manifest.version,
-      kibanaVersion:
-        typeof manifest.kibanaVersion === 'string' && manifest.kibanaVersion
-          ? manifest.kibanaVersion
-          : manifest.version,
-      requiredPlugins: Array.isArray(manifest.requiredPlugins) ? manifest.requiredPlugins : [],
-      optionalPlugins: Array.isArray(manifest.optionalPlugins) ? manifest.optionalPlugins : [],
-      ui: typeof manifest.ui === 'boolean' ? manifest.ui : false,
-    };
+  if (!manifest || typeof manifest !== 'object') {
+    throw new Error('Plugin manifest must contain a JSON encoded object.');
   }
 
-  throw new Error('The "id" or/and "version" is missing in the plugin manifest.');
+  if (!manifest.id || typeof manifest.id !== 'string') {
+    throw new Error('Plugin manifest must contain an "id" property.');
+  }
+
+  if (!manifest.version || typeof manifest.version !== 'string') {
+    throw new Error(`Plugin manifest for "${manifest.id}" must contain a "version" property.`);
+  }
+
+  return {
+    id: manifest.id,
+    version: manifest.version,
+    kibanaVersion:
+      typeof manifest.kibanaVersion === 'string' && manifest.kibanaVersion
+        ? manifest.kibanaVersion
+        : manifest.version,
+    requiredPlugins: Array.isArray(manifest.requiredPlugins) ? manifest.requiredPlugins : [],
+    optionalPlugins: Array.isArray(manifest.optionalPlugins) ? manifest.optionalPlugins : [],
+    ui: typeof manifest.ui === 'boolean' ? manifest.ui : false,
+  };
 }
 
 /**

From c83f084f8c75838f0b00d32b9a372f0298045fce Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Fri, 2 Nov 2018 20:32:28 +0100
Subject: [PATCH 10/15] =?UTF-8?q?Review=20=E2=84=962:=20log=20errors=20bef?=
 =?UTF-8?q?ore=20discovery=20result=20message.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 src/core/server/plugins/plugins_service.ts | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/core/server/plugins/plugins_service.ts b/src/core/server/plugins/plugins_service.ts
index 1bedfc512c281..b29715dadc8dc 100644
--- a/src/core/server/plugins/plugins_service.ts
+++ b/src/core/server/plugins/plugins_service.ts
@@ -46,17 +46,17 @@ export class PluginsService implements CoreService {
       )
       .toPromise();
 
-    await plugin$
+    await error$
       .pipe(
-        toArray(),
-        tap(plugins => this.log.debug(`Discovered ${plugins.length} plugins.`))
+        filter(error => error.type === PluginDiscoveryErrorType.InvalidManifest),
+        tap(invalidManifestError => this.log.error(invalidManifestError))
       )
       .toPromise();
 
-    await error$
+    await plugin$
       .pipe(
-        filter(error => error.type === PluginDiscoveryErrorType.InvalidManifest),
-        tap(invalidManifestError => this.log.error(invalidManifestError))
+        toArray(),
+        tap(plugins => this.log.debug(`Discovered ${plugins.length} plugins.`))
       )
       .toPromise();
   }

From 89a85ea77a6f1c83c89e33610702f429cc856a80 Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Mon, 5 Nov 2018 15:57:23 +0100
Subject: [PATCH 11/15] Add version compatibility check.

---
 src/core/server/config/env.ts                 |   2 +-
 src/core/server/config/index.ts               |   2 +-
 src/core/server/index.ts                      |   2 +-
 src/core/server/plugins/discovery/index.ts    |  21 ++
 .../{ => discovery}/plugin_discovery.test.ts  | 178 +++++++++++--
 .../{ => discovery}/plugin_discovery_error.ts |  26 ++
 .../discovery/plugin_manifest_parser.ts       | 174 +++++++++++++
 .../plugins/discovery/plugins_discovery.ts    | 156 ++++++++++++
 src/core/server/plugins/index.ts              |   6 +-
 src/core/server/plugins/plugins_discovery.ts  | 238 ------------------
 src/core/server/plugins/plugins_service.ts    |  10 +-
 11 files changed, 542 insertions(+), 273 deletions(-)
 create mode 100644 src/core/server/plugins/discovery/index.ts
 rename src/core/server/plugins/{ => discovery}/plugin_discovery.test.ts (68%)
 rename src/core/server/plugins/{ => discovery}/plugin_discovery_error.ts (58%)
 create mode 100644 src/core/server/plugins/discovery/plugin_manifest_parser.ts
 create mode 100644 src/core/server/plugins/discovery/plugins_discovery.ts
 delete mode 100644 src/core/server/plugins/plugins_discovery.ts

diff --git a/src/core/server/config/env.ts b/src/core/server/config/env.ts
index 3c20c126f33fe..2651514d31049 100644
--- a/src/core/server/config/env.ts
+++ b/src/core/server/config/env.ts
@@ -22,7 +22,7 @@ import process from 'process';
 
 import { pkg } from '../../../utils/package_json';
 
-interface PackageInfo {
+export interface PackageInfo {
   version: string;
   branch: string;
   buildNum: number;
diff --git a/src/core/server/config/index.ts b/src/core/server/config/index.ts
index bbddec03a0f41..51fcd8a41fbc3 100644
--- a/src/core/server/config/index.ts
+++ b/src/core/server/config/index.ts
@@ -22,5 +22,5 @@ export { RawConfigService } from './raw_config_service';
 export { Config, ConfigPath } from './config';
 /** @internal */
 export { ObjectToConfigAdapter } from './object_to_config_adapter';
-export { Env, CliArgs } from './env';
+export { Env, CliArgs, PackageInfo } from './env';
 export { ConfigWithSchema } from './config_with_schema';
diff --git a/src/core/server/index.ts b/src/core/server/index.ts
index f1d7d29dd9f6c..0c9518f87e010 100644
--- a/src/core/server/index.ts
+++ b/src/core/server/index.ts
@@ -41,7 +41,7 @@ export class Server {
     this.log = logger.get('server');
 
     this.http = new HttpModule(configService.atPath('server', HttpConfig), logger);
-    this.plugins = new PluginsModule(configService, logger);
+    this.plugins = new PluginsModule(configService, logger, env);
     this.legacy = new LegacyCompatModule(configService, logger, env);
   }
 
diff --git a/src/core/server/plugins/discovery/index.ts b/src/core/server/plugins/discovery/index.ts
new file mode 100644
index 0000000000000..768112cf7f6fe
--- /dev/null
+++ b/src/core/server/plugins/discovery/index.ts
@@ -0,0 +1,21 @@
+/*
+ * 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.
+ */
+
+export { PluginDiscoveryErrorType } from './plugin_discovery_error';
+export { discover } from './plugins_discovery';
diff --git a/src/core/server/plugins/plugin_discovery.test.ts b/src/core/server/plugins/discovery/plugin_discovery.test.ts
similarity index 68%
rename from src/core/server/plugins/plugin_discovery.test.ts
rename to src/core/server/plugins/discovery/plugin_discovery.test.ts
index 54e4fb13095e5..ba22b359c7acb 100644
--- a/src/core/server/plugins/plugin_discovery.test.ts
+++ b/src/core/server/plugins/discovery/plugin_discovery.test.ts
@@ -29,8 +29,9 @@ jest.mock('fs', () => ({
 import { resolve } from 'path';
 import { Observable } from 'rxjs';
 import { map, toArray } from 'rxjs/operators';
-import { logger } from '../logging/__mocks__';
-import { discover, PluginManifest } from './plugins_discovery';
+import { logger } from '../../logging/__mocks__';
+import { PluginManifest } from './plugin_manifest_parser';
+import { discover } from './plugins_discovery';
 
 /**
  * Resolves absolute path and escapes backslashes (used on windows systems).
@@ -60,7 +61,7 @@ beforeEach(() => {
     if (path === TEST_PATHS.scanDirs.nonEmpty) {
       cb(null, ['1', '2-no-manifest', '3', '4-incomplete-manifest']);
     } else if (path === TEST_PATHS.scanDirs.nonEmpty2) {
-      cb(null, ['5-invalid-manifest', '6', '7-non-dir']);
+      cb(null, ['5-invalid-manifest', '6', '7-non-dir', '8-incompatible-manifest']);
     } else if (path === TEST_PATHS.scanDirs.nonExistent) {
       cb(new Error('ENOENT'));
     } else {
@@ -83,8 +84,10 @@ beforeEach(() => {
       cb(null, Buffer.from('not-json'));
     } else if (path.includes('incomplete-manifest')) {
       cb(null, Buffer.from(JSON.stringify({ version: '1' })));
-    } else {
+    } else if (path.includes('incompatible-manifest')) {
       cb(null, Buffer.from(JSON.stringify({ id: 'plugin', version: '1' })));
+    } else {
+      cb(null, Buffer.from(JSON.stringify({ id: 'plugin', version: '1', kibanaVersion: '1.2.3' })));
     }
   });
 });
@@ -94,18 +97,27 @@ afterEach(() => {
 });
 
 test('properly scans folders and paths', async () => {
-  const { plugin$, error$ } = discover(logger.get(), {
-    initialize: true,
-    scanDirs: Object.values(TEST_PATHS.scanDirs),
-    paths: Object.values(TEST_PATHS.paths),
-  });
+  const { plugin$, error$ } = discover(
+    {
+      initialize: true,
+      scanDirs: Object.values(TEST_PATHS.scanDirs),
+      paths: Object.values(TEST_PATHS.paths),
+    },
+    {
+      branch: 'master',
+      buildNum: 1,
+      buildSha: '',
+      version: '1.2.3',
+    },
+    logger.get()
+  );
 
   await expect(plugin$.pipe(toArray()).toPromise()).resolves.toMatchInlineSnapshot(`
 Array [
   Object {
     "manifest": Object {
       "id": "plugin",
-      "kibanaVersion": "1",
+      "kibanaVersion": "1.2.3",
       "optionalPlugins": Array [],
       "requiredPlugins": Array [],
       "ui": false,
@@ -116,7 +128,7 @@ Array [
   Object {
     "manifest": Object {
       "id": "plugin",
-      "kibanaVersion": "1",
+      "kibanaVersion": "1.2.3",
       "optionalPlugins": Array [],
       "requiredPlugins": Array [],
       "ui": false,
@@ -127,7 +139,7 @@ Array [
   Object {
     "manifest": Object {
       "id": "plugin",
-      "kibanaVersion": "1",
+      "kibanaVersion": "1.2.3",
       "optionalPlugins": Array [],
       "requiredPlugins": Array [],
       "ui": false,
@@ -138,7 +150,7 @@ Array [
   Object {
     "manifest": Object {
       "id": "plugin",
-      "kibanaVersion": "1",
+      "kibanaVersion": "1.2.3",
       "optionalPlugins": Array [],
       "requiredPlugins": Array [],
       "ui": false,
@@ -149,7 +161,7 @@ Array [
   Object {
     "manifest": Object {
       "id": "plugin",
-      "kibanaVersion": "1",
+      "kibanaVersion": "1.2.3",
       "optionalPlugins": Array [],
       "requiredPlugins": Array [],
       "ui": false,
@@ -184,6 +196,11 @@ Array [
     '5-invalid-manifest',
     'kibana.json'
   )})",
+  "Error: Plugin \\"plugin\\" is only compatible with Kibana version \\"1\\", but used Kibana version is \\"1.2.3\\". (incompatible-version, ${resolveForSnapshot(
+    TEST_PATHS.scanDirs.nonEmpty2,
+    '8-incompatible-manifest',
+    'kibana.json'
+  )})",
   "Error: ENOENT (invalid-scan-dir, ${resolveForSnapshot(TEST_PATHS.scanDirs.nonExistent)})",
   "Error: ${resolveForSnapshot(
     TEST_PATHS.paths.nonDir
@@ -197,11 +214,20 @@ describe('parsing plugin manifest', () => {
   let plugins$: Observable<Array<{ path: string; manifest: PluginManifest }>>;
   let errors$: Observable<string[]>;
   beforeEach(async () => {
-    const discoveryResult = discover(logger.get(), {
-      initialize: true,
-      scanDirs: [],
-      paths: [TEST_PATHS.paths.existentDir],
-    });
+    const discoveryResult = discover(
+      {
+        initialize: true,
+        scanDirs: [],
+        paths: [TEST_PATHS.paths.existentDir],
+      },
+      {
+        branch: 'master',
+        buildNum: 1,
+        buildSha: '',
+        version: '7.0.0-alpha1',
+      },
+      logger.get()
+    );
 
     plugins$ = discoveryResult.plugin$.pipe(toArray());
     errors$ = discoveryResult.error$.pipe(
@@ -290,9 +316,41 @@ Array [
 `);
   });
 
+  test('return error when plugin expected Kibana version is lower than actual version', async () => {
+    mockReadFile.mockImplementation((path, cb) => {
+      cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '6.4.2' })));
+    });
+
+    await expect(plugins$.toPromise()).resolves.toEqual([]);
+    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
+Array [
+  "Error: Plugin \\"some-id\\" is only compatible with Kibana version \\"6.4.2\\", but used Kibana version is \\"7.0.0-alpha1\\". (incompatible-version, ${resolveForSnapshot(
+    TEST_PATHS.paths.existentDir,
+    'kibana.json'
+  )})",
+]
+`);
+  });
+
+  test('return error when plugin expected Kibana version is higher than actual version', async () => {
+    mockReadFile.mockImplementation((path, cb) => {
+      cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '7.0.1' })));
+    });
+
+    await expect(plugins$.toPromise()).resolves.toEqual([]);
+    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
+Array [
+  "Error: Plugin \\"some-id\\" is only compatible with Kibana version \\"7.0.1\\", but used Kibana version is \\"7.0.0-alpha1\\". (incompatible-version, ${resolveForSnapshot(
+    TEST_PATHS.paths.existentDir,
+    'kibana.json'
+  )})",
+]
+`);
+  });
+
   test('set defaults for all missing optional fields', async () => {
     mockReadFile.mockImplementation((path, cb) => {
-      cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: 'some-version' })));
+      cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '7.0.0' })));
     });
 
     await expect(plugins$.toPromise()).resolves.toMatchInlineSnapshot(`
@@ -300,13 +358,13 @@ Array [
   Object {
     "manifest": Object {
       "id": "some-id",
-      "kibanaVersion": "some-version",
+      "kibanaVersion": "7.0.0",
       "optionalPlugins": Array [],
       "requiredPlugins": Array [],
       "ui": false,
-      "version": "some-version",
+      "version": "7.0.0",
     },
-    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir)}",
+    "path": "/projects/elastic/master/kibana/path/existent-dir",
   },
 ]
 `);
@@ -321,7 +379,7 @@ Array [
           JSON.stringify({
             id: 'some-id',
             version: 'some-version',
-            kibanaVersion: 'some-kibana-version',
+            kibanaVersion: '7.0.0',
             requiredPlugins: ['some-required-plugin', 'some-required-plugin-2'],
             optionalPlugins: ['some-optional-plugin'],
             ui: true,
@@ -335,7 +393,7 @@ Array [
   Object {
     "manifest": Object {
       "id": "some-id",
-      "kibanaVersion": "some-kibana-version",
+      "kibanaVersion": "7.0.0",
       "optionalPlugins": Array [
         "some-optional-plugin",
       ],
@@ -349,6 +407,76 @@ Array [
     "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir)}",
   },
 ]
+`);
+    await expect(errors$.toPromise()).resolves.toEqual([]);
+  });
+
+  test('return manifest when plugin expected Kibana version matches actual version', async () => {
+    mockReadFile.mockImplementation((path, cb) => {
+      cb(
+        null,
+        Buffer.from(
+          JSON.stringify({
+            id: 'some-id',
+            version: 'some-version',
+            kibanaVersion: '7.0.0-alpha2',
+            requiredPlugins: ['some-required-plugin'],
+          })
+        )
+      );
+    });
+
+    await expect(plugins$.toPromise()).resolves.toMatchInlineSnapshot(`
+Array [
+  Object {
+    "manifest": Object {
+      "id": "some-id",
+      "kibanaVersion": "7.0.0-alpha2",
+      "optionalPlugins": Array [],
+      "requiredPlugins": Array [
+        "some-required-plugin",
+      ],
+      "ui": false,
+      "version": "some-version",
+    },
+    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir)}",
+  },
+]
+`);
+    await expect(errors$.toPromise()).resolves.toEqual([]);
+  });
+
+  test('return manifest when plugin expected Kibana version is `kibana`', async () => {
+    mockReadFile.mockImplementation((path, cb) => {
+      cb(
+        null,
+        Buffer.from(
+          JSON.stringify({
+            id: 'some-id',
+            version: 'some-version',
+            kibanaVersion: 'kibana',
+            requiredPlugins: ['some-required-plugin'],
+          })
+        )
+      );
+    });
+
+    await expect(plugins$.toPromise()).resolves.toMatchInlineSnapshot(`
+Array [
+  Object {
+    "manifest": Object {
+      "id": "some-id",
+      "kibanaVersion": "kibana",
+      "optionalPlugins": Array [],
+      "requiredPlugins": Array [
+        "some-required-plugin",
+      ],
+      "ui": false,
+      "version": "some-version",
+    },
+    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir)}",
+  },
+]
 `);
     await expect(errors$.toPromise()).resolves.toEqual([]);
   });
diff --git a/src/core/server/plugins/plugin_discovery_error.ts b/src/core/server/plugins/discovery/plugin_discovery_error.ts
similarity index 58%
rename from src/core/server/plugins/plugin_discovery_error.ts
rename to src/core/server/plugins/discovery/plugin_discovery_error.ts
index 857725f1d9755..7c68eb378456b 100644
--- a/src/core/server/plugins/plugin_discovery_error.ts
+++ b/src/core/server/plugins/discovery/plugin_discovery_error.ts
@@ -18,6 +18,7 @@
  */
 
 export enum PluginDiscoveryErrorType {
+  IncompatibleVersion = 'incompatible-version',
   InvalidScanDirectory = 'invalid-scan-dir',
   InvalidPluginDirectory = 'invalid-plugin-dir',
   InvalidManifest = 'invalid-manifest',
@@ -25,6 +26,31 @@ export enum PluginDiscoveryErrorType {
 }
 
 export class PluginDiscoveryError extends Error {
+  public static incompatibleVersion(path: string, cause: Error) {
+    return new PluginDiscoveryError(PluginDiscoveryErrorType.IncompatibleVersion, path, cause);
+  }
+
+  public static invalidScanDirectory(path: string, cause: Error) {
+    return new PluginDiscoveryError(PluginDiscoveryErrorType.InvalidScanDirectory, path, cause);
+  }
+
+  public static invalidPluginDirectory(path: string, cause: Error) {
+    return new PluginDiscoveryError(PluginDiscoveryErrorType.InvalidPluginDirectory, path, cause);
+  }
+
+  public static invalidManifest(path: string, cause: Error) {
+    return new PluginDiscoveryError(PluginDiscoveryErrorType.InvalidManifest, path, cause);
+  }
+
+  public static missingManifest(path: string, cause: Error) {
+    return new PluginDiscoveryError(PluginDiscoveryErrorType.MissingManifest, path, cause);
+  }
+
+  /**
+   * @param type Type of the discovery error (invalid directory, invalid manifest etc.)
+   * @param path Path at which discovery error occurred.
+   * @param cause "Raw" error object that caused discovery error.
+   */
   constructor(
     public readonly type: PluginDiscoveryErrorType,
     public readonly path: string,
diff --git a/src/core/server/plugins/discovery/plugin_manifest_parser.ts b/src/core/server/plugins/discovery/plugin_manifest_parser.ts
new file mode 100644
index 0000000000000..a0944e070c75e
--- /dev/null
+++ b/src/core/server/plugins/discovery/plugin_manifest_parser.ts
@@ -0,0 +1,174 @@
+/*
+ * 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 { readFile } from 'fs';
+import { resolve } from 'path';
+import { bindNodeCallback, throwError } from 'rxjs';
+import { catchError, map } from 'rxjs/operators';
+import { PackageInfo } from '../../config';
+import { PluginDiscoveryError } from './plugin_discovery_error';
+
+const fsReadFile$ = bindNodeCallback(readFile);
+
+/**
+ * Describes the set of required and optional properties plugin can define in its
+ * mandatory JSON manifest file.
+ */
+export interface PluginManifest {
+  /**
+   * Identifier of the plugin.
+   */
+  readonly id: string;
+
+  /**
+   * Version of the plugin.
+   */
+  readonly version: string;
+
+  /**
+   * The version of Kibana the plugin is compatible with, defaults to "version".
+   */
+  readonly kibanaVersion: string;
+
+  /**
+   * An optional list of the other plugins that **must be** installed and enabled
+   * for this plugin to function properly.
+   */
+  readonly requiredPlugins: ReadonlyArray<string>;
+
+  /**
+   * An optional list of the other plugins that if installed and enabled **may be**
+   * leveraged by this plugin for some additional functionality but otherwise are
+   * not required for this plugin to work properly.
+   */
+  readonly optionalPlugins: ReadonlyArray<string>;
+
+  /**
+   * Specifies whether plugin includes some client/browser specific functionality
+   * that should be included into client bundle via `public/ui_plugin.js` file.
+   */
+  readonly ui: boolean;
+}
+
+/**
+ * Name of the JSON manifest file that should be located in the plugin directory.
+ */
+const MANIFEST_FILE_NAME = 'kibana.json';
+
+/**
+ * The special "kibana" version can be used by the plugins to be always compatible.
+ */
+const ALWAYS_COMPATIBLE_VERSION = 'kibana';
+
+/**
+ * Regular expression used to extract semantic version part from the plugin or
+ * kibana version, e.g. `1.2.3` ---> `1.2.3` and `7.0.0-alpha1` ---> `7.0.0`.
+ */
+const SEM_VER_REGEX = /\d+\.\d+\.\d+/;
+
+/**
+ * Tries to load and parse the plugin manifest file located at the provided plugin
+ * directory path and produces an error result if it fails to do so or plugin manifest
+ * isn't valid.
+ * @param pluginPath Path to the plugin directory where manifest should be loaded from.
+ * @param packageInfo Kibana package info.
+ */
+export function parseManifest$(pluginPath: string, packageInfo: PackageInfo) {
+  const manifestPath = resolve(pluginPath, MANIFEST_FILE_NAME);
+  return fsReadFile$(manifestPath).pipe(
+    catchError(err => throwError(PluginDiscoveryError.missingManifest(manifestPath, err))),
+    map(manifestContent => {
+      let manifest: Partial<PluginManifest>;
+      try {
+        manifest = JSON.parse(manifestContent.toString());
+      } catch (err) {
+        throw PluginDiscoveryError.invalidManifest(manifestPath, err);
+      }
+
+      if (!manifest || typeof manifest !== 'object') {
+        throw PluginDiscoveryError.invalidManifest(
+          manifestPath,
+          new Error('Plugin manifest must contain a JSON encoded object.')
+        );
+      }
+
+      if (!manifest.id || typeof manifest.id !== 'string') {
+        throw PluginDiscoveryError.invalidManifest(
+          manifestPath,
+          new Error('Plugin manifest must contain an "id" property.')
+        );
+      }
+
+      if (!manifest.version || typeof manifest.version !== 'string') {
+        throw PluginDiscoveryError.invalidManifest(
+          manifestPath,
+          new Error(`Plugin manifest for "${manifest.id}" must contain a "version" property.`)
+        );
+      }
+
+      const expectedKibanaVersion =
+        typeof manifest.kibanaVersion === 'string' && manifest.kibanaVersion
+          ? manifest.kibanaVersion
+          : manifest.version;
+      if (!isVersionCompatible(expectedKibanaVersion, packageInfo.version)) {
+        throw PluginDiscoveryError.incompatibleVersion(
+          manifestPath,
+          new Error(
+            `Plugin "${
+              manifest.id
+            }" is only compatible with Kibana version "${expectedKibanaVersion}", but used Kibana version is "${
+              packageInfo.version
+            }".`
+          )
+        );
+      }
+
+      return {
+        id: manifest.id,
+        version: manifest.version,
+        kibanaVersion: expectedKibanaVersion,
+        requiredPlugins: Array.isArray(manifest.requiredPlugins) ? manifest.requiredPlugins : [],
+        optionalPlugins: Array.isArray(manifest.optionalPlugins) ? manifest.optionalPlugins : [],
+        ui: typeof manifest.ui === 'boolean' ? manifest.ui : false,
+      };
+    })
+  );
+}
+
+/**
+ * Checks whether plugin expected Kibana version is compatible with the used Kibana version.
+ * @param expectedKibanaVersion Kibana version expected by the plugin.
+ * @param actualKibanaVersion Used Kibana version.
+ */
+function isVersionCompatible(expectedKibanaVersion: string, actualKibanaVersion: string) {
+  if (expectedKibanaVersion === ALWAYS_COMPATIBLE_VERSION) {
+    return true;
+  }
+
+  return extractSemVer(actualKibanaVersion) === extractSemVer(expectedKibanaVersion);
+}
+
+/**
+ * Tries to extract semantic version part from the full version string.
+ * @param version
+ */
+function extractSemVer(version: string) {
+  const semVerMatch = version.match(SEM_VER_REGEX);
+  return semVerMatch === null ? version : semVerMatch[0];
+}
diff --git a/src/core/server/plugins/discovery/plugins_discovery.ts b/src/core/server/plugins/discovery/plugins_discovery.ts
new file mode 100644
index 0000000000000..12c2989dd4a11
--- /dev/null
+++ b/src/core/server/plugins/discovery/plugins_discovery.ts
@@ -0,0 +1,156 @@
+/*
+ * 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 { readdir, stat } from 'fs';
+import { resolve } from 'path';
+import { bindNodeCallback, from, merge, Observable, throwError } from 'rxjs';
+import { catchError, map, mergeMap, shareReplay } from 'rxjs/operators';
+import { PackageInfo } from '../../config';
+import { Logger } from '../../logging';
+import { PluginsConfig } from '../plugins_config';
+import { PluginDiscoveryError } from './plugin_discovery_error';
+import { parseManifest$, PluginManifest } from './plugin_manifest_parser';
+
+const fsReadDir$ = bindNodeCallback(readdir);
+const fsStat$ = bindNodeCallback(stat);
+
+interface DiscoveryResult {
+  plugin?: { path: string; manifest: PluginManifest };
+  error?: PluginDiscoveryError;
+}
+
+/**
+ * Tries to discover all possible plugins based on the provided plugin config.
+ * Discovery result consists of two separate streams, the one (`plugin$`) is
+ * for the successfully discovered plugins and the other one (`error$`) is for
+ * all the errors that occurred during discovery process.
+ *
+ * @param config Plugin config instance.
+ * @param packageInfo Kibana package info.
+ * @param log Plugin discovery logger instance.
+ */
+export function discover(config: PluginsConfig, packageInfo: PackageInfo, log: Logger) {
+  log.debug('Discovering plugins...');
+
+  const discoveryResults$ = merge(
+    processScanDirs$(config.scanDirs, log),
+    processPaths$(config.paths, log)
+  ).pipe(
+    mergeMap(pluginPathOrError => {
+      return typeof pluginPathOrError === 'string'
+        ? createPlugin$(pluginPathOrError, packageInfo, log)
+        : [pluginPathOrError];
+    }),
+    shareReplay()
+  );
+
+  return {
+    plugin$: discoveryResults$.pipe(
+      mergeMap(entry => (entry.plugin !== undefined ? [entry.plugin] : []))
+    ),
+    error$: discoveryResults$.pipe(
+      mergeMap(entry => (entry.error !== undefined ? [entry.error] : []))
+    ),
+  };
+}
+
+/**
+ * Iterates over every entry in `scanDirs` and returns a merged stream of all
+ * sub-directories. If directory cannot be read or it's impossible to get stat
+ * for any of the nested entries then error is added into the stream instead.
+ * @param scanDirs List of the top-level directories to process.
+ * @param log Plugin discovery logger instance.
+ */
+function processScanDirs$(scanDirs: string[], log: Logger) {
+  return from(scanDirs).pipe(
+    mergeMap(dir => {
+      log.debug(`Scanning "${dir}" for plugin sub-directories...`);
+
+      return fsReadDir$(dir).pipe(
+        mergeMap(subDirs => subDirs.map(subDir => resolve(dir, subDir))),
+        mergeMap(path =>
+          fsStat$(path).pipe(
+            // Filter out non-directory entries from target directories, it's expected that
+            // these directories may contain files (e.g. `README.md` or `package.json`).
+            // We shouldn't silently ignore the entries we couldn't get stat for though.
+            mergeMap(pathStat => (pathStat.isDirectory() ? [path] : [])),
+            catchError(err => [wrapError(PluginDiscoveryError.invalidPluginDirectory(path, err))])
+          )
+        ),
+        catchError(err => [wrapError(PluginDiscoveryError.invalidScanDirectory(dir, err))])
+      );
+    })
+  );
+}
+
+/**
+ * Iterates over every entry in `paths` and returns a stream of all paths that
+ * are directories. If path is not a directory or it's impossible to get stat
+ * for this path then error is added into the stream instead.
+ * @param paths List of paths to process.
+ * @param log Plugin discovery logger instance.
+ */
+function processPaths$(paths: string[], log: Logger) {
+  return from(paths).pipe(
+    mergeMap(path => {
+      log.debug(`Including "${path}" into the plugin path list.`);
+
+      return fsStat$(path).pipe(
+        // Since every path is specifically provided we should treat non-directory
+        // entries as mistakes we should report of.
+        mergeMap(pathStat => {
+          return pathStat.isDirectory()
+            ? [path]
+            : throwError(new Error(`${path} is not a directory.`));
+        }),
+        catchError(err => [wrapError(PluginDiscoveryError.invalidPluginDirectory(path, err))])
+      );
+    })
+  );
+}
+
+/**
+ * Tries to load and parse the plugin manifest file located at the provided plugin
+ * directory path and produces an error result if it fails to do so or plugin manifest
+ * isn't valid.
+ * @param path Path to the plugin directory where manifest should be loaded from.
+ * @param packageInfo Kibana package info.
+ * @param log Plugin discovery logger instance.
+ */
+function createPlugin$(
+  path: string,
+  packageInfo: PackageInfo,
+  log: Logger
+): Observable<DiscoveryResult> {
+  return parseManifest$(path, packageInfo).pipe(
+    map(manifest => {
+      log.debug(`Successfully discovered plugin "${manifest.id}" at "${path}"`);
+      return { plugin: { path, manifest } };
+    }),
+    catchError(err => [wrapError(err)])
+  );
+}
+
+/**
+ * Wraps `PluginDiscoveryError` into `DiscoveryResult` entry.
+ * @param error Instance of the `PluginDiscoveryError` error.
+ */
+function wrapError(error: PluginDiscoveryError): DiscoveryResult {
+  return { error };
+}
diff --git a/src/core/server/plugins/index.ts b/src/core/server/plugins/index.ts
index 01c2721e6172e..6ed7870b86c6d 100644
--- a/src/core/server/plugins/index.ts
+++ b/src/core/server/plugins/index.ts
@@ -17,14 +17,14 @@
  * under the License.
  */
 
-import { ConfigService } from '../config';
+import { ConfigService, Env } from '../config';
 import { LoggerFactory } from '../logging';
 import { PluginsService } from './plugins_service';
 
 export class PluginsModule {
   public readonly service: PluginsService;
 
-  constructor(private readonly configService: ConfigService, logger: LoggerFactory) {
-    this.service = new PluginsService(logger, this.configService);
+  constructor(private readonly configService: ConfigService, logger: LoggerFactory, env: Env) {
+    this.service = new PluginsService(env, logger, this.configService);
   }
 }
diff --git a/src/core/server/plugins/plugins_discovery.ts b/src/core/server/plugins/plugins_discovery.ts
deleted file mode 100644
index e5db2f4426aad..0000000000000
--- a/src/core/server/plugins/plugins_discovery.ts
+++ /dev/null
@@ -1,238 +0,0 @@
-/*
- * 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 { readdir, readFile, stat } from 'fs';
-import { resolve } from 'path';
-import { bindNodeCallback, from, merge, throwError } from 'rxjs';
-import { catchError, mergeMap, shareReplay } from 'rxjs/operators';
-import { Logger } from '../logging';
-import { PluginDiscoveryError, PluginDiscoveryErrorType } from './plugin_discovery_error';
-import { PluginsConfig } from './plugins_config';
-
-const fsReadDir$ = bindNodeCallback(readdir);
-const fsStat$ = bindNodeCallback(stat);
-const fsReadFile$ = bindNodeCallback(readFile);
-
-/**
- * Describes the set of required and optional properties plugin can define in its
- * mandatory JSON manifest file.
- */
-export interface PluginManifest {
-  /**
-   * Identifier of the plugin.
-   */
-  readonly id: string;
-
-  /**
-   * Version of the plugin.
-   */
-  readonly version: string;
-
-  /**
-   * The version of Kibana the plugin is compatible with, defaults to "version".
-   */
-  readonly kibanaVersion: string;
-
-  /**
-   * An optional list of the other plugins that **must be** installed and enabled
-   * for this plugin to function properly.
-   */
-  readonly requiredPlugins: ReadonlyArray<string>;
-
-  /**
-   * An optional list of the other plugins that if installed and enabled **may be**
-   * leveraged by this plugin for some additional functionality but otherwise are
-   * not required for this plugin to work properly.
-   */
-  readonly optionalPlugins: ReadonlyArray<string>;
-
-  /**
-   * Specifies whether plugin includes some client/browser specific functionality
-   * that should be included into client bundle via `public/ui_plugin.js` file.
-   */
-  readonly ui: boolean;
-}
-
-interface DiscoveryResult {
-  plugin?: { path: string; manifest: PluginManifest };
-  error?: PluginDiscoveryError;
-}
-
-/**
- * Name of the JSON manifest file that should be located in the plugin directory.
- */
-const MANIFEST_FILE_NAME = 'kibana.json';
-
-/**
- * Tries to discover all possible plugins based on the provided plugin config.
- * Discovery result consists of two separate streams, the one (`plugin$`) is
- * for the successfully discovered plugins and the other one (`error$`) is for
- * all the errors that occurred during discovery process.
- * @param log Plugin discovery logger instance.
- * @param config Plugin config instance.
- */
-export function discover(log: Logger, config: PluginsConfig) {
-  log.debug('Discovering plugins...');
-
-  const discoveryResults$ = merge(
-    processScanDirs$(log, config.scanDirs),
-    processPaths$(log, config.paths)
-  ).pipe(
-    mergeMap(pluginPathOrError => {
-      return typeof pluginPathOrError === 'string'
-        ? createPlugin$(log, pluginPathOrError)
-        : [pluginPathOrError];
-    }),
-    shareReplay()
-  );
-
-  return {
-    plugin$: discoveryResults$.pipe(
-      mergeMap(entry => (entry.plugin !== undefined ? [entry.plugin] : []))
-    ),
-    error$: discoveryResults$.pipe(
-      mergeMap(entry => (entry.error !== undefined ? [entry.error] : []))
-    ),
-  };
-}
-
-/**
- * Iterates over every entry in `scanDirs` and returns a merged stream of all
- * sub-directories. If directory cannot be read or it's impossible to get stat
- * for any of the nested entries then error is added into the stream instead.
- * @param log Plugin discovery logger instance.
- * @param scanDirs List of the top-level directories to process.
- */
-function processScanDirs$(log: Logger, scanDirs: string[]) {
-  return from(scanDirs).pipe(
-    mergeMap(dir => {
-      log.debug(`Scanning "${dir}" for plugin sub-directories...`);
-
-      return fsReadDir$(dir).pipe(
-        mergeMap(subDirs => subDirs.map(subDir => resolve(dir, subDir))),
-        mergeMap(path =>
-          fsStat$(path).pipe(
-            // Filter out non-directory entries from target directories, it's expected that
-            // these directories may contain files (e.g. `README.md` or `package.json`).
-            // We shouldn't silently ignore the entries we couldn't get stat for though.
-            mergeMap(pathStat => (pathStat.isDirectory() ? [path] : [])),
-            catchError(err => [
-              wrapError(PluginDiscoveryErrorType.InvalidPluginDirectory, path, err),
-            ])
-          )
-        ),
-        catchError(err => [wrapError(PluginDiscoveryErrorType.InvalidScanDirectory, dir, err)])
-      );
-    })
-  );
-}
-
-/**
- * Iterates over every entry in `paths` and returns a stream of all paths that
- * are directories. If path is not a directory or it's impossible to get stat
- * for this path then error is added into the stream instead.
- * @param log Plugin discovery logger instance.
- * @param paths List of paths to process.
- */
-function processPaths$(log: Logger, paths: string[]) {
-  return from(paths).pipe(
-    mergeMap(path => {
-      log.debug(`Including "${path}" into the plugin path list.`);
-
-      return fsStat$(path).pipe(
-        // Since every path is specifically provided we should treat non-directory
-        // entries as mistakes we should report of.
-        mergeMap(pathStat => {
-          return pathStat.isDirectory()
-            ? [path]
-            : throwError(new Error(`${path} is not a directory.`));
-        }),
-        catchError(err => [wrapError(PluginDiscoveryErrorType.InvalidPluginDirectory, path, err)])
-      );
-    })
-  );
-}
-
-/**
- * Tries to load and parse the plugin manifest file located at the provided plugin
- * directory path and produces an error result if it fails to do so or plugin manifest
- * isn't valid.
- * @param log Plugin discovery logger instance.
- * @param path Path to the plugin directory where manifest should be loaded from.
- */
-function createPlugin$(log: Logger, path: string) {
-  const manifestPath = resolve(path, MANIFEST_FILE_NAME);
-  return fsReadFile$(manifestPath).pipe(
-    mergeMap(manifestContent => {
-      try {
-        const plugin = { path, manifest: parseManifest(manifestContent) };
-
-        log.debug(`Successfully discovered plugin "${plugin.manifest.id}" at "${path}"`);
-
-        return [{ plugin }];
-      } catch (err) {
-        return [wrapError(PluginDiscoveryErrorType.InvalidManifest, manifestPath, err)];
-      }
-    }),
-    catchError(err => [wrapError(PluginDiscoveryErrorType.MissingManifest, manifestPath, err)])
-  );
-}
-
-/**
- * Parses raw buffer content into plugin manifest with the preliminary checks.
- * @param rawManifest Buffer containing plugin manifest JSON.
- */
-function parseManifest(rawManifest: Buffer): PluginManifest {
-  const manifest: Partial<PluginManifest> = JSON.parse(rawManifest.toString());
-  if (!manifest || typeof manifest !== 'object') {
-    throw new Error('Plugin manifest must contain a JSON encoded object.');
-  }
-
-  if (!manifest.id || typeof manifest.id !== 'string') {
-    throw new Error('Plugin manifest must contain an "id" property.');
-  }
-
-  if (!manifest.version || typeof manifest.version !== 'string') {
-    throw new Error(`Plugin manifest for "${manifest.id}" must contain a "version" property.`);
-  }
-
-  return {
-    id: manifest.id,
-    version: manifest.version,
-    kibanaVersion:
-      typeof manifest.kibanaVersion === 'string' && manifest.kibanaVersion
-        ? manifest.kibanaVersion
-        : manifest.version,
-    requiredPlugins: Array.isArray(manifest.requiredPlugins) ? manifest.requiredPlugins : [],
-    optionalPlugins: Array.isArray(manifest.optionalPlugins) ? manifest.optionalPlugins : [],
-    ui: typeof manifest.ui === 'boolean' ? manifest.ui : false,
-  };
-}
-
-/**
- * Wraps any error instance into `PluginDiscoveryError` tagging it with specific
- * type that can be used later to distinguish between different types of errors
- * that can occur during plugin discovery process.
- * @param type Type of the discovery error (invalid directory, invalid manifest etc.)
- * @param path Path at which discovery error occurred.
- * @param error Instance of the "raw" error that occurred during discovery.
- */
-function wrapError(type: PluginDiscoveryErrorType, path: string, error: Error): DiscoveryResult {
-  return { error: new PluginDiscoveryError(type, path, error) };
-}
diff --git a/src/core/server/plugins/plugins_service.ts b/src/core/server/plugins/plugins_service.ts
index b29715dadc8dc..836107ba0d0a4 100644
--- a/src/core/server/plugins/plugins_service.ts
+++ b/src/core/server/plugins/plugins_service.ts
@@ -19,16 +19,16 @@
 
 import { filter, first, map, tap, toArray } from 'rxjs/operators';
 import { CoreService } from '../../types/core_service';
-import { ConfigService } from '../config';
+import { ConfigService, Env } from '../config';
 import { Logger, LoggerFactory } from '../logging';
-import { PluginDiscoveryErrorType } from './plugin_discovery_error';
+import { discover, PluginDiscoveryErrorType } from './discovery';
 import { PluginsConfig } from './plugins_config';
-import { discover } from './plugins_discovery';
 
 export class PluginsService implements CoreService {
   private readonly log: Logger;
 
   constructor(
+    private readonly env: Env,
     private readonly logger: LoggerFactory,
     private readonly configService: ConfigService
   ) {
@@ -42,7 +42,9 @@ export class PluginsService implements CoreService {
       .atPath('plugins', PluginsConfig)
       .pipe(
         first(),
-        map(config => discover(this.logger.get('plugins', 'discovery'), config))
+        map(config =>
+          discover(config, this.env.packageInfo, this.logger.get('plugins', 'discovery'))
+        )
       )
       .toPromise();
 

From c361f81188fc4c93725b19d397fd981c5b68658e Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Mon, 5 Nov 2018 17:30:51 +0100
Subject: [PATCH 12/15] Fix unit test.

---
 src/core/server/plugins/discovery/plugin_discovery.test.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/core/server/plugins/discovery/plugin_discovery.test.ts b/src/core/server/plugins/discovery/plugin_discovery.test.ts
index ba22b359c7acb..82e8dafda39f4 100644
--- a/src/core/server/plugins/discovery/plugin_discovery.test.ts
+++ b/src/core/server/plugins/discovery/plugin_discovery.test.ts
@@ -364,7 +364,7 @@ Array [
       "ui": false,
       "version": "7.0.0",
     },
-    "path": "/projects/elastic/master/kibana/path/existent-dir",
+    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir)}",
   },
 ]
 `);

From 82e470bb929f58a4c4a7f5cb5da7a206fdeb2815 Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Tue, 6 Nov 2018 11:34:41 +0100
Subject: [PATCH 13/15] =?UTF-8?q?Review=20=E2=84=963:=20use=20`toEqual`=20?=
 =?UTF-8?q?instead=20of=20`toMatchInlineSnapshot`=20for=20the=20tests=20th?=
 =?UTF-8?q?at=20involve=20paths.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../discovery/plugin_discovery.test.ts        | 361 +++++++-----------
 1 file changed, 140 insertions(+), 221 deletions(-)

diff --git a/src/core/server/plugins/discovery/plugin_discovery.test.ts b/src/core/server/plugins/discovery/plugin_discovery.test.ts
index 82e8dafda39f4..9ee8ed3f1b7b2 100644
--- a/src/core/server/plugins/discovery/plugin_discovery.test.ts
+++ b/src/core/server/plugins/discovery/plugin_discovery.test.ts
@@ -33,14 +33,6 @@ import { logger } from '../../logging/__mocks__';
 import { PluginManifest } from './plugin_manifest_parser';
 import { discover } from './plugins_discovery';
 
-/**
- * Resolves absolute path and escapes backslashes (used on windows systems).
- * @param pathSegments Test path segments.
- */
-function resolveForSnapshot(...pathSegments: string[]) {
-  return resolve(...pathSegments).replace(/\\/g, '\\\\');
-}
-
 const TEST_PATHS = {
   scanDirs: {
     nonEmpty: resolve('scan', 'non-empty'),
@@ -112,65 +104,25 @@ test('properly scans folders and paths', async () => {
     logger.get()
   );
 
-  await expect(plugin$.pipe(toArray()).toPromise()).resolves.toMatchInlineSnapshot(`
-Array [
-  Object {
-    "manifest": Object {
-      "id": "plugin",
-      "kibanaVersion": "1.2.3",
-      "optionalPlugins": Array [],
-      "requiredPlugins": Array [],
-      "ui": false,
-      "version": "1",
-    },
-    "path": "${resolveForSnapshot(TEST_PATHS.scanDirs.nonEmpty, '1')}",
-  },
-  Object {
-    "manifest": Object {
-      "id": "plugin",
-      "kibanaVersion": "1.2.3",
-      "optionalPlugins": Array [],
-      "requiredPlugins": Array [],
-      "ui": false,
-      "version": "1",
-    },
-    "path": "${resolveForSnapshot(TEST_PATHS.scanDirs.nonEmpty, '3')}",
-  },
-  Object {
-    "manifest": Object {
-      "id": "plugin",
-      "kibanaVersion": "1.2.3",
-      "optionalPlugins": Array [],
-      "requiredPlugins": Array [],
-      "ui": false,
-      "version": "1",
-    },
-    "path": "${resolveForSnapshot(TEST_PATHS.scanDirs.nonEmpty2, '6')}",
-  },
-  Object {
-    "manifest": Object {
-      "id": "plugin",
-      "kibanaVersion": "1.2.3",
-      "optionalPlugins": Array [],
-      "requiredPlugins": Array [],
-      "ui": false,
-      "version": "1",
-    },
-    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir)}",
-  },
-  Object {
-    "manifest": Object {
-      "id": "plugin",
-      "kibanaVersion": "1.2.3",
-      "optionalPlugins": Array [],
-      "requiredPlugins": Array [],
-      "ui": false,
-      "version": "1",
-    },
-    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir2)}",
-  },
-]
-`);
+  await expect(plugin$.pipe(toArray()).toPromise()).resolves.toEqual(
+    [
+      resolve(TEST_PATHS.scanDirs.nonEmpty, '1'),
+      resolve(TEST_PATHS.scanDirs.nonEmpty, '3'),
+      resolve(TEST_PATHS.scanDirs.nonEmpty2, '6'),
+      resolve(TEST_PATHS.paths.existentDir),
+      resolve(TEST_PATHS.paths.existentDir2),
+    ].map(path => ({
+      manifest: {
+        id: 'plugin',
+        version: '1',
+        kibanaVersion: '1.2.3',
+        optionalPlugins: [],
+        requiredPlugins: [],
+        ui: false,
+      },
+      path,
+    }))
+  );
 
   await expect(
     error$
@@ -179,35 +131,33 @@ Array [
         toArray()
       )
       .toPromise()
-  ).resolves.toMatchInlineSnapshot(`
-Array [
-  "Error: ENOENT (missing-manifest, ${resolveForSnapshot(
-    TEST_PATHS.scanDirs.nonEmpty,
-    '2-no-manifest',
-    'kibana.json'
-  )})",
-  "Error: Plugin manifest must contain an \\"id\\" property. (invalid-manifest, ${resolveForSnapshot(
-    TEST_PATHS.scanDirs.nonEmpty,
-    '4-incomplete-manifest',
-    'kibana.json'
-  )})",
-  "Error: Unexpected token o in JSON at position 1 (invalid-manifest, ${resolveForSnapshot(
-    TEST_PATHS.scanDirs.nonEmpty2,
-    '5-invalid-manifest',
-    'kibana.json'
-  )})",
-  "Error: Plugin \\"plugin\\" is only compatible with Kibana version \\"1\\", but used Kibana version is \\"1.2.3\\". (incompatible-version, ${resolveForSnapshot(
-    TEST_PATHS.scanDirs.nonEmpty2,
-    '8-incompatible-manifest',
-    'kibana.json'
-  )})",
-  "Error: ENOENT (invalid-scan-dir, ${resolveForSnapshot(TEST_PATHS.scanDirs.nonExistent)})",
-  "Error: ${resolveForSnapshot(
-    TEST_PATHS.paths.nonDir
-  )} is not a directory. (invalid-plugin-dir, ${resolveForSnapshot(TEST_PATHS.paths.nonDir)})",
-  "Error: ENOENT (invalid-plugin-dir, ${resolveForSnapshot(TEST_PATHS.paths.nonExistent)})",
-]
-`);
+  ).resolves.toEqual([
+    `Error: ENOENT (missing-manifest, ${resolve(
+      TEST_PATHS.scanDirs.nonEmpty,
+      '2-no-manifest',
+      'kibana.json'
+    )})`,
+    `Error: Plugin manifest must contain an "id" property. (invalid-manifest, ${resolve(
+      TEST_PATHS.scanDirs.nonEmpty,
+      '4-incomplete-manifest',
+      'kibana.json'
+    )})`,
+    `Error: Unexpected token o in JSON at position 1 (invalid-manifest, ${resolve(
+      TEST_PATHS.scanDirs.nonEmpty2,
+      '5-invalid-manifest',
+      'kibana.json'
+    )})`,
+    `Error: Plugin "plugin" is only compatible with Kibana version "1", but used Kibana version is "1.2.3". (incompatible-version, ${resolve(
+      TEST_PATHS.scanDirs.nonEmpty2,
+      '8-incompatible-manifest',
+      'kibana.json'
+    )})`,
+    `Error: ENOENT (invalid-scan-dir, ${resolve(TEST_PATHS.scanDirs.nonExistent)})`,
+    `Error: ${resolve(TEST_PATHS.paths.nonDir)} is not a directory. (invalid-plugin-dir, ${resolve(
+      TEST_PATHS.paths.nonDir
+    )})`,
+    `Error: ENOENT (invalid-plugin-dir, ${resolve(TEST_PATHS.paths.nonExistent)})`,
+  ]);
 });
 
 describe('parsing plugin manifest', () => {
@@ -242,14 +192,12 @@ describe('parsing plugin manifest', () => {
     });
 
     await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
-Array [
-  "Error: Unexpected end of JSON input (invalid-manifest, ${resolveForSnapshot(
-    TEST_PATHS.paths.existentDir,
-    'kibana.json'
-  )})",
-]
-`);
+    await expect(errors$.toPromise()).resolves.toEqual([
+      `Error: Unexpected end of JSON input (invalid-manifest, ${resolve(
+        TEST_PATHS.paths.existentDir,
+        'kibana.json'
+      )})`,
+    ]);
   });
 
   test('return error when manifest content is null', async () => {
@@ -258,14 +206,12 @@ Array [
     });
 
     await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
-Array [
-  "Error: Plugin manifest must contain a JSON encoded object. (invalid-manifest, ${resolveForSnapshot(
-    TEST_PATHS.paths.existentDir,
-    'kibana.json'
-  )})",
-]
-`);
+    await expect(errors$.toPromise()).resolves.toEqual([
+      `Error: Plugin manifest must contain a JSON encoded object. (invalid-manifest, ${resolve(
+        TEST_PATHS.paths.existentDir,
+        'kibana.json'
+      )})`,
+    ]);
   });
 
   test('return error when manifest content is not a valid JSON', async () => {
@@ -274,14 +220,12 @@ Array [
     });
 
     await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
-Array [
-  "Error: Unexpected token o in JSON at position 1 (invalid-manifest, ${resolveForSnapshot(
-    TEST_PATHS.paths.existentDir,
-    'kibana.json'
-  )})",
-]
-`);
+    await expect(errors$.toPromise()).resolves.toEqual([
+      `Error: Unexpected token o in JSON at position 1 (invalid-manifest, ${resolve(
+        TEST_PATHS.paths.existentDir,
+        'kibana.json'
+      )})`,
+    ]);
   });
 
   test('return error when plugin id is missing', async () => {
@@ -290,14 +234,12 @@ Array [
     });
 
     await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
-Array [
-  "Error: Plugin manifest must contain an \\"id\\" property. (invalid-manifest, ${resolveForSnapshot(
-    TEST_PATHS.paths.existentDir,
-    'kibana.json'
-  )})",
-]
-`);
+    await expect(errors$.toPromise()).resolves.toEqual([
+      `Error: Plugin manifest must contain an "id" property. (invalid-manifest, ${resolve(
+        TEST_PATHS.paths.existentDir,
+        'kibana.json'
+      )})`,
+    ]);
   });
 
   test('return error when plugin version is missing', async () => {
@@ -306,14 +248,12 @@ Array [
     });
 
     await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
-Array [
-  "Error: Plugin manifest for \\"some-id\\" must contain a \\"version\\" property. (invalid-manifest, ${resolveForSnapshot(
-    TEST_PATHS.paths.existentDir,
-    'kibana.json'
-  )})",
-]
-`);
+    await expect(errors$.toPromise()).resolves.toEqual([
+      `Error: Plugin manifest for "some-id" must contain a "version" property. (invalid-manifest, ${resolve(
+        TEST_PATHS.paths.existentDir,
+        'kibana.json'
+      )})`,
+    ]);
   });
 
   test('return error when plugin expected Kibana version is lower than actual version', async () => {
@@ -322,14 +262,12 @@ Array [
     });
 
     await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
-Array [
-  "Error: Plugin \\"some-id\\" is only compatible with Kibana version \\"6.4.2\\", but used Kibana version is \\"7.0.0-alpha1\\". (incompatible-version, ${resolveForSnapshot(
-    TEST_PATHS.paths.existentDir,
-    'kibana.json'
-  )})",
-]
-`);
+    await expect(errors$.toPromise()).resolves.toEqual([
+      `Error: Plugin "some-id" is only compatible with Kibana version "6.4.2", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${resolve(
+        TEST_PATHS.paths.existentDir,
+        'kibana.json'
+      )})`,
+    ]);
   });
 
   test('return error when plugin expected Kibana version is higher than actual version', async () => {
@@ -338,14 +276,12 @@ Array [
     });
 
     await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toMatchInlineSnapshot(`
-Array [
-  "Error: Plugin \\"some-id\\" is only compatible with Kibana version \\"7.0.1\\", but used Kibana version is \\"7.0.0-alpha1\\". (incompatible-version, ${resolveForSnapshot(
-    TEST_PATHS.paths.existentDir,
-    'kibana.json'
-  )})",
-]
-`);
+    await expect(errors$.toPromise()).resolves.toEqual([
+      `Error: Plugin "some-id" is only compatible with Kibana version "7.0.1", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${resolve(
+        TEST_PATHS.paths.existentDir,
+        'kibana.json'
+      )})`,
+    ]);
   });
 
   test('set defaults for all missing optional fields', async () => {
@@ -353,21 +289,19 @@ Array [
       cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '7.0.0' })));
     });
 
-    await expect(plugins$.toPromise()).resolves.toMatchInlineSnapshot(`
-Array [
-  Object {
-    "manifest": Object {
-      "id": "some-id",
-      "kibanaVersion": "7.0.0",
-      "optionalPlugins": Array [],
-      "requiredPlugins": Array [],
-      "ui": false,
-      "version": "7.0.0",
-    },
-    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir)}",
-  },
-]
-`);
+    await expect(plugins$.toPromise()).resolves.toEqual([
+      {
+        manifest: {
+          id: 'some-id',
+          version: '7.0.0',
+          kibanaVersion: '7.0.0',
+          optionalPlugins: [],
+          requiredPlugins: [],
+          ui: false,
+        },
+        path: resolve(TEST_PATHS.paths.existentDir),
+      },
+    ]);
     await expect(errors$.toPromise()).resolves.toEqual([]);
   });
 
@@ -388,26 +322,19 @@ Array [
       );
     });
 
-    await expect(plugins$.toPromise()).resolves.toMatchInlineSnapshot(`
-Array [
-  Object {
-    "manifest": Object {
-      "id": "some-id",
-      "kibanaVersion": "7.0.0",
-      "optionalPlugins": Array [
-        "some-optional-plugin",
-      ],
-      "requiredPlugins": Array [
-        "some-required-plugin",
-        "some-required-plugin-2",
-      ],
-      "ui": true,
-      "version": "some-version",
-    },
-    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir)}",
-  },
-]
-`);
+    await expect(plugins$.toPromise()).resolves.toEqual([
+      {
+        manifest: {
+          id: 'some-id',
+          version: 'some-version',
+          kibanaVersion: '7.0.0',
+          optionalPlugins: ['some-optional-plugin'],
+          requiredPlugins: ['some-required-plugin', 'some-required-plugin-2'],
+          ui: true,
+        },
+        path: resolve(TEST_PATHS.paths.existentDir),
+      },
+    ]);
     await expect(errors$.toPromise()).resolves.toEqual([]);
   });
 
@@ -426,23 +353,19 @@ Array [
       );
     });
 
-    await expect(plugins$.toPromise()).resolves.toMatchInlineSnapshot(`
-Array [
-  Object {
-    "manifest": Object {
-      "id": "some-id",
-      "kibanaVersion": "7.0.0-alpha2",
-      "optionalPlugins": Array [],
-      "requiredPlugins": Array [
-        "some-required-plugin",
-      ],
-      "ui": false,
-      "version": "some-version",
-    },
-    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir)}",
-  },
-]
-`);
+    await expect(plugins$.toPromise()).resolves.toEqual([
+      {
+        manifest: {
+          id: 'some-id',
+          version: 'some-version',
+          kibanaVersion: '7.0.0-alpha2',
+          optionalPlugins: [],
+          requiredPlugins: ['some-required-plugin'],
+          ui: false,
+        },
+        path: resolve(TEST_PATHS.paths.existentDir),
+      },
+    ]);
     await expect(errors$.toPromise()).resolves.toEqual([]);
   });
 
@@ -461,23 +384,19 @@ Array [
       );
     });
 
-    await expect(plugins$.toPromise()).resolves.toMatchInlineSnapshot(`
-Array [
-  Object {
-    "manifest": Object {
-      "id": "some-id",
-      "kibanaVersion": "kibana",
-      "optionalPlugins": Array [],
-      "requiredPlugins": Array [
-        "some-required-plugin",
-      ],
-      "ui": false,
-      "version": "some-version",
-    },
-    "path": "${resolveForSnapshot(TEST_PATHS.paths.existentDir)}",
-  },
-]
-`);
+    await expect(plugins$.toPromise()).resolves.toEqual([
+      {
+        manifest: {
+          id: 'some-id',
+          version: 'some-version',
+          kibanaVersion: 'kibana',
+          optionalPlugins: [],
+          requiredPlugins: ['some-required-plugin'],
+          ui: false,
+        },
+        path: resolve(TEST_PATHS.paths.existentDir),
+      },
+    ]);
     await expect(errors$.toPromise()).resolves.toEqual([]);
   });
 });

From 52290ee9fb7488540ea95c0e67907993d40b73b3 Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Tue, 6 Nov 2018 12:09:49 +0100
Subject: [PATCH 14/15] =?UTF-8?q?Review=20=E2=84=963:=20move=20manifest=20?=
 =?UTF-8?q?parser=20tests=20to=20a=20separate=20file=20+=20switch=20manife?=
 =?UTF-8?q?st=20parsing=20to=20promise=20based=20signature.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../discovery/plugin_discovery.test.ts        | 253 +-----------------
 .../discovery/plugin_manifest_parser.test.ts  | 217 +++++++++++++++
 .../discovery/plugin_manifest_parser.ts       | 126 ++++-----
 .../plugins/discovery/plugins_discovery.ts    |   4 +-
 4 files changed, 288 insertions(+), 312 deletions(-)
 create mode 100644 src/core/server/plugins/discovery/plugin_manifest_parser.test.ts

diff --git a/src/core/server/plugins/discovery/plugin_discovery.test.ts b/src/core/server/plugins/discovery/plugin_discovery.test.ts
index 9ee8ed3f1b7b2..d1e4f22c495fc 100644
--- a/src/core/server/plugins/discovery/plugin_discovery.test.ts
+++ b/src/core/server/plugins/discovery/plugin_discovery.test.ts
@@ -27,10 +27,8 @@ jest.mock('fs', () => ({
 }));
 
 import { resolve } from 'path';
-import { Observable } from 'rxjs';
 import { map, toArray } from 'rxjs/operators';
 import { logger } from '../../logging/__mocks__';
-import { PluginManifest } from './plugin_manifest_parser';
 import { discover } from './plugins_discovery';
 
 const TEST_PATHS = {
@@ -132,6 +130,11 @@ test('properly scans folders and paths', async () => {
       )
       .toPromise()
   ).resolves.toEqual([
+    `Error: ENOENT (invalid-scan-dir, ${resolve(TEST_PATHS.scanDirs.nonExistent)})`,
+    `Error: ${resolve(TEST_PATHS.paths.nonDir)} is not a directory. (invalid-plugin-dir, ${resolve(
+      TEST_PATHS.paths.nonDir
+    )})`,
+    `Error: ENOENT (invalid-plugin-dir, ${resolve(TEST_PATHS.paths.nonExistent)})`,
     `Error: ENOENT (missing-manifest, ${resolve(
       TEST_PATHS.scanDirs.nonEmpty,
       '2-no-manifest',
@@ -152,251 +155,5 @@ test('properly scans folders and paths', async () => {
       '8-incompatible-manifest',
       'kibana.json'
     )})`,
-    `Error: ENOENT (invalid-scan-dir, ${resolve(TEST_PATHS.scanDirs.nonExistent)})`,
-    `Error: ${resolve(TEST_PATHS.paths.nonDir)} is not a directory. (invalid-plugin-dir, ${resolve(
-      TEST_PATHS.paths.nonDir
-    )})`,
-    `Error: ENOENT (invalid-plugin-dir, ${resolve(TEST_PATHS.paths.nonExistent)})`,
   ]);
 });
-
-describe('parsing plugin manifest', () => {
-  let plugins$: Observable<Array<{ path: string; manifest: PluginManifest }>>;
-  let errors$: Observable<string[]>;
-  beforeEach(async () => {
-    const discoveryResult = discover(
-      {
-        initialize: true,
-        scanDirs: [],
-        paths: [TEST_PATHS.paths.existentDir],
-      },
-      {
-        branch: 'master',
-        buildNum: 1,
-        buildSha: '',
-        version: '7.0.0-alpha1',
-      },
-      logger.get()
-    );
-
-    plugins$ = discoveryResult.plugin$.pipe(toArray());
-    errors$ = discoveryResult.error$.pipe(
-      map(error => error.toString()),
-      toArray()
-    );
-  });
-
-  test('return error when manifest is empty', async () => {
-    mockReadFile.mockImplementation((path, cb) => {
-      cb(null, Buffer.from(''));
-    });
-
-    await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toEqual([
-      `Error: Unexpected end of JSON input (invalid-manifest, ${resolve(
-        TEST_PATHS.paths.existentDir,
-        'kibana.json'
-      )})`,
-    ]);
-  });
-
-  test('return error when manifest content is null', async () => {
-    mockReadFile.mockImplementation((path, cb) => {
-      cb(null, Buffer.from('null'));
-    });
-
-    await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toEqual([
-      `Error: Plugin manifest must contain a JSON encoded object. (invalid-manifest, ${resolve(
-        TEST_PATHS.paths.existentDir,
-        'kibana.json'
-      )})`,
-    ]);
-  });
-
-  test('return error when manifest content is not a valid JSON', async () => {
-    mockReadFile.mockImplementation((path, cb) => {
-      cb(null, Buffer.from('not-json'));
-    });
-
-    await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toEqual([
-      `Error: Unexpected token o in JSON at position 1 (invalid-manifest, ${resolve(
-        TEST_PATHS.paths.existentDir,
-        'kibana.json'
-      )})`,
-    ]);
-  });
-
-  test('return error when plugin id is missing', async () => {
-    mockReadFile.mockImplementation((path, cb) => {
-      cb(null, Buffer.from(JSON.stringify({ version: 'some-version' })));
-    });
-
-    await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toEqual([
-      `Error: Plugin manifest must contain an "id" property. (invalid-manifest, ${resolve(
-        TEST_PATHS.paths.existentDir,
-        'kibana.json'
-      )})`,
-    ]);
-  });
-
-  test('return error when plugin version is missing', async () => {
-    mockReadFile.mockImplementation((path, cb) => {
-      cb(null, Buffer.from(JSON.stringify({ id: 'some-id' })));
-    });
-
-    await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toEqual([
-      `Error: Plugin manifest for "some-id" must contain a "version" property. (invalid-manifest, ${resolve(
-        TEST_PATHS.paths.existentDir,
-        'kibana.json'
-      )})`,
-    ]);
-  });
-
-  test('return error when plugin expected Kibana version is lower than actual version', async () => {
-    mockReadFile.mockImplementation((path, cb) => {
-      cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '6.4.2' })));
-    });
-
-    await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toEqual([
-      `Error: Plugin "some-id" is only compatible with Kibana version "6.4.2", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${resolve(
-        TEST_PATHS.paths.existentDir,
-        'kibana.json'
-      )})`,
-    ]);
-  });
-
-  test('return error when plugin expected Kibana version is higher than actual version', async () => {
-    mockReadFile.mockImplementation((path, cb) => {
-      cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '7.0.1' })));
-    });
-
-    await expect(plugins$.toPromise()).resolves.toEqual([]);
-    await expect(errors$.toPromise()).resolves.toEqual([
-      `Error: Plugin "some-id" is only compatible with Kibana version "7.0.1", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${resolve(
-        TEST_PATHS.paths.existentDir,
-        'kibana.json'
-      )})`,
-    ]);
-  });
-
-  test('set defaults for all missing optional fields', async () => {
-    mockReadFile.mockImplementation((path, cb) => {
-      cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '7.0.0' })));
-    });
-
-    await expect(plugins$.toPromise()).resolves.toEqual([
-      {
-        manifest: {
-          id: 'some-id',
-          version: '7.0.0',
-          kibanaVersion: '7.0.0',
-          optionalPlugins: [],
-          requiredPlugins: [],
-          ui: false,
-        },
-        path: resolve(TEST_PATHS.paths.existentDir),
-      },
-    ]);
-    await expect(errors$.toPromise()).resolves.toEqual([]);
-  });
-
-  test('return all set optional fields as they are in manifest', async () => {
-    mockReadFile.mockImplementation((path, cb) => {
-      cb(
-        null,
-        Buffer.from(
-          JSON.stringify({
-            id: 'some-id',
-            version: 'some-version',
-            kibanaVersion: '7.0.0',
-            requiredPlugins: ['some-required-plugin', 'some-required-plugin-2'],
-            optionalPlugins: ['some-optional-plugin'],
-            ui: true,
-          })
-        )
-      );
-    });
-
-    await expect(plugins$.toPromise()).resolves.toEqual([
-      {
-        manifest: {
-          id: 'some-id',
-          version: 'some-version',
-          kibanaVersion: '7.0.0',
-          optionalPlugins: ['some-optional-plugin'],
-          requiredPlugins: ['some-required-plugin', 'some-required-plugin-2'],
-          ui: true,
-        },
-        path: resolve(TEST_PATHS.paths.existentDir),
-      },
-    ]);
-    await expect(errors$.toPromise()).resolves.toEqual([]);
-  });
-
-  test('return manifest when plugin expected Kibana version matches actual version', async () => {
-    mockReadFile.mockImplementation((path, cb) => {
-      cb(
-        null,
-        Buffer.from(
-          JSON.stringify({
-            id: 'some-id',
-            version: 'some-version',
-            kibanaVersion: '7.0.0-alpha2',
-            requiredPlugins: ['some-required-plugin'],
-          })
-        )
-      );
-    });
-
-    await expect(plugins$.toPromise()).resolves.toEqual([
-      {
-        manifest: {
-          id: 'some-id',
-          version: 'some-version',
-          kibanaVersion: '7.0.0-alpha2',
-          optionalPlugins: [],
-          requiredPlugins: ['some-required-plugin'],
-          ui: false,
-        },
-        path: resolve(TEST_PATHS.paths.existentDir),
-      },
-    ]);
-    await expect(errors$.toPromise()).resolves.toEqual([]);
-  });
-
-  test('return manifest when plugin expected Kibana version is `kibana`', async () => {
-    mockReadFile.mockImplementation((path, cb) => {
-      cb(
-        null,
-        Buffer.from(
-          JSON.stringify({
-            id: 'some-id',
-            version: 'some-version',
-            kibanaVersion: 'kibana',
-            requiredPlugins: ['some-required-plugin'],
-          })
-        )
-      );
-    });
-
-    await expect(plugins$.toPromise()).resolves.toEqual([
-      {
-        manifest: {
-          id: 'some-id',
-          version: 'some-version',
-          kibanaVersion: 'kibana',
-          optionalPlugins: [],
-          requiredPlugins: ['some-required-plugin'],
-          ui: false,
-        },
-        path: resolve(TEST_PATHS.paths.existentDir),
-      },
-    ]);
-    await expect(errors$.toPromise()).resolves.toEqual([]);
-  });
-});
diff --git a/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts b/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts
new file mode 100644
index 0000000000000..ada52c17e11ee
--- /dev/null
+++ b/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts
@@ -0,0 +1,217 @@
+/*
+ * 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 { PluginDiscoveryErrorType } from './plugin_discovery_error';
+
+const mockReadFile = jest.fn();
+jest.mock('fs', () => ({ readFile: mockReadFile }));
+
+import { resolve } from 'path';
+import { parseManifest } from './plugin_manifest_parser';
+
+const pluginPath = resolve('path', 'existent-dir');
+const pluginManifestPath = resolve(pluginPath, 'kibana.json');
+const packageInfo = {
+  branch: 'master',
+  buildNum: 1,
+  buildSha: '',
+  version: '7.0.0-alpha1',
+};
+
+afterEach(() => {
+  jest.clearAllMocks();
+});
+
+test('return error when manifest is empty', async () => {
+  mockReadFile.mockImplementation((path, cb) => {
+    cb(null, Buffer.from(''));
+  });
+
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
+    message: 'Unexpected end of JSON input',
+    type: PluginDiscoveryErrorType.InvalidManifest,
+    path: pluginManifestPath,
+  });
+});
+
+test('return error when manifest content is null', async () => {
+  mockReadFile.mockImplementation((path, cb) => {
+    cb(null, Buffer.from('null'));
+  });
+
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
+    message: 'Plugin manifest must contain a JSON encoded object.',
+    type: PluginDiscoveryErrorType.InvalidManifest,
+    path: pluginManifestPath,
+  });
+});
+
+test('return error when manifest content is not a valid JSON', async () => {
+  mockReadFile.mockImplementation((path, cb) => {
+    cb(null, Buffer.from('not-json'));
+  });
+
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
+    message: 'Unexpected token o in JSON at position 1',
+    type: PluginDiscoveryErrorType.InvalidManifest,
+    path: pluginManifestPath,
+  });
+});
+
+test('return error when plugin id is missing', async () => {
+  mockReadFile.mockImplementation((path, cb) => {
+    cb(null, Buffer.from(JSON.stringify({ version: 'some-version' })));
+  });
+
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
+    message: 'Plugin manifest must contain an "id" property.',
+    type: PluginDiscoveryErrorType.InvalidManifest,
+    path: pluginManifestPath,
+  });
+});
+
+test('return error when plugin version is missing', async () => {
+  mockReadFile.mockImplementation((path, cb) => {
+    cb(null, Buffer.from(JSON.stringify({ id: 'some-id' })));
+  });
+
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
+    message: 'Plugin manifest for "some-id" must contain a "version" property.',
+    type: PluginDiscoveryErrorType.InvalidManifest,
+    path: pluginManifestPath,
+  });
+});
+
+test('return error when plugin expected Kibana version is lower than actual version', async () => {
+  mockReadFile.mockImplementation((path, cb) => {
+    cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '6.4.2' })));
+  });
+
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
+    message:
+      'Plugin "some-id" is only compatible with Kibana version "6.4.2", but used Kibana version is "7.0.0-alpha1".',
+    type: PluginDiscoveryErrorType.IncompatibleVersion,
+    path: pluginManifestPath,
+  });
+});
+
+test('return error when plugin expected Kibana version is higher than actual version', async () => {
+  mockReadFile.mockImplementation((path, cb) => {
+    cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '7.0.1' })));
+  });
+
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
+    message:
+      'Plugin "some-id" is only compatible with Kibana version "7.0.1", but used Kibana version is "7.0.0-alpha1".',
+    type: PluginDiscoveryErrorType.IncompatibleVersion,
+    path: pluginManifestPath,
+  });
+});
+
+test('set defaults for all missing optional fields', async () => {
+  mockReadFile.mockImplementation((path, cb) => {
+    cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '7.0.0' })));
+  });
+
+  await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({
+    id: 'some-id',
+    version: '7.0.0',
+    kibanaVersion: '7.0.0',
+    optionalPlugins: [],
+    requiredPlugins: [],
+    ui: false,
+  });
+});
+
+test('return all set optional fields as they are in manifest', async () => {
+  mockReadFile.mockImplementation((path, cb) => {
+    cb(
+      null,
+      Buffer.from(
+        JSON.stringify({
+          id: 'some-id',
+          version: 'some-version',
+          kibanaVersion: '7.0.0',
+          requiredPlugins: ['some-required-plugin', 'some-required-plugin-2'],
+          optionalPlugins: ['some-optional-plugin'],
+          ui: true,
+        })
+      )
+    );
+  });
+
+  await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({
+    id: 'some-id',
+    version: 'some-version',
+    kibanaVersion: '7.0.0',
+    optionalPlugins: ['some-optional-plugin'],
+    requiredPlugins: ['some-required-plugin', 'some-required-plugin-2'],
+    ui: true,
+  });
+});
+
+test('return manifest when plugin expected Kibana version matches actual version', async () => {
+  mockReadFile.mockImplementation((path, cb) => {
+    cb(
+      null,
+      Buffer.from(
+        JSON.stringify({
+          id: 'some-id',
+          version: 'some-version',
+          kibanaVersion: '7.0.0-alpha2',
+          requiredPlugins: ['some-required-plugin'],
+        })
+      )
+    );
+  });
+
+  await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({
+    id: 'some-id',
+    version: 'some-version',
+    kibanaVersion: '7.0.0-alpha2',
+    optionalPlugins: [],
+    requiredPlugins: ['some-required-plugin'],
+    ui: false,
+  });
+});
+
+test('return manifest when plugin expected Kibana version is `kibana`', async () => {
+  mockReadFile.mockImplementation((path, cb) => {
+    cb(
+      null,
+      Buffer.from(
+        JSON.stringify({
+          id: 'some-id',
+          version: 'some-version',
+          kibanaVersion: 'kibana',
+          requiredPlugins: ['some-required-plugin'],
+        })
+      )
+    );
+  });
+
+  await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({
+    id: 'some-id',
+    version: 'some-version',
+    kibanaVersion: 'kibana',
+    optionalPlugins: [],
+    requiredPlugins: ['some-required-plugin'],
+    ui: false,
+  });
+});
diff --git a/src/core/server/plugins/discovery/plugin_manifest_parser.ts b/src/core/server/plugins/discovery/plugin_manifest_parser.ts
index a0944e070c75e..4152d22fc5906 100644
--- a/src/core/server/plugins/discovery/plugin_manifest_parser.ts
+++ b/src/core/server/plugins/discovery/plugin_manifest_parser.ts
@@ -19,12 +19,11 @@
 
 import { readFile } from 'fs';
 import { resolve } from 'path';
-import { bindNodeCallback, throwError } from 'rxjs';
-import { catchError, map } from 'rxjs/operators';
+import { promisify } from 'util';
 import { PackageInfo } from '../../config';
 import { PluginDiscoveryError } from './plugin_discovery_error';
 
-const fsReadFile$ = bindNodeCallback(readFile);
+const fsReadFileAsync = promisify(readFile);
 
 /**
  * Describes the set of required and optional properties plugin can define in its
@@ -89,66 +88,69 @@ const SEM_VER_REGEX = /\d+\.\d+\.\d+/;
  * @param pluginPath Path to the plugin directory where manifest should be loaded from.
  * @param packageInfo Kibana package info.
  */
-export function parseManifest$(pluginPath: string, packageInfo: PackageInfo) {
+export async function parseManifest(pluginPath: string, packageInfo: PackageInfo) {
   const manifestPath = resolve(pluginPath, MANIFEST_FILE_NAME);
-  return fsReadFile$(manifestPath).pipe(
-    catchError(err => throwError(PluginDiscoveryError.missingManifest(manifestPath, err))),
-    map(manifestContent => {
-      let manifest: Partial<PluginManifest>;
-      try {
-        manifest = JSON.parse(manifestContent.toString());
-      } catch (err) {
-        throw PluginDiscoveryError.invalidManifest(manifestPath, err);
-      }
-
-      if (!manifest || typeof manifest !== 'object') {
-        throw PluginDiscoveryError.invalidManifest(
-          manifestPath,
-          new Error('Plugin manifest must contain a JSON encoded object.')
-        );
-      }
-
-      if (!manifest.id || typeof manifest.id !== 'string') {
-        throw PluginDiscoveryError.invalidManifest(
-          manifestPath,
-          new Error('Plugin manifest must contain an "id" property.')
-        );
-      }
-
-      if (!manifest.version || typeof manifest.version !== 'string') {
-        throw PluginDiscoveryError.invalidManifest(
-          manifestPath,
-          new Error(`Plugin manifest for "${manifest.id}" must contain a "version" property.`)
-        );
-      }
-
-      const expectedKibanaVersion =
-        typeof manifest.kibanaVersion === 'string' && manifest.kibanaVersion
-          ? manifest.kibanaVersion
-          : manifest.version;
-      if (!isVersionCompatible(expectedKibanaVersion, packageInfo.version)) {
-        throw PluginDiscoveryError.incompatibleVersion(
-          manifestPath,
-          new Error(
-            `Plugin "${
-              manifest.id
-            }" is only compatible with Kibana version "${expectedKibanaVersion}", but used Kibana version is "${
-              packageInfo.version
-            }".`
-          )
-        );
-      }
-
-      return {
-        id: manifest.id,
-        version: manifest.version,
-        kibanaVersion: expectedKibanaVersion,
-        requiredPlugins: Array.isArray(manifest.requiredPlugins) ? manifest.requiredPlugins : [],
-        optionalPlugins: Array.isArray(manifest.optionalPlugins) ? manifest.optionalPlugins : [],
-        ui: typeof manifest.ui === 'boolean' ? manifest.ui : false,
-      };
-    })
-  );
+
+  let manifestContent;
+  try {
+    manifestContent = await fsReadFileAsync(manifestPath);
+  } catch (err) {
+    throw PluginDiscoveryError.missingManifest(manifestPath, err);
+  }
+
+  let manifest: Partial<PluginManifest>;
+  try {
+    manifest = JSON.parse(manifestContent.toString());
+  } catch (err) {
+    throw PluginDiscoveryError.invalidManifest(manifestPath, err);
+  }
+
+  if (!manifest || typeof manifest !== 'object') {
+    throw PluginDiscoveryError.invalidManifest(
+      manifestPath,
+      new Error('Plugin manifest must contain a JSON encoded object.')
+    );
+  }
+
+  if (!manifest.id || typeof manifest.id !== 'string') {
+    throw PluginDiscoveryError.invalidManifest(
+      manifestPath,
+      new Error('Plugin manifest must contain an "id" property.')
+    );
+  }
+
+  if (!manifest.version || typeof manifest.version !== 'string') {
+    throw PluginDiscoveryError.invalidManifest(
+      manifestPath,
+      new Error(`Plugin manifest for "${manifest.id}" must contain a "version" property.`)
+    );
+  }
+
+  const expectedKibanaVersion =
+    typeof manifest.kibanaVersion === 'string' && manifest.kibanaVersion
+      ? manifest.kibanaVersion
+      : manifest.version;
+  if (!isVersionCompatible(expectedKibanaVersion, packageInfo.version)) {
+    throw PluginDiscoveryError.incompatibleVersion(
+      manifestPath,
+      new Error(
+        `Plugin "${
+          manifest.id
+        }" is only compatible with Kibana version "${expectedKibanaVersion}", but used Kibana version is "${
+          packageInfo.version
+        }".`
+      )
+    );
+  }
+
+  return {
+    id: manifest.id,
+    version: manifest.version,
+    kibanaVersion: expectedKibanaVersion,
+    requiredPlugins: Array.isArray(manifest.requiredPlugins) ? manifest.requiredPlugins : [],
+    optionalPlugins: Array.isArray(manifest.optionalPlugins) ? manifest.optionalPlugins : [],
+    ui: typeof manifest.ui === 'boolean' ? manifest.ui : false,
+  };
 }
 
 /**
diff --git a/src/core/server/plugins/discovery/plugins_discovery.ts b/src/core/server/plugins/discovery/plugins_discovery.ts
index 12c2989dd4a11..7ecfaa38b3826 100644
--- a/src/core/server/plugins/discovery/plugins_discovery.ts
+++ b/src/core/server/plugins/discovery/plugins_discovery.ts
@@ -25,7 +25,7 @@ import { PackageInfo } from '../../config';
 import { Logger } from '../../logging';
 import { PluginsConfig } from '../plugins_config';
 import { PluginDiscoveryError } from './plugin_discovery_error';
-import { parseManifest$, PluginManifest } from './plugin_manifest_parser';
+import { parseManifest, PluginManifest } from './plugin_manifest_parser';
 
 const fsReadDir$ = bindNodeCallback(readdir);
 const fsStat$ = bindNodeCallback(stat);
@@ -138,7 +138,7 @@ function createPlugin$(
   packageInfo: PackageInfo,
   log: Logger
 ): Observable<DiscoveryResult> {
-  return parseManifest$(path, packageInfo).pipe(
+  return from(parseManifest(path, packageInfo)).pipe(
     map(manifest => {
       log.debug(`Successfully discovered plugin "${manifest.id}" at "${path}"`);
       return { plugin: { path, manifest } };

From c2af2d3b3cec342822b445f6c7922954e1ab6c74 Mon Sep 17 00:00:00 2001
From: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Date: Tue, 6 Nov 2018 14:13:23 +0100
Subject: [PATCH 15/15] =?UTF-8?q?Review=20=E2=84=963:=20add=20basic=20`Plu?=
 =?UTF-8?q?ginService`=20test.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../discovery/plugin_discovery_error.ts       |   6 +-
 .../discovery/plugin_manifest_parser.test.ts  |  16 +-
 .../server/plugins/plugins_service.test.ts    | 146 ++++++++++++++++++
 src/core/server/plugins/plugins_service.ts    |  10 +-
 4 files changed, 163 insertions(+), 15 deletions(-)
 create mode 100644 src/core/server/plugins/plugins_service.test.ts

diff --git a/src/core/server/plugins/discovery/plugin_discovery_error.ts b/src/core/server/plugins/discovery/plugin_discovery_error.ts
index 7c68eb378456b..d513091ac4505 100644
--- a/src/core/server/plugins/discovery/plugin_discovery_error.ts
+++ b/src/core/server/plugins/discovery/plugin_discovery_error.ts
@@ -56,14 +56,10 @@ export class PluginDiscoveryError extends Error {
     public readonly path: string,
     public readonly cause: Error
   ) {
-    super(cause.message);
+    super(`${cause.message} (${type}, ${path})`);
 
     // Set the prototype explicitly, see:
     // https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
     Object.setPrototypeOf(this, PluginDiscoveryError.prototype);
   }
-
-  public toString() {
-    return `${super.toString()} (${this.type}, ${this.path})`;
-  }
 }
diff --git a/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts b/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts
index ada52c17e11ee..1f73855511c24 100644
--- a/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts
+++ b/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts
@@ -44,7 +44,7 @@ test('return error when manifest is empty', async () => {
   });
 
   await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
-    message: 'Unexpected end of JSON input',
+    message: `Unexpected end of JSON input (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
   });
@@ -56,7 +56,7 @@ test('return error when manifest content is null', async () => {
   });
 
   await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
-    message: 'Plugin manifest must contain a JSON encoded object.',
+    message: `Plugin manifest must contain a JSON encoded object. (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
   });
@@ -68,7 +68,7 @@ test('return error when manifest content is not a valid JSON', async () => {
   });
 
   await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
-    message: 'Unexpected token o in JSON at position 1',
+    message: `Unexpected token o in JSON at position 1 (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
   });
@@ -80,7 +80,7 @@ test('return error when plugin id is missing', async () => {
   });
 
   await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
-    message: 'Plugin manifest must contain an "id" property.',
+    message: `Plugin manifest must contain an "id" property. (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
   });
@@ -92,7 +92,7 @@ test('return error when plugin version is missing', async () => {
   });
 
   await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
-    message: 'Plugin manifest for "some-id" must contain a "version" property.',
+    message: `Plugin manifest for "some-id" must contain a "version" property. (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
   });
@@ -104,8 +104,7 @@ test('return error when plugin expected Kibana version is lower than actual vers
   });
 
   await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
-    message:
-      'Plugin "some-id" is only compatible with Kibana version "6.4.2", but used Kibana version is "7.0.0-alpha1".',
+    message: `Plugin "some-id" is only compatible with Kibana version "6.4.2", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.IncompatibleVersion,
     path: pluginManifestPath,
   });
@@ -117,8 +116,7 @@ test('return error when plugin expected Kibana version is higher than actual ver
   });
 
   await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
-    message:
-      'Plugin "some-id" is only compatible with Kibana version "7.0.1", but used Kibana version is "7.0.0-alpha1".',
+    message: `Plugin "some-id" is only compatible with Kibana version "7.0.1", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.IncompatibleVersion,
     path: pluginManifestPath,
   });
diff --git a/src/core/server/plugins/plugins_service.test.ts b/src/core/server/plugins/plugins_service.test.ts
new file mode 100644
index 0000000000000..4d448226e16f9
--- /dev/null
+++ b/src/core/server/plugins/plugins_service.test.ts
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ */
+
+const mockPackage = new Proxy({ raw: {} as any }, { get: (obj, prop) => obj.raw[prop] });
+jest.mock('../../../utils/package_json', () => ({ pkg: mockPackage }));
+
+const mockDiscover = jest.fn();
+jest.mock('./discovery/plugins_discovery', () => ({ discover: mockDiscover }));
+
+import { BehaviorSubject, from } from 'rxjs';
+
+import { Config, ConfigService, Env, ObjectToConfigAdapter } from '../config';
+import { getEnvOptions } from '../config/__mocks__/env';
+import { logger } from '../logging/__mocks__';
+import { PluginDiscoveryError } from './discovery/plugin_discovery_error';
+import { PluginsService } from './plugins_service';
+
+let pluginsService: PluginsService;
+let configService: ConfigService;
+let env: Env;
+beforeEach(() => {
+  mockPackage.raw = {
+    branch: 'feature-v1',
+    version: 'v1',
+    build: {
+      distributable: true,
+      number: 100,
+      sha: 'feature-v1-build-sha',
+    },
+  };
+
+  env = Env.createDefault(getEnvOptions());
+
+  configService = new ConfigService(
+    new BehaviorSubject<Config>(
+      new ObjectToConfigAdapter({
+        plugins: {
+          initialize: true,
+          scanDirs: ['one', 'two'],
+          paths: ['three', 'four'],
+        },
+      })
+    ),
+    env,
+    logger
+  );
+  pluginsService = new PluginsService(env, logger, configService);
+});
+
+afterEach(() => {
+  jest.clearAllMocks();
+});
+
+test('properly invokes `discover` on `start`.', async () => {
+  mockDiscover.mockReturnValue({
+    error$: from([
+      PluginDiscoveryError.invalidManifest('path-1', new Error('Invalid JSON')),
+      PluginDiscoveryError.missingManifest('path-2', new Error('No manifest')),
+      PluginDiscoveryError.invalidScanDirectory('dir-1', new Error('No dir')),
+      PluginDiscoveryError.incompatibleVersion('path-3', new Error('Incompatible version')),
+    ]),
+
+    plugin$: from([
+      {
+        path: 'path-4',
+        manifest: {
+          id: 'some-id',
+          version: 'some-version',
+          kibanaVersion: '7.0.0',
+          requiredPlugins: ['some-required-plugin', 'some-required-plugin-2'],
+          optionalPlugins: ['some-optional-plugin'],
+          ui: true,
+        },
+      },
+      {
+        path: 'path-5',
+        manifest: {
+          id: 'some-other-id',
+          version: 'some-other-version',
+          kibanaVersion: '7.0.0',
+          requiredPlugins: ['some-required-plugin'],
+          optionalPlugins: [],
+          ui: false,
+        },
+      },
+    ]),
+  });
+
+  await pluginsService.start();
+
+  expect(mockDiscover).toHaveBeenCalledTimes(1);
+  expect(mockDiscover).toHaveBeenCalledWith(
+    { initialize: true, paths: ['three', 'four'], scanDirs: ['one', 'two'] },
+    { branch: 'feature-v1', buildNum: 100, buildSha: 'feature-v1-build-sha', version: 'v1' },
+    expect.objectContaining({
+      debug: expect.any(Function),
+      error: expect.any(Function),
+      info: expect.any(Function),
+    })
+  );
+
+  expect(logger.mockCollect()).toMatchInlineSnapshot(`
+Object {
+  "debug": Array [
+    Array [
+      "starting plugins service",
+    ],
+    Array [
+      "Marking config path as handled: plugins",
+    ],
+    Array [
+      "Discovered 2 plugins.",
+    ],
+  ],
+  "error": Array [
+    Array [
+      [Error: Invalid JSON (invalid-manifest, path-1)],
+    ],
+    Array [
+      [Error: Incompatible version (incompatible-version, path-3)],
+    ],
+  ],
+  "fatal": Array [],
+  "info": Array [],
+  "log": Array [],
+  "trace": Array [],
+  "warn": Array [],
+}
+`);
+});
diff --git a/src/core/server/plugins/plugins_service.ts b/src/core/server/plugins/plugins_service.ts
index 836107ba0d0a4..602a801ccd300 100644
--- a/src/core/server/plugins/plugins_service.ts
+++ b/src/core/server/plugins/plugins_service.ts
@@ -38,6 +38,14 @@ export class PluginsService implements CoreService {
   public async start() {
     this.log.debug('starting plugins service');
 
+    // At this stage we report only errors that can occur when new platform plugin
+    // manifest is present, otherwise we can't be sure that the plugin is for the new
+    // platform and let legacy platform to handle it.
+    const errorTypesToReport = [
+      PluginDiscoveryErrorType.IncompatibleVersion,
+      PluginDiscoveryErrorType.InvalidManifest,
+    ];
+
     const { error$, plugin$ } = await this.configService
       .atPath('plugins', PluginsConfig)
       .pipe(
@@ -50,7 +58,7 @@ export class PluginsService implements CoreService {
 
     await error$
       .pipe(
-        filter(error => error.type === PluginDiscoveryErrorType.InvalidManifest),
+        filter(error => errorTypesToReport.includes(error.type)),
         tap(invalidManifestError => this.log.error(invalidManifestError))
       )
       .toPromise();