From a7e01ab339913ea90837b5aba29c1556b15c2572 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 18 Feb 2022 13:34:28 +0100 Subject: [PATCH] make some breaking changes to TaskStore and TaskBroker Signed-off-by: blam --- .../src/scaffolder/tasks/DatabaseTaskStore.ts | 13 +++-- .../tasks/StorageTaskBroker.test.ts | 27 +++++----- .../src/scaffolder/tasks/StorageTaskBroker.ts | 9 ++-- .../src/scaffolder/tasks/TaskWorker.test.ts | 36 +++++++------ .../src/scaffolder/tasks/index.ts | 2 + .../src/scaffolder/tasks/types.ts | 53 +++++++++++++++---- .../scaffolder-backend/src/service/router.ts | 9 ++-- 7 files changed, 98 insertions(+), 51 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts index 173fbb7778..389ecae5c6 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts @@ -24,11 +24,11 @@ import { SerializedTask, TaskStatus, TaskEventType, - TaskSecrets, - TaskSpec, TaskStore, TaskStoreEmitOptions, TaskStoreListEventsOptions, + TaskStoreCreateTaskOptions, + TaskStoreCreateTaskResult, } from './types'; import { DateTime } from 'luxon'; @@ -108,14 +108,13 @@ export class DatabaseTaskStore implements TaskStore { } async createTask( - spec: TaskSpec, - secrets?: TaskSecrets, - ): Promise<{ taskId: string }> { + options: TaskStoreCreateTaskOptions, + ): Promise { const taskId = uuid(); await this.db('tasks').insert({ id: taskId, - spec: JSON.stringify(spec), - secrets: secrets ? JSON.stringify(secrets) : undefined, + spec: JSON.stringify(options.spec), + secrets: options.secrets ? JSON.stringify(options.secrets) : undefined, status: 'open', }); return { taskId }; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.test.ts index 93285a4ab3..daf444cdd8 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.test.ts @@ -47,7 +47,7 @@ describe('StorageTaskBroker', () => { const logger = getVoidLogger(); it('should claim a dispatched work item', async () => { const broker = new StorageTaskBroker(storage, logger); - await broker.dispatch({} as TaskSpec); + await broker.dispatch({ spec: {} as TaskSpec }); await expect(broker.claim()).resolves.toEqual(expect.any(TaskManager)); }); @@ -57,15 +57,15 @@ describe('StorageTaskBroker', () => { await expect(Promise.race([promise, 'waiting'])).resolves.toBe('waiting'); - await broker.dispatch({} as TaskSpec); + await broker.dispatch({ spec: {} as TaskSpec }); await expect(promise).resolves.toEqual(expect.any(TaskManager)); }); it('should dispatch multiple items and claim them in order', async () => { const broker = new StorageTaskBroker(storage, logger); - await broker.dispatch({ steps: [{ id: 'a' }] } as TaskSpec); - await broker.dispatch({ steps: [{ id: 'b' }] } as TaskSpec); - await broker.dispatch({ steps: [{ id: 'c' }] } as TaskSpec); + await broker.dispatch({ spec: { steps: [{ id: 'a' }] } as TaskSpec }); + await broker.dispatch({ spec: { steps: [{ id: 'b' }] } as TaskSpec }); + await broker.dispatch({ spec: { steps: [{ id: 'c' }] } as TaskSpec }); const taskA = await broker.claim(); const taskB = await broker.claim(); @@ -80,14 +80,14 @@ describe('StorageTaskBroker', () => { it('should store secrets', async () => { const broker = new StorageTaskBroker(storage, logger); - await broker.dispatch({} as TaskSpec, fakeSecrets); + await broker.dispatch({ spec: {} as TaskSpec, secrets: fakeSecrets }); const task = await broker.claim(); expect(task.secrets).toEqual(fakeSecrets); }, 10000); it('should complete a task', async () => { const broker = new StorageTaskBroker(storage, logger); - const dispatchResult = await broker.dispatch({} as TaskSpec); + const dispatchResult = await broker.dispatch({ spec: {} as TaskSpec }); const task = await broker.claim(); await task.complete('completed'); const taskRow = await storage.getTask(dispatchResult.taskId); @@ -96,7 +96,10 @@ describe('StorageTaskBroker', () => { it('should remove secrets after picking up a task', async () => { const broker = new StorageTaskBroker(storage, logger); - const dispatchResult = await broker.dispatch({} as TaskSpec, fakeSecrets); + const dispatchResult = await broker.dispatch({ + spec: {} as TaskSpec, + secrets: fakeSecrets, + }); await broker.claim(); const taskRow = await storage.getTask(dispatchResult.taskId); @@ -105,7 +108,7 @@ describe('StorageTaskBroker', () => { it('should fail a task', async () => { const broker = new StorageTaskBroker(storage, logger); - const dispatchResult = await broker.dispatch({} as TaskSpec); + const dispatchResult = await broker.dispatch({ spec: {} as TaskSpec }); const task = await broker.claim(); await task.complete('failed'); const taskRow = await storage.getTask(dispatchResult.taskId); @@ -116,7 +119,7 @@ describe('StorageTaskBroker', () => { const broker1 = new StorageTaskBroker(storage, logger); const broker2 = new StorageTaskBroker(storage, logger); - const { taskId } = await broker1.dispatch({} as TaskSpec); + const { taskId } = await broker1.dispatch({ spec: {} as TaskSpec }); const logPromise = new Promise(resolve => { const observedEvents = new Array(); @@ -155,7 +158,7 @@ describe('StorageTaskBroker', () => { it('should heartbeat', async () => { const broker = new StorageTaskBroker(storage, logger); - const { taskId } = await broker.dispatch({} as TaskSpec); + const { taskId } = await broker.dispatch({ spec: {} as TaskSpec }); const task = await broker.claim(); const initialTask = await storage.getTask(taskId); @@ -173,7 +176,7 @@ describe('StorageTaskBroker', () => { it('should be update the status to failed if heartbeat fails', async () => { const broker = new StorageTaskBroker(storage, logger); - const { taskId } = await broker.dispatch({} as TaskSpec); + const { taskId } = await broker.dispatch({ spec: {} as TaskSpec }); const task = await broker.claim(); jest diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts index 7f09623cb5..49c1056727 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts @@ -23,10 +23,10 @@ import { TaskSpec, TaskStore, TaskBroker, - DispatchResult, SerializedTaskEvent, SerializedTask, } from './types'; +import { TaskBrokerDispatchOptions } from '.'; /** * TaskManager @@ -155,10 +155,9 @@ export class StorageTaskBroker implements TaskBroker { } async dispatch( - spec: TaskSpec, - secrets?: TaskSecrets, - ): Promise { - const taskRow = await this.storage.createTask(spec, secrets); + options: TaskBrokerDispatchOptions, + ): Promise<{ taskId: string }> { + const taskRow = await this.storage.createTask(options); this.signalDispatch(); return { taskId: taskRow.taskId, diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.test.ts index 9b6baf3b84..9ff6b9c8a1 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.test.ts @@ -91,12 +91,14 @@ describe('TaskWorker', () => { }); await broker.dispatch({ - apiVersion: 'backstage.io/v1beta2', - steps: [{ id: 'test', name: 'test', action: 'not-found-action' }], - output: { - result: '{{ steps.test.output.testOutput }}', + spec: { + apiVersion: 'backstage.io/v1beta2', + steps: [{ id: 'test', name: 'test', action: 'not-found-action' }], + output: { + result: '{{ steps.test.output.testOutput }}', + }, + values: {}, }, - values: {}, }); const task = await broker.claim(); @@ -122,12 +124,14 @@ describe('TaskWorker', () => { }); await broker.dispatch({ - apiVersion: 'scaffolder.backstage.io/v1beta3', - steps: [{ id: 'test', name: 'test', action: 'not-found-action' }], - output: { - result: '{{ steps.test.output.testOutput }}', + spec: { + apiVersion: 'scaffolder.backstage.io/v1beta3', + steps: [{ id: 'test', name: 'test', action: 'not-found-action' }], + output: { + result: '{{ steps.test.output.testOutput }}', + }, + parameters: {}, }, - parameters: {}, }); const task = await broker.claim(); @@ -151,12 +155,14 @@ describe('TaskWorker', () => { }); const { taskId } = await broker.dispatch({ - apiVersion: 'scaffolder.backstage.io/v1beta3', - steps: [{ id: 'test', name: 'test', action: 'not-found-action' }], - output: { - result: '{{ steps.test.output.testOutput }}', + spec: { + apiVersion: 'scaffolder.backstage.io/v1beta3', + steps: [{ id: 'test', name: 'test', action: 'not-found-action' }], + output: { + result: '{{ steps.test.output.testOutput }}', + }, + parameters: {}, }, - parameters: {}, }); const task = await broker.claim(); diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/index.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/index.ts index 47507b8c9c..be1eaef643 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/index.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/index.ts @@ -36,5 +36,7 @@ export type { TaskContext, TaskStore, DispatchResult, + TaskBrokerDispatchResult, + TaskBrokerDispatchOptions, TemplateMetadata, } from './types'; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts index abb3fe45ef..147fa359d2 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts @@ -74,7 +74,7 @@ export type CompletedTaskState = TaskCompletionState; export type SerializedTask = { id: string; spec: TaskSpec; - status: Status; + status: TaskStatus; createdAt: string; lastHeartbeatAt?: string; secrets?: TaskSecrets; @@ -110,14 +110,31 @@ export type TaskSecrets = Record & { }; /** - * DispatchResult + * The result of the TaskBroker Dispatch * * @public */ -export type DispatchResult = { +export type TaskBrokerDispatchResult = { taskId: string; }; +/** + * The options passed to TaskBroker Dispatch + * Currently a spec and optional secrets + * + * @public + */ +export type TaskBrokerDispatchOptions = { + spec: TaskSpec; + secrets?: TaskSecrets; +}; + +/** + * DispatchResult + * @deprecated use TaskBrokerDispatchResult instead + */ +export type DispatchResult = TaskBrokerDispatchResult; + /** * Task * @@ -128,7 +145,7 @@ export interface TaskContext { secrets?: TaskSecrets; done: boolean; emitLog(message: string, metadata?: JsonValue): Promise; - complete(result: CompletedTaskState, metadata?: JsonValue): Promise; + complete(result: TaskCompletionState, metadata?: JsonValue): Promise; getWorkspaceName(): Promise; } @@ -139,7 +156,9 @@ export interface TaskContext { */ export interface TaskBroker { claim(): Promise; - dispatch(spec: TaskSpec, secrets?: TaskSecrets): Promise; + dispatch( + options: TaskBrokerDispatchOptions, + ): Promise; vacuumTasks(timeoutS: { timeoutS: number }): Promise; observe( options: { @@ -174,6 +193,23 @@ export type TaskStoreListEventsOptions = { after?: number | undefined; }; +/** + * The options passed to TaskStore create + * @public + */ +export type TaskStoreCreateTaskOptions = { + spec: TaskSpec; + secrets?: TaskSecrets; +}; + +/** + * The response from TaskStoreCreate + * @public + */ +export type TaskStoreCreateTaskResult = { + taskId: string; +}; + /** * TaskStore * @@ -181,14 +217,13 @@ export type TaskStoreListEventsOptions = { */ export interface TaskStore { createTask( - task: TaskSpec, - secrets?: TaskSecrets, - ): Promise<{ taskId: string }>; + options: TaskStoreCreateTaskOptions, + ): Promise; getTask(taskId: string): Promise; claimTask(): Promise; completeTask(options: { taskId: string; - status: Status; + status: TaskStatus; eventBody: JsonObject; }): Promise; heartbeatTask(taskId: string): Promise; diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index 8a3645cf31..371fc47dc2 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -266,9 +266,12 @@ export async function createRouter( ); } - const result = await taskBroker.dispatch(taskSpec, { - ...req.body.secrets, - backstageToken: token, + const result = await taskBroker.dispatch({ + spec: taskSpec, + secrets: { + ...req.body.secrets, + backstageToken: token, + }, }); res.status(201).json({ id: result.taskId });