Skip to content

Commit

Permalink
[Task Manager] Cleans up legacy plugin structure (elastic#80381) (ela…
Browse files Browse the repository at this point in the history
…stic#81139)

This PR addresses a list of legacy code debt the plugin has incurred over the past year due to extensive changes in its internals and the adoption of the Kibana Platform.

It includes:
1. The `TaskManager` class has been split into several independent components: `TaskTypeDictionary`,  `TaskPollingLifecycle`,  `TaskScheduling`,  `Middleware`. This has made it easier to understand the roles of the different parts and makes it easier to plug them into the observability work.
2. The exposed `mocks` have been corrected to correctly express the Kibana Platform api
3. The lifecycle has been corrected to remove the need for  intermediary streames/promises which we're needed when we first introduced the `setup`/`start` lifecycle to support legacy.
4. The Logger mocks have been replaced with the platform's `coreMocks` implementation
5. The integration tests now test the plugin's actual public api (instead of the internals).
6. The Legacy Elasticsearch client has been replaced with the typed client in response to the deprecation notice.
7. Typing has been narrowed to prevent the `type` field from conflicting with the key in the `TaskDictionary`. This could have caused the displayed `type` on a task to differ from the `type` used in the Dictionary itself (this broke a test during refactoring and could have caused a bug in production code if left).
  • Loading branch information
gmmorris authored Oct 20, 2020
1 parent 6b29f9f commit 831dbf9
Show file tree
Hide file tree
Showing 69 changed files with 1,583 additions and 1,635 deletions.
5 changes: 2 additions & 3 deletions x-pack/plugins/actions/server/action_type_registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { taskManagerMock } from '../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../task_manager/server/mocks';
import { ActionTypeRegistry, ActionTypeRegistryOpts } from './action_type_registry';
import { ActionType, ExecutorType } from './types';
import { ActionExecutor, ExecutorError, ILicenseState, TaskRunnerFactory } from './lib';
Expand All @@ -13,7 +13,7 @@ import { licenseStateMock } from './lib/license_state.mock';
import { ActionsConfigurationUtilities } from './actions_config';
import { licensingMock } from '../../licensing/server/mocks';

const mockTaskManager = taskManagerMock.setup();
const mockTaskManager = taskManagerMock.createSetup();
let mockedLicenseState: jest.Mocked<ILicenseState>;
let mockedActionsConfig: jest.Mocked<ActionsConfigurationUtilities>;
let actionTypeRegistryParams: ActionTypeRegistryOpts;
Expand Down Expand Up @@ -66,7 +66,6 @@ describe('register()', () => {
"getRetry": [Function],
"maxAttempts": 1,
"title": "My action type",
"type": "actions:my-action-type",
},
},
]
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/actions/server/action_type_registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@ export class ActionTypeRegistry {
this.taskManager.registerTaskDefinitions({
[`actions:${actionType.id}`]: {
title: actionType.name,
type: `actions:${actionType.id}`,
maxAttempts: actionType.maxAttempts || 1,
getRetry(attempts: number, error: unknown) {
if (error instanceof ExecutorError) {
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/actions/server/actions_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { ActionTypeRegistry, ActionTypeRegistryOpts } from './action_type_regist
import { ActionsClient } from './actions_client';
import { ExecutorType, ActionType } from './types';
import { ActionExecutor, TaskRunnerFactory, ILicenseState } from './lib';
import { taskManagerMock } from '../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../task_manager/server/mocks';
import { actionsConfigMock } from './actions_config.mock';
import { getActionsConfigurationUtilities } from './actions_config';
import { licenseStateMock } from './lib/license_state.mock';
Expand All @@ -34,7 +34,7 @@ const authorization = actionsAuthorizationMock.create();
const executionEnqueuer = jest.fn();
const request = {} as KibanaRequest;

const mockTaskManager = taskManagerMock.setup();
const mockTaskManager = taskManagerMock.createSetup();

let actionsClient: ActionsClient;
let mockedLicenseState: jest.Mocked<ILicenseState>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import { ActionExecutor, TaskRunnerFactory } from '../lib';
import { ActionTypeRegistry } from '../action_type_registry';
import { taskManagerMock } from '../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../task_manager/server/mocks';
import { registerBuiltInActionTypes } from './index';
import { Logger } from '../../../../../src/core/server';
import { loggingSystemMock } from '../../../../../src/core/server/mocks';
Expand All @@ -22,8 +22,8 @@ export function createActionTypeRegistry(): {
} {
const logger = loggingSystemMock.create().get() as jest.Mocked<Logger>;
const actionTypeRegistry = new ActionTypeRegistry({
taskManager: taskManagerMock.createSetup(),
licensing: licensingMock.createSetup(),
taskManager: taskManagerMock.setup(),
taskRunnerFactory: new TaskRunnerFactory(
new ActionExecutor({ isESOUsingEphemeralEncryptionKey: false })
),
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/actions/server/create_execute_function.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import { KibanaRequest } from 'src/core/server';
import uuid from 'uuid';
import { taskManagerMock } from '../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../task_manager/server/mocks';
import { createExecutionEnqueuerFunction } from './create_execute_function';
import { savedObjectsClientMock } from '../../../../src/core/server/mocks';
import { actionTypeRegistryMock } from './action_type_registry.mock';
Expand All @@ -15,7 +15,7 @@ import {
asSavedObjectExecutionSource,
} from './lib/action_execution_source';

const mockTaskManager = taskManagerMock.start();
const mockTaskManager = taskManagerMock.createStart();
const savedObjectsClient = savedObjectsClientMock.create();
const request = {} as KibanaRequest;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@

import { UsageCollectionSetup } from 'src/plugins/usage_collection/server';
import { registerActionsUsageCollector } from './actions_usage_collector';
import { taskManagerMock } from '../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../task_manager/server/mocks';

const mockTaskManagerStart = taskManagerMock.start();
const mockTaskManagerStart = taskManagerMock.createStart();

beforeEach(() => jest.resetAllMocks());

Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/actions/server/usage/task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ function registerActionsTelemetryTask(
taskManager.registerTaskDefinitions({
[TELEMETRY_TASK_TYPE]: {
title: 'Actions usage fetch task',
type: TELEMETRY_TASK_TYPE,
timeout: '5m',
createTaskRunner: telemetryTaskRunner(logger, core, kibanaIndex),
},
Expand Down
5 changes: 2 additions & 3 deletions x-pack/plugins/alerts/server/alert_type_registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
import { TaskRunnerFactory } from './task_runner';
import { AlertTypeRegistry } from './alert_type_registry';
import { AlertType } from './types';
import { taskManagerMock } from '../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../task_manager/server/mocks';

const taskManager = taskManagerMock.setup();
const taskManager = taskManagerMock.createSetup();
const alertTypeRegistryParams = {
taskManager,
taskRunnerFactory: new TaskRunnerFactory(),
Expand Down Expand Up @@ -118,7 +118,6 @@ describe('register()', () => {
"alerting:test": Object {
"createTaskRunner": [Function],
"title": "Test",
"type": "alerting:test",
},
},
]
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/alerts/server/alert_type_registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ export class AlertTypeRegistry {
this.taskManager.registerTaskDefinitions({
[`alerting:${alertType.id}`]: {
title: alertType.name,
type: `alerting:${alertType.id}`,
createTaskRunner: (context: RunContext) =>
this.taskRunnerFactory.create({ ...alertType } as AlertType, context),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { schema } from '@kbn/config-schema';
import { AlertsClient, ConstructorOptions, CreateOptions } from '../alerts_client';
import { savedObjectsClientMock, loggingSystemMock } from '../../../../../../src/core/server/mocks';
import { taskManagerMock } from '../../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../../task_manager/server/mocks';
import { alertTypeRegistryMock } from '../../alert_type_registry.mock';
import { alertsAuthorizationMock } from '../../authorization/alerts_authorization.mock';
import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/server/mocks';
Expand All @@ -16,7 +16,7 @@ import { ActionsAuthorization, ActionsClient } from '../../../../actions/server'
import { TaskStatus } from '../../../../task_manager/server';
import { getBeforeSetup, setGlobalDate } from './lib';

const taskManager = taskManagerMock.start();
const taskManager = taskManagerMock.createStart();
const alertTypeRegistry = alertTypeRegistryMock.create();
const unsecuredSavedObjectsClient = savedObjectsClientMock.create();
const encryptedSavedObjects = encryptedSavedObjectsMock.createClient();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import { AlertsClient, ConstructorOptions } from '../alerts_client';
import { savedObjectsClientMock, loggingSystemMock } from '../../../../../../src/core/server/mocks';
import { taskManagerMock } from '../../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../../task_manager/server/mocks';
import { alertTypeRegistryMock } from '../../alert_type_registry.mock';
import { alertsAuthorizationMock } from '../../authorization/alerts_authorization.mock';
import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/server/mocks';
Expand All @@ -14,7 +14,7 @@ import { AlertsAuthorization } from '../../authorization/alerts_authorization';
import { ActionsAuthorization } from '../../../../actions/server';
import { getBeforeSetup } from './lib';

const taskManager = taskManagerMock.start();
const taskManager = taskManagerMock.createStart();
const alertTypeRegistry = alertTypeRegistryMock.create();
const unsecuredSavedObjectsClient = savedObjectsClientMock.create();
const encryptedSavedObjects = encryptedSavedObjectsMock.createClient();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import { AlertsClient, ConstructorOptions } from '../alerts_client';
import { savedObjectsClientMock, loggingSystemMock } from '../../../../../../src/core/server/mocks';
import { taskManagerMock } from '../../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../../task_manager/server/mocks';
import { alertTypeRegistryMock } from '../../alert_type_registry.mock';
import { alertsAuthorizationMock } from '../../authorization/alerts_authorization.mock';
import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/server/mocks';
Expand All @@ -14,7 +14,7 @@ import { AlertsAuthorization } from '../../authorization/alerts_authorization';
import { ActionsAuthorization } from '../../../../actions/server';
import { getBeforeSetup } from './lib';

const taskManager = taskManagerMock.start();
const taskManager = taskManagerMock.createStart();
const alertTypeRegistry = alertTypeRegistryMock.create();
const unsecuredSavedObjectsClient = savedObjectsClientMock.create();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import { AlertsClient, ConstructorOptions } from '../alerts_client';
import { savedObjectsClientMock, loggingSystemMock } from '../../../../../../src/core/server/mocks';
import { taskManagerMock } from '../../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../../task_manager/server/mocks';
import { alertTypeRegistryMock } from '../../alert_type_registry.mock';
import { alertsAuthorizationMock } from '../../authorization/alerts_authorization.mock';
import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/server/mocks';
Expand All @@ -15,7 +15,7 @@ import { ActionsAuthorization } from '../../../../actions/server';
import { TaskStatus } from '../../../../task_manager/server';
import { getBeforeSetup } from './lib';

const taskManager = taskManagerMock.start();
const taskManager = taskManagerMock.createStart();
const alertTypeRegistry = alertTypeRegistryMock.create();
const unsecuredSavedObjectsClient = savedObjectsClientMock.create();

Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/alerts/server/alerts_client/tests/find.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import { AlertsClient, ConstructorOptions } from '../alerts_client';
import { savedObjectsClientMock, loggingSystemMock } from '../../../../../../src/core/server/mocks';
import { taskManagerMock } from '../../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../../task_manager/server/mocks';
import { alertTypeRegistryMock } from '../../alert_type_registry.mock';
import { alertsAuthorizationMock } from '../../authorization/alerts_authorization.mock';
import { nodeTypes } from '../../../../../../src/plugins/data/common';
Expand All @@ -16,7 +16,7 @@ import { AlertsAuthorization } from '../../authorization/alerts_authorization';
import { ActionsAuthorization } from '../../../../actions/server';
import { getBeforeSetup, setGlobalDate } from './lib';

const taskManager = taskManagerMock.start();
const taskManager = taskManagerMock.createStart();
const alertTypeRegistry = alertTypeRegistryMock.create();
const unsecuredSavedObjectsClient = savedObjectsClientMock.create();

Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/alerts/server/alerts_client/tests/get.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import { AlertsClient, ConstructorOptions } from '../alerts_client';
import { savedObjectsClientMock, loggingSystemMock } from '../../../../../../src/core/server/mocks';
import { taskManagerMock } from '../../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../../task_manager/server/mocks';
import { alertTypeRegistryMock } from '../../alert_type_registry.mock';
import { alertsAuthorizationMock } from '../../authorization/alerts_authorization.mock';
import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/server/mocks';
Expand All @@ -14,7 +14,7 @@ import { AlertsAuthorization } from '../../authorization/alerts_authorization';
import { ActionsAuthorization } from '../../../../actions/server';
import { getBeforeSetup, setGlobalDate } from './lib';

const taskManager = taskManagerMock.start();
const taskManager = taskManagerMock.createStart();
const alertTypeRegistry = alertTypeRegistryMock.create();
const unsecuredSavedObjectsClient = savedObjectsClientMock.create();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import { AlertsClient, ConstructorOptions } from '../alerts_client';
import { savedObjectsClientMock, loggingSystemMock } from '../../../../../../src/core/server/mocks';
import { taskManagerMock } from '../../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../../task_manager/server/mocks';
import { alertTypeRegistryMock } from '../../alert_type_registry.mock';
import { alertsAuthorizationMock } from '../../authorization/alerts_authorization.mock';
import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/server/mocks';
Expand All @@ -19,7 +19,7 @@ import { EventsFactory } from '../../lib/alert_instance_summary_from_event_log.t
import { RawAlert } from '../../types';
import { getBeforeSetup, mockedDateString, setGlobalDate } from './lib';

const taskManager = taskManagerMock.start();
const taskManager = taskManagerMock.createStart();
const alertTypeRegistry = alertTypeRegistryMock.create();
const unsecuredSavedObjectsClient = savedObjectsClientMock.create();
const eventLogClient = eventLogClientMock.create();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import { AlertsClient, ConstructorOptions } from '../alerts_client';
import { savedObjectsClientMock, loggingSystemMock } from '../../../../../../src/core/server/mocks';
import { taskManagerMock } from '../../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../../task_manager/server/mocks';
import { alertTypeRegistryMock } from '../../alert_type_registry.mock';
import { alertsAuthorizationMock } from '../../authorization/alerts_authorization.mock';
import { TaskStatus } from '../../../../task_manager/server';
Expand All @@ -15,7 +15,7 @@ import { AlertsAuthorization } from '../../authorization/alerts_authorization';
import { ActionsAuthorization } from '../../../../actions/server';
import { getBeforeSetup } from './lib';

const taskManager = taskManagerMock.start();
const taskManager = taskManagerMock.createStart();
const alertTypeRegistry = alertTypeRegistryMock.create();
const unsecuredSavedObjectsClient = savedObjectsClientMock.create();

Expand Down
7 changes: 2 additions & 5 deletions x-pack/plugins/alerts/server/alerts_client/tests/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { TaskManager } from '../../../../task_manager/server/task_manager';
import { taskManagerMock } from '../../../../task_manager/server/mocks';
import { IEventLogClient } from '../../../../event_log/server';
import { actionsClientMock } from '../../../../actions/server/mocks';
import { ConstructorOptions } from '../alerts_client';
Expand Down Expand Up @@ -41,9 +40,7 @@ export function setGlobalDate() {

export function getBeforeSetup(
alertsClientParams: jest.Mocked<ConstructorOptions>,
taskManager: jest.Mocked<
Pick<TaskManager, 'fetch' | 'get' | 'remove' | 'schedule' | 'runNow' | 'ensureScheduled'>
>,
taskManager: ReturnType<typeof taskManagerMock.createStart>,
alertTypeRegistry: jest.Mocked<Pick<AlertTypeRegistry, 'get' | 'has' | 'register' | 'list'>>,
eventLogClient?: jest.Mocked<IEventLogClient>
) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import { AlertsClient, ConstructorOptions } from '../alerts_client';
import { savedObjectsClientMock, loggingSystemMock } from '../../../../../../src/core/server/mocks';
import { taskManagerMock } from '../../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../../task_manager/server/mocks';
import { alertTypeRegistryMock } from '../../alert_type_registry.mock';
import { alertsAuthorizationMock } from '../../authorization/alerts_authorization.mock';
import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/server/mocks';
Expand All @@ -14,7 +14,7 @@ import { AlertsAuthorization } from '../../authorization/alerts_authorization';
import { ActionsAuthorization } from '../../../../actions/server';
import { getBeforeSetup } from './lib';

const taskManager = taskManagerMock.start();
const taskManager = taskManagerMock.createStart();
const alertTypeRegistry = alertTypeRegistryMock.create();
const unsecuredSavedObjectsClient = savedObjectsClientMock.create();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import { AlertsClient, ConstructorOptions } from '../alerts_client';
import { savedObjectsClientMock, loggingSystemMock } from '../../../../../../src/core/server/mocks';
import { taskManagerMock } from '../../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../../task_manager/server/mocks';
import { alertTypeRegistryMock } from '../../alert_type_registry.mock';
import { alertsAuthorizationMock } from '../../authorization/alerts_authorization.mock';
import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/server/mocks';
Expand All @@ -14,7 +14,7 @@ import { AlertsAuthorization } from '../../authorization/alerts_authorization';
import { ActionsAuthorization } from '../../../../actions/server';
import { getBeforeSetup } from './lib';

const taskManager = taskManagerMock.start();
const taskManager = taskManagerMock.createStart();
const alertTypeRegistry = alertTypeRegistryMock.create();
const unsecuredSavedObjectsClient = savedObjectsClientMock.create();
const encryptedSavedObjects = encryptedSavedObjectsMock.createClient();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import { AlertsClient, ConstructorOptions } from '../alerts_client';
import { savedObjectsClientMock, loggingSystemMock } from '../../../../../../src/core/server/mocks';
import { taskManagerMock } from '../../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../../task_manager/server/mocks';
import { alertTypeRegistryMock } from '../../alert_type_registry.mock';
import { alertsAuthorizationMock } from '../../authorization/alerts_authorization.mock';
import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/server/mocks';
Expand All @@ -14,7 +14,7 @@ import { AlertsAuthorization } from '../../authorization/alerts_authorization';
import { ActionsAuthorization } from '../../../../actions/server';
import { getBeforeSetup } from './lib';

const taskManager = taskManagerMock.start();
const taskManager = taskManagerMock.createStart();
const alertTypeRegistry = alertTypeRegistryMock.create();
const unsecuredSavedObjectsClient = savedObjectsClientMock.create();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import { AlertsClient, ConstructorOptions } from '../alerts_client';
import { savedObjectsClientMock, loggingSystemMock } from '../../../../../../src/core/server/mocks';
import { taskManagerMock } from '../../../../task_manager/server/task_manager.mock';
import { taskManagerMock } from '../../../../task_manager/server/mocks';
import { alertTypeRegistryMock } from '../../alert_type_registry.mock';
import { alertsAuthorizationMock } from '../../authorization/alerts_authorization.mock';
import { encryptedSavedObjectsMock } from '../../../../encrypted_saved_objects/server/mocks';
Expand All @@ -14,7 +14,7 @@ import { AlertsAuthorization } from '../../authorization/alerts_authorization';
import { ActionsAuthorization } from '../../../../actions/server';
import { getBeforeSetup } from './lib';

const taskManager = taskManagerMock.start();
const taskManager = taskManagerMock.createStart();
const alertTypeRegistry = alertTypeRegistryMock.create();
const unsecuredSavedObjectsClient = savedObjectsClientMock.create();

Expand Down
Loading

0 comments on commit 831dbf9

Please sign in to comment.