From 77b0f086b6ef3a91191dd21264bde487a28d93a5 Mon Sep 17 00:00:00 2001 From: Riley Bauer Date: Mon, 5 Nov 2018 20:00:51 -0800 Subject: [PATCH 1/5] Updates the frontend to correctly parse the new format of conditional pipelines --- .../mock-conditional-template.yaml | 67 ++++--------- frontend/src/lib/StaticGraphParser.ts | 93 ++++--------------- frontend/third_party/argo-ui/argo_template.ts | 8 ++ 3 files changed, 42 insertions(+), 126 deletions(-) diff --git a/frontend/mock-backend/mock-conditional-template.yaml b/frontend/mock-backend/mock-conditional-template.yaml index c10c1a0addd..501c622f4a5 100644 --- a/frontend/mock-backend/mock-conditional-template.yaml +++ b/frontend/mock-backend/mock-conditional-template.yaml @@ -15,24 +15,13 @@ apiVersion: argoproj.io/v1alpha1 kind: Workflow metadata: - generateName: pipelineflipcoin- + generateName: pipeline-flip-coin- spec: arguments: parameters: [] - entrypoint: pipelineflipcoin + entrypoint: pipeline-flip-coin + serviceAccountName: pipeline-runner templates: - - inputs: - parameters: - - name: flip-output - name: condition-1 - steps: - - - arguments: - parameters: - - name: flip-output - value: '{{inputs.parameters.flip-output}}' - name: condition-1-child - template: condition-1-child - when: '{{inputs.parameters.flip-output}} == heads' - dag: tasks: - arguments: @@ -41,10 +30,11 @@ spec: value: '{{tasks.flip-again.outputs.parameters.flip-again-output}}' - name: flip-output value: '{{inputs.parameters.flip-output}}' - dependencies: - - flip-again name: condition-2 template: condition-2 + dependencies: + - flip-again + when: '{{tasks.flip-again.outputs.parameters.flip-again-output}} == tails' - arguments: parameters: - name: flip-output @@ -54,22 +44,7 @@ spec: inputs: parameters: - name: flip-output - name: condition-1-child - - inputs: - parameters: - - name: flip-again-output - - name: flip-output - name: condition-2 - steps: - - - arguments: - parameters: - - name: flip-again-output - value: '{{inputs.parameters.flip-again-output}}' - - name: flip-output - value: '{{inputs.parameters.flip-output}}' - name: condition-2-child - template: condition-2-child - when: '{{inputs.parameters.flip-again-output}} == tails' + name: condition-1 - dag: tasks: - arguments: @@ -84,19 +59,7 @@ spec: parameters: - name: flip-again-output - name: flip-output - name: condition-2-child - - inputs: - parameters: - - name: flip-output - name: condition-3 - steps: - - - arguments: - parameters: - - name: flip-output - value: '{{inputs.parameters.flip-output}}' - name: condition-3-child - template: condition-3-child - when: '{{inputs.parameters.flip-output}} == tails' + name: condition-2 - dag: tasks: - arguments: @@ -108,7 +71,7 @@ spec: inputs: parameters: - name: flip-output - name: condition-3-child + name: condition-3 - container: args: - python -c "import random; result = 'heads' if random.randint(0,1) == 0 else @@ -143,21 +106,23 @@ spec: parameters: - name: flip-output value: '{{tasks.flip.outputs.parameters.flip-output}}' - dependencies: - - flip name: condition-1 template: condition-1 + dependencies: + - flip + when: '{{tasks.flip.outputs.parameters.flip-output}} == heads' - arguments: parameters: - name: flip-output value: '{{tasks.flip.outputs.parameters.flip-output}}' - dependencies: - - flip name: condition-3 template: condition-3 + dependencies: + - flip + when: '{{tasks.flip.outputs.parameters.flip-output}} == tails' - name: flip template: flip - name: pipelineflipcoin + name: pipeline-flip-coin - container: command: - echo diff --git a/frontend/src/lib/StaticGraphParser.ts b/frontend/src/lib/StaticGraphParser.ts index 3f2d68a9861..4f1d338ba45 100644 --- a/frontend/src/lib/StaticGraphParser.ts +++ b/frontend/src/lib/StaticGraphParser.ts @@ -15,7 +15,7 @@ */ import * as dagre from 'dagre'; -import { Workflow, WorkflowStep } from '../../third_party/argo-ui/argo_template'; +import { Workflow } from '../../third_party/argo-ui/argo_template'; export interface SelectedNodeInfo { nodeType: 'container' | 'steps' | 'unknown'; @@ -46,12 +46,7 @@ export function createGraph(workflow: Workflow): dagre.graphlib.Graph { const workflowTemplates = workflow.spec.templates; - const nonEntryPointTemplatesAndDagTasks = new Map(); - const templatesAndSteps: Array<[string, string]> = []; - - // Iterate through the workflow's templates to gather the information needed to construct the - // graph. Some information is stored in the above variables because Containers, Dags, and Steps - // are all templates, and we do not know in which order we will encounter them while iterating. + // Iterate through the workflow's templates to construct the graph for (const template of workflowTemplates) { // Argo allows specifying a single global exit handler. We also highlight that node. if (template.name === workflow.spec.onExit) { @@ -63,85 +58,33 @@ export function createGraph(workflow: Workflow): dagre.graphlib.Graph { }); } - // Steps are only used by the DSL as a workaround for handling conditionals because DAGs are not - // sufficient. (TODO: why?) We keep track of each template and its step(s) so that we can draw - // edges from the DAG task with that template name to its grandchild. This template/DAG task - // will always be a conditional, so we can use this list of tuples to do additional formatting - // of the node as desired. - if (template.steps) { - if (template.steps.length > 1 || template.steps[0].length > 1) { - throw new Error('Pipeline had template with multiple steps. Was this Pipeline compiled?'); - } - const step = template.steps[0]; - // Though the types system says 'steps' is WorkflowStep[][], sometimes the inner elements - // are just WorkflowStep, not WorkflowStep[], but we still want to treat them the same. - (step.length ? step : [step as WorkflowStep]).forEach((s) => { - templatesAndSteps.push([template.name, s.name!]); - }); - } - // DAGs are the main component making up a Pipeline, and each DAG is composed of tasks. - // TODO: add tests for the assertions below - // A compiled Pipeline will only have multiple DAGs if: - // 1) it defines an exit-handler within the DSL, which will result in its own DAG wrapping the - // entirety of the Pipeline. - // 2) it includes conditionals. In this case, there will also be Steps templates corresponding - // to those conditions, and each Steps template will have a child DAG representing the rest - // of the Pipeline from that conditional on. if (template.dag && template.dag.tasks) { template.dag.tasks.forEach((task) => { - g.setNode(task.name, { - height: NODE_HEIGHT, - label: task.name, - width: NODE_WIDTH, - }); + // tslint:disable-next-line:no-console + console.log('task', task); + if (!task.name.startsWith('exit-handler')) { + g.setNode(task.name, { + bgColor: task.when ? 'cornsilk' : undefined, + height: NODE_HEIGHT, + label: task.name, + width: NODE_WIDTH, + }); + } + + if (template.name !== workflow.spec.entrypoint && !template.name.startsWith('exit-handler')) { + g.setEdge(template.name, task.name); + } // DAG tasks can indicate dependencies which are graphically shown as parents with edges // pointing to their children (the task(s)). (task.dependencies || []).forEach((dep) => g.setEdge(dep, task.name)); - - // A Pipeline may contain multiple DAGs, but the Pipeline will only have a single entry - // point. - // - // The first task of the entry point DAG can be thought of as the root of the Pipeline. - // TODO: add tests for the assertion below - // Compiled Pipelines should always have exactly 1 root. - // - // For DAGs that do not contain the entry point, we keep track of all tasks which don't have - // dependencies because they are the roots of that DAG (note that a DAG may have multiple - // roots). These roots are important for stitching together the overall Pipeline graph, but - // they are not relevant themselves to the user. - // - // The non-root tasks within these DAGs, however, are important parts of the Pipeline. We - // keep a map of DAG templates to their non-root tasks so that we can draw edges between - // the tasks and their grandparents. - // - // We also use the map to remove any of the templates that weren't already skipped for being - // step templates. - if (template.name !== workflow.spec.entrypoint - && (!task.dependencies || !task.dependencies.length)) { - nonEntryPointTemplatesAndDagTasks.set(template.name, task.name); - } }); } } - // TODO: do we need to worry about rewiring from parents to children here? - // Remove all non-entry-point DAG templates. They are artifacts of the compilation system and not - // useful to users. - nonEntryPointTemplatesAndDagTasks.forEach((_, k) => g.removeNode(k)); - - // Connect conditionals to their grandchildren; the tasks of non-entry-point DAGs. - // We can also apply any desired formatting to the conditional nodes here. - templatesAndSteps.forEach((tuple) => { - // Add styling for conditional nodes - g.node(tuple[0]).bgColor = 'cornsilk'; - - const grandchild = nonEntryPointTemplatesAndDagTasks.get(tuple[1]); - if (grandchild) { - g.setEdge(tuple[0], grandchild); - } - }); + // tslint:disable-next-line:no-console + console.log(g); // DSL-compiled Pipelines will always include a DAG, so they should never reach this point. // It is, however, possible for users to upload manually constructed Pipelines, and extremely diff --git a/frontend/third_party/argo-ui/argo_template.ts b/frontend/third_party/argo-ui/argo_template.ts index c6cce608990..0156c7e069f 100644 --- a/frontend/third_party/argo-ui/argo_template.ts +++ b/frontend/third_party/argo-ui/argo_template.ts @@ -869,6 +869,14 @@ export interface DAGTask { * Dependencies are name of other targets which this depends on */ dependencies: string[]; + + // TODO: This exists in https://github.com/argoproj/argo/blob/master/api/openapi-spec/swagger.json + // but not in https://github.com/argoproj/argo-ui/blob/master/src/models/workflows.ts + // Perhaps we should generate this definition file from the swagger? + /** + * When is an expression in which the task should conditionally execute + */ + when?: string; } /** From b01a8105c28f6282c0de4c18f774961168e5be27 Mon Sep 17 00:00:00 2001 From: Riley Bauer Date: Mon, 12 Nov 2018 15:44:54 -0800 Subject: [PATCH 2/5] WIP - Assume tasks and templates don't share names --- .../mock-conditional-template.yaml | 14 +-- frontend/src/lib/StaticGraphParser.test.ts | 104 +++++++++--------- frontend/src/lib/StaticGraphParser.ts | 48 +++++--- frontend/src/pages/PipelineDetails.tsx | 55 ++++++--- 4 files changed, 131 insertions(+), 90 deletions(-) diff --git a/frontend/mock-backend/mock-conditional-template.yaml b/frontend/mock-backend/mock-conditional-template.yaml index 501c622f4a5..23d8c26fd35 100644 --- a/frontend/mock-backend/mock-conditional-template.yaml +++ b/frontend/mock-backend/mock-conditional-template.yaml @@ -30,7 +30,7 @@ spec: value: '{{tasks.flip-again.outputs.parameters.flip-again-output}}' - name: flip-output value: '{{inputs.parameters.flip-output}}' - name: condition-2 + name: condition-2-task template: condition-2 dependencies: - flip-again @@ -39,7 +39,7 @@ spec: parameters: - name: flip-output value: '{{inputs.parameters.flip-output}}' - name: flip-again + name: flip-again-task template: flip-again inputs: parameters: @@ -53,7 +53,7 @@ spec: value: '{{inputs.parameters.flip-again-output}}' - name: flip-output value: '{{inputs.parameters.flip-output}}' - name: print1 + name: print1-task template: print1 inputs: parameters: @@ -66,7 +66,7 @@ spec: parameters: - name: flip-output value: '{{inputs.parameters.flip-output}}' - name: print2 + name: print2-task template: print2 inputs: parameters: @@ -106,7 +106,7 @@ spec: parameters: - name: flip-output value: '{{tasks.flip.outputs.parameters.flip-output}}' - name: condition-1 + name: condition-1-task template: condition-1 dependencies: - flip @@ -115,12 +115,12 @@ spec: parameters: - name: flip-output value: '{{tasks.flip.outputs.parameters.flip-output}}' - name: condition-3 + name: condition-3-task template: condition-3 dependencies: - flip when: '{{tasks.flip.outputs.parameters.flip-output}} == tails' - - name: flip + - name: flip-task template: flip name: pipeline-flip-coin - container: diff --git a/frontend/src/lib/StaticGraphParser.test.ts b/frontend/src/lib/StaticGraphParser.test.ts index 89331f8be8a..c1504546557 100644 --- a/frontend/src/lib/StaticGraphParser.test.ts +++ b/frontend/src/lib/StaticGraphParser.test.ts @@ -498,64 +498,64 @@ describe('StaticGraphParser', () => { }); it('Returns nodeType \'steps\' when node template has steps', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - name: 'template-1', - steps: [[ - 'something', - ]] - }, - ], - }, - } as any; - const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - expect(nodeInfo.nodeType).toBe('steps'); - expect(nodeInfo.stepsInfo).toEqual({ conditional: '', parameters: [[]] }); + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // name: 'template-1', + // steps: [[ + // 'something', + // ]] + // }, + // ], + // }, + // } as any; + // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); + // expect(nodeInfo.nodeType).toBe('steps'); + // expect(nodeInfo.stepsInfo).toEqual({ conditional: '', parameters: [[]] }); }); it('Returns nodeInfo with step template\'s conditional when node template has \'when\' property', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - name: 'template-1', - steps: [[ { when: '{{someVar}} == something' } ]] - }, - ], - }, - } as any; - const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - expect(nodeInfo.nodeType).toBe('steps'); - expect(nodeInfo.stepsInfo).toEqual({ conditional: '{{someVar}} == something', parameters: [[]] }); + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // name: 'template-1', + // steps: [[ { when: '{{someVar}} == something' } ]] + // }, + // ], + // }, + // } as any; + // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); + // expect(nodeInfo.nodeType).toBe('steps'); + // expect(nodeInfo.stepsInfo).toEqual({ conditional: '{{someVar}} == something', parameters: [[]] }); }); it('Returns nodeInfo with step template\'s arguments', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - name: 'template-1', - steps: [[{ - arguments: { - parameters: [ - { name: 'param1', value: 'val1' }, - { name: 'param2', value: 'val2' }, - ], - }, - }]] - }, - ], - }, - } as any; - const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - expect(nodeInfo.nodeType).toBe('steps'); - expect(nodeInfo.stepsInfo) - .toEqual({ conditional: '', parameters: [['param1', 'val1'], ['param2', 'val2']] }); + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // name: 'template-1', + // steps: [[{ + // arguments: { + // parameters: [ + // { name: 'param1', value: 'val1' }, + // { name: 'param2', value: 'val2' }, + // ], + // }, + // }]] + // }, + // ], + // }, + // } as any; + // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); + // expect(nodeInfo.nodeType).toBe('steps'); + // expect(nodeInfo.stepsInfo) + // .toEqual({ conditional: '', parameters: [['param1', 'val1'], ['param2', 'val2']] }); }); }); diff --git a/frontend/src/lib/StaticGraphParser.ts b/frontend/src/lib/StaticGraphParser.ts index 4f1d338ba45..60a3bcf66b5 100644 --- a/frontend/src/lib/StaticGraphParser.ts +++ b/frontend/src/lib/StaticGraphParser.ts @@ -17,8 +17,13 @@ import * as dagre from 'dagre'; import { Workflow } from '../../third_party/argo-ui/argo_template'; +interface ConditionalInfo { + condition: string; + taskName: string; +} + export interface SelectedNodeInfo { - nodeType: 'container' | 'steps' | 'unknown'; + nodeType: 'container' | 'dag' | 'unknown'; containerInfo?: { args: string[]; command: string[]; @@ -26,9 +31,8 @@ export interface SelectedNodeInfo { inputs: string[][]; outputs: string[][]; }; - stepsInfo?: { - conditional: string; - parameters: string[][]; + dagInfo?: { + conditionalTasks: ConditionalInfo[]; }; } @@ -58,8 +62,15 @@ export function createGraph(workflow: Workflow): dagre.graphlib.Graph { }); } + // TODO remove + // g.setNode(template.name, { + // height: NODE_HEIGHT, + // label: template.name, + // width: NODE_WIDTH, + // }); // DAGs are the main component making up a Pipeline, and each DAG is composed of tasks. if (template.dag && template.dag.tasks) { + template.dag.tasks.forEach((task) => { // tslint:disable-next-line:no-console console.log('task', task); @@ -72,13 +83,16 @@ export function createGraph(workflow: Workflow): dagre.graphlib.Graph { }); } + // TODO remove + // g.setEdge(task.name, task.template); + if (template.name !== workflow.spec.entrypoint && !template.name.startsWith('exit-handler')) { - g.setEdge(template.name, task.name); + // g.setEdge(template.name, task.name); } // DAG tasks can indicate dependencies which are graphically shown as parents with edges // pointing to their children (the task(s)). - (task.dependencies || []).forEach((dep) => g.setEdge(dep, task.name)); + // (task.dependencies || []).forEach((dep) => g.setEdge(dep, task.name)); }); } } @@ -135,18 +149,16 @@ export function getNodeInfo(workflow?: Workflow, nodeId?: string): SelectedNodeI return [p.name, value]; }); } - } else if (template.steps && template.steps[0] && template.steps[0].length) { - // 'template.steps' represent conditionals. - // There should only ever be 1 WorkflowStep in a steps template, and it should have a 'when'. - info.nodeType = 'steps'; - info.stepsInfo = { conditional: '', parameters: [[]] }; - - info.stepsInfo.conditional = template.steps[0][0].when || ''; - - if (template.steps[0][0].arguments) { - info.stepsInfo.parameters = - (template.steps[0][0].arguments!.parameters || []).map(p => [p.name, p.value || '']); - } + } else if (template.dag && template.dag.tasks) { + // Conditionals are represented by DAG tasks with 'when' properties. + info.nodeType = 'dag'; + const conditionalTasks: ConditionalInfo[] = []; + template.dag.tasks.forEach((task) => { + if (task.when) { + conditionalTasks.push({ condition: task.when, taskName: task.name }); + } + }); + info.dagInfo = { conditionalTasks }; } } return info; diff --git a/frontend/src/pages/PipelineDetails.tsx b/frontend/src/pages/PipelineDetails.tsx index 454f2630b9f..bda1a231e1d 100644 --- a/frontend/src/pages/PipelineDetails.tsx +++ b/frontend/src/pages/PipelineDetails.tsx @@ -38,7 +38,7 @@ import { URLParser, QUERY_PARAMS } from '../lib/URLParser'; import { UnControlled as CodeMirror } from 'react-codemirror2'; import { Workflow } from '../../third_party/argo-ui/argo_template'; import { classes, stylesheet } from 'typestyle'; -import { color, commonCss, padding } from '../Css'; +import { color, commonCss, padding, fontsize } from '../Css'; import { logger, errorToMessage } from '../lib/Utils'; interface PipelineDetailsState { @@ -74,6 +74,9 @@ const css = stylesheet({ }, background: '#f7f7f7', }, + fontSizeTitle: { + fontSize: fontsize.title, + }, nodeName: { flexGrow: 1, textAlign: 'center', @@ -104,6 +107,28 @@ const css = stylesheet({ color: color.strong, marginTop: 10, }, + task: { + paddingLeft: 10, + }, + taskInfo: { + paddingLeft: 10, + }, + taskInfoHeader: { + fontSize: 15, + fontWeight: 'bold', + paddingBottom: 16, + paddingTop: 20, + }, + taskName: { + fontSize: fontsize.large, + fontWeight: 'bold', + paddingTop: 20, + }, + taskTitle: { + fontSize: fontsize.title, + fontWeight: 'bold', + paddingTop: 20, + }, }); class PipelineDetails extends Page<{}, PipelineDetailsState> { @@ -316,33 +341,37 @@ class PipelineDetails extends Page<{}, PipelineDetailsState> { // TODO: The headers for these DetailsTables should just be a part of DetailsTables nodeInfoJsx =
-
Input parameters
+
Input parameters
-
Output parameters
+
Output parameters
-
Arguments
+
Arguments
{nodeInfo.containerInfo.args.map((arg, i) =>
{arg}
)} -
Command
+
Command
{nodeInfo.containerInfo.command.map((c, i) =>
{c}
)} -
Image
+
Image
{nodeInfo.containerInfo.image}
; } break; - case 'steps': - if (nodeInfo.stepsInfo) { + case 'dag': + if (nodeInfo.dagInfo) { nodeInfoJsx =
-
Conditional
-
{nodeInfo.stepsInfo.conditional}
- -
Parameters
- + {nodeInfo.dagInfo.conditionalTasks && !!nodeInfo.dagInfo.conditionalTasks.length && ( +
Conditionals
+ )} + {nodeInfo.dagInfo.conditionalTasks.map((conditionalTask, i) => +
+
When: {conditionalTask.condition}
+
Run task: {conditionalTask.taskName}
+
+ )}
; } break; From d556ca86f2d9d695da25c8002d0eedf1629db35d Mon Sep 17 00:00:00 2001 From: Riley Bauer Date: Mon, 12 Nov 2018 17:53:40 -0800 Subject: [PATCH 3/5] Greatly simplifies graphing of conditional and non-conditional pipelines --- .../mock-conditional-template.yaml | 18 +- frontend/src/lib/StaticGraphParser.test.ts | 566 +++++++++--------- frontend/src/lib/StaticGraphParser.ts | 254 ++++---- frontend/src/pages/PipelineDetails.tsx | 83 ++- 4 files changed, 476 insertions(+), 445 deletions(-) diff --git a/frontend/mock-backend/mock-conditional-template.yaml b/frontend/mock-backend/mock-conditional-template.yaml index 23d8c26fd35..5bb546326d8 100644 --- a/frontend/mock-backend/mock-conditional-template.yaml +++ b/frontend/mock-backend/mock-conditional-template.yaml @@ -27,14 +27,14 @@ spec: - arguments: parameters: - name: flip-again-output - value: '{{tasks.flip-again.outputs.parameters.flip-again-output}}' + value: '{{tasks.flip-again-task.outputs.parameters.flip-again-output}}' - name: flip-output value: '{{inputs.parameters.flip-output}}' name: condition-2-task template: condition-2 dependencies: - - flip-again - when: '{{tasks.flip-again.outputs.parameters.flip-again-output}} == tails' + - flip-again-task + when: '{{tasks.flip-again-task.outputs.parameters.flip-again-output}} == tails' - arguments: parameters: - name: flip-output @@ -105,21 +105,21 @@ spec: - arguments: parameters: - name: flip-output - value: '{{tasks.flip.outputs.parameters.flip-output}}' + value: '{{tasks.flip-task.outputs.parameters.flip-output}}' name: condition-1-task template: condition-1 dependencies: - - flip - when: '{{tasks.flip.outputs.parameters.flip-output}} == heads' + - flip-task + when: '{{tasks.flip-task.outputs.parameters.flip-output}} == heads' - arguments: parameters: - name: flip-output - value: '{{tasks.flip.outputs.parameters.flip-output}}' + value: '{{tasks.flip-task.outputs.parameters.flip-output}}' name: condition-3-task template: condition-3 dependencies: - - flip - when: '{{tasks.flip.outputs.parameters.flip-output}} == tails' + - flip-task + when: '{{tasks.flip-task.outputs.parameters.flip-output}} == tails' - name: flip-task template: flip name: pipeline-flip-coin diff --git a/frontend/src/lib/StaticGraphParser.test.ts b/frontend/src/lib/StaticGraphParser.test.ts index c1504546557..b82ef63d76b 100644 --- a/frontend/src/lib/StaticGraphParser.test.ts +++ b/frontend/src/lib/StaticGraphParser.test.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { createGraph, getNodeInfo, SelectedNodeInfo } from './StaticGraphParser'; +import { createGraph } from './StaticGraphParser'; describe('StaticGraphParser', () => { @@ -243,280 +243,280 @@ describe('StaticGraphParser', () => { }); }); - describe('getNodeInfo', () => { - it('Returns null if nodeId is undefined', () => { - const nodeInfo = getNodeInfo(newWorkflow(), undefined); - expect(nodeInfo).toBeNull(); - }); - - it('Returns null if nodeId is empty', () => { - const nodeInfo = getNodeInfo(newWorkflow(), ''); - expect(nodeInfo).toBeNull(); - }); - - it('Returns null if workflow is undefined', () => { - const nodeInfo = getNodeInfo(undefined, 'someId'); - expect(nodeInfo).toBeNull(); - }); - - it('Returns null if workflow.spec is undefined', () => { - const workflow = newWorkflow(); - workflow.spec = undefined; - const nodeInfo = getNodeInfo(workflow, 'someId'); - expect(nodeInfo).toBeNull(); - }); - - it('Returns null if workflow.spec.templates is undefined', () => { - const workflow = newWorkflow(); - workflow.spec.templates = undefined; - const nodeInfo = getNodeInfo(workflow, 'someId'); - expect(nodeInfo).toBeNull(); - }); - - it('Returns nodeInfo containing only \'unknown\' nodeType if no template matches provided ID', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - container: {}, - name: 'template-1', - }, - ], - }, - } as any; - const nodeInfo = getNodeInfo(workflow, 'id-not-in-spec'); - expect(nodeInfo).toEqual({ nodeType: 'unknown' }); - }); - - it('Returns nodeInfo containing only \'unknown\' nodeType if template matching provided ID has no container or steps', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - // No container or steps here - name: 'template-1', - }, - ], - }, - } as any; - const nodeInfo = getNodeInfo(workflow, 'template-1'); - expect(nodeInfo).toEqual({ nodeType: 'unknown' }); - }); - - it('Returns nodeInfo containing only \'unknown\' nodeType if template matching provided ID has no container and empty steps', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - // No container here - name: 'template-1', - steps: [], - }, - ], - }, - } as any; - const nodeInfo = getNodeInfo(workflow, 'template-1'); - expect(nodeInfo).toEqual({ nodeType: 'unknown' }); - }); - - it('Returns nodeInfo containing only \'unknown\' nodeType if template matching provided ID has no container and array of empty steps', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - // No container here - name: 'template-1', - steps: [[]], - }, - ], - }, - } as any; - const nodeInfo = getNodeInfo(workflow, 'template-1'); - expect(nodeInfo).toEqual({ nodeType: 'unknown' }); - }); - - it('Returns nodeInfo containing container args', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - container: { - args: ['arg1', 'arg2'], - }, - name: 'template-1', - }, - ], - }, - } as any; - const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - expect(nodeInfo.nodeType).toBe('container'); - expect(nodeInfo.containerInfo!.args).toEqual(['arg1', 'arg2']); - }); - - it('Returns nodeInfo containing container commands', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - container: { - command: ['command1', 'command2'] - }, - name: 'template-1', - }, - ], - }, - } as any; - const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - expect(nodeInfo.nodeType).toBe('container'); - expect(nodeInfo.containerInfo!.command).toEqual(['command1', 'command2']); - }); - - it('Returns nodeInfo containing container image name', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - container: { - image: 'someImageID' - }, - name: 'template-1', - }, - ], - }, - } as any; - const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - expect(nodeInfo.nodeType).toBe('container'); - expect(nodeInfo.containerInfo!.image).toBe('someImageID'); - }); - - it('Returns nodeInfo containing container inputs as list of name/value tuples', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - container: {}, - inputs: { - parameters: [ - { name: 'param1', value: 'val1' }, - { name: 'param2', value: 'val2' }, - ], - }, - name: 'template-1', - }, - ], - }, - } as any; - const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - expect(nodeInfo.nodeType).toBe('container'); - expect(nodeInfo.containerInfo!.inputs).toEqual([['param1', 'val1'], ['param2', 'val2']]); - }); - - it('Returns empty strings for inputs with no specified value', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - container: {}, - inputs: { - parameters: [ - { name: 'param1' }, - { name: 'param2' }, - ], - }, - name: 'template-1', - }, - ], - }, - } as any; - const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - expect(nodeInfo.nodeType).toBe('container'); - expect(nodeInfo.containerInfo!.inputs).toEqual([['param1', ''], ['param2', '']]); - }); - - it('Returns nodeInfo containing container outputs as list of name/value tuples, pulling from valueFrom if necessary', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - container: {}, - name: 'template-1', - outputs: { - parameters: [ - { name: 'param1', value: 'val1' }, - { name: 'param2', valueFrom: { jsonPath: 'jsonPath' } }, - { name: 'param3', valueFrom: { path: 'path' } }, - { name: 'param4', valueFrom: { parameter: 'parameterReference' } }, - { name: 'param5', valueFrom: { jqFilter: 'jqFilter' } }, - ], - }, - }, - ], - }, - } as any; - const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - expect(nodeInfo.nodeType).toBe('container'); - expect(nodeInfo.containerInfo!.outputs).toEqual([ - ['param1', 'val1'], - ['param2', 'jsonPath'], - ['param3', 'path'], - ['param4', 'parameterReference'], - ['param5', 'jqFilter'], - ]); - }); - - it('Returns empty strings for outputs with no specified value', () => { - const workflow = { - spec: { - entrypoint: 'template-1', - templates: [ - { - container: {}, - name: 'template-1', - outputs: { - parameters: [ - { name: 'param1' }, - { name: 'param2' }, - ], - }, - }, - ], - }, - } as any; - const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - expect(nodeInfo.nodeType).toBe('container'); - expect(nodeInfo.containerInfo!.outputs).toEqual([['param1', ''], ['param2', '']]); - }); - - it('Returns nodeType \'steps\' when node template has steps', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // name: 'template-1', - // steps: [[ - // 'something', - // ]] - // }, - // ], - // }, - // } as any; - // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - // expect(nodeInfo.nodeType).toBe('steps'); - // expect(nodeInfo.stepsInfo).toEqual({ conditional: '', parameters: [[]] }); - }); - - it('Returns nodeInfo with step template\'s conditional when node template has \'when\' property', () => { + // describe('getNodeInfo', () => { + // it('Returns null if nodeId is undefined', () => { + // const nodeInfo = getNodeInfo(newWorkflow(), undefined); + // expect(nodeInfo).toBeNull(); + // }); + + // it('Returns null if nodeId is empty', () => { + // const nodeInfo = getNodeInfo(newWorkflow(), ''); + // expect(nodeInfo).toBeNull(); + // }); + + // it('Returns null if workflow is undefined', () => { + // const nodeInfo = getNodeInfo(undefined, 'someId'); + // expect(nodeInfo).toBeNull(); + // }); + + // it('Returns null if workflow.spec is undefined', () => { + // const workflow = newWorkflow(); + // workflow.spec = undefined; + // const nodeInfo = getNodeInfo(workflow, 'someId'); + // expect(nodeInfo).toBeNull(); + // }); + + // it('Returns null if workflow.spec.templates is undefined', () => { + // const workflow = newWorkflow(); + // workflow.spec.templates = undefined; + // const nodeInfo = getNodeInfo(workflow, 'someId'); + // expect(nodeInfo).toBeNull(); + // }); + + // it('Returns nodeInfo containing only \'unknown\' nodeType if no template matches provided ID', () => { + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // container: {}, + // name: 'template-1', + // }, + // ], + // }, + // } as any; + // const nodeInfo = getNodeInfo(workflow, 'id-not-in-spec'); + // expect(nodeInfo).toEqual({ nodeType: 'unknown' }); + // }); + + // it('Returns nodeInfo containing only \'unknown\' nodeType if template matching provided ID has no container or steps', () => { + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // // No container or steps here + // name: 'template-1', + // }, + // ], + // }, + // } as any; + // const nodeInfo = getNodeInfo(workflow, 'template-1'); + // expect(nodeInfo).toEqual({ nodeType: 'unknown' }); + // }); + + // it('Returns nodeInfo containing only \'unknown\' nodeType if template matching provided ID has no container and empty steps', () => { + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // // No container here + // name: 'template-1', + // steps: [], + // }, + // ], + // }, + // } as any; + // const nodeInfo = getNodeInfo(workflow, 'template-1'); + // expect(nodeInfo).toEqual({ nodeType: 'unknown' }); + // }); + + // it('Returns nodeInfo containing only \'unknown\' nodeType if template matching provided ID has no container and array of empty steps', () => { + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // // No container here + // name: 'template-1', + // steps: [[]], + // }, + // ], + // }, + // } as any; + // const nodeInfo = getNodeInfo(workflow, 'template-1'); + // expect(nodeInfo).toEqual({ nodeType: 'unknown' }); + // }); + + // it('Returns nodeInfo containing container args', () => { + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // container: { + // args: ['arg1', 'arg2'], + // }, + // name: 'template-1', + // }, + // ], + // }, + // } as any; + // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); + // expect(nodeInfo.nodeType).toBe('container'); + // expect(nodeInfo.containerInfo!.args).toEqual(['arg1', 'arg2']); + // }); + + // it('Returns nodeInfo containing container commands', () => { + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // container: { + // command: ['command1', 'command2'] + // }, + // name: 'template-1', + // }, + // ], + // }, + // } as any; + // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); + // expect(nodeInfo.nodeType).toBe('container'); + // expect(nodeInfo.containerInfo!.command).toEqual(['command1', 'command2']); + // }); + + // it('Returns nodeInfo containing container image name', () => { + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // container: { + // image: 'someImageID' + // }, + // name: 'template-1', + // }, + // ], + // }, + // } as any; + // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); + // expect(nodeInfo.nodeType).toBe('container'); + // expect(nodeInfo.containerInfo!.image).toBe('someImageID'); + // }); + + // it('Returns nodeInfo containing container inputs as list of name/value tuples', () => { + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // container: {}, + // inputs: { + // parameters: [ + // { name: 'param1', value: 'val1' }, + // { name: 'param2', value: 'val2' }, + // ], + // }, + // name: 'template-1', + // }, + // ], + // }, + // } as any; + // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); + // expect(nodeInfo.nodeType).toBe('container'); + // expect(nodeInfo.containerInfo!.inputs).toEqual([['param1', 'val1'], ['param2', 'val2']]); + // }); + + // it('Returns empty strings for inputs with no specified value', () => { + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // container: {}, + // inputs: { + // parameters: [ + // { name: 'param1' }, + // { name: 'param2' }, + // ], + // }, + // name: 'template-1', + // }, + // ], + // }, + // } as any; + // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); + // expect(nodeInfo.nodeType).toBe('container'); + // expect(nodeInfo.containerInfo!.inputs).toEqual([['param1', ''], ['param2', '']]); + // }); + + // it('Returns nodeInfo containing container outputs as list of name/value tuples, pulling from valueFrom if necessary', () => { + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // container: {}, + // name: 'template-1', + // outputs: { + // parameters: [ + // { name: 'param1', value: 'val1' }, + // { name: 'param2', valueFrom: { jsonPath: 'jsonPath' } }, + // { name: 'param3', valueFrom: { path: 'path' } }, + // { name: 'param4', valueFrom: { parameter: 'parameterReference' } }, + // { name: 'param5', valueFrom: { jqFilter: 'jqFilter' } }, + // ], + // }, + // }, + // ], + // }, + // } as any; + // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); + // expect(nodeInfo.nodeType).toBe('container'); + // expect(nodeInfo.containerInfo!.outputs).toEqual([ + // ['param1', 'val1'], + // ['param2', 'jsonPath'], + // ['param3', 'path'], + // ['param4', 'parameterReference'], + // ['param5', 'jqFilter'], + // ]); + // }); + + // it('Returns empty strings for outputs with no specified value', () => { + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // container: {}, + // name: 'template-1', + // outputs: { + // parameters: [ + // { name: 'param1' }, + // { name: 'param2' }, + // ], + // }, + // }, + // ], + // }, + // } as any; + // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); + // expect(nodeInfo.nodeType).toBe('container'); + // expect(nodeInfo.containerInfo!.outputs).toEqual([['param1', ''], ['param2', '']]); + // }); + + // it('Returns nodeType \'steps\' when node template has steps', () => { + // const workflow = { + // spec: { + // entrypoint: 'template-1', + // templates: [ + // { + // name: 'template-1', + // steps: [[ + // 'something', + // ]] + // }, + // ], + // }, + // } as any; + // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); + // expect(nodeInfo.nodeType).toBe('steps'); + // expect(nodeInfo.stepsInfo).toEqual({ conditional: '', parameters: [[]] }); + // }); + + // it('Returns nodeInfo with step template\'s conditional when node template has \'when\' property', () => { // const workflow = { // spec: { // entrypoint: 'template-1', @@ -531,9 +531,9 @@ describe('StaticGraphParser', () => { // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); // expect(nodeInfo.nodeType).toBe('steps'); // expect(nodeInfo.stepsInfo).toEqual({ conditional: '{{someVar}} == something', parameters: [[]] }); - }); + // }); - it('Returns nodeInfo with step template\'s arguments', () => { + // it('Returns nodeInfo with step template\'s arguments', () => { // const workflow = { // spec: { // entrypoint: 'template-1', @@ -556,11 +556,11 @@ describe('StaticGraphParser', () => { // expect(nodeInfo.nodeType).toBe('steps'); // expect(nodeInfo.stepsInfo) // .toEqual({ conditional: '', parameters: [['param1', 'val1'], ['param2', 'val2']] }); - }); - }); + // }); + // }); - function verifyNodeInfoNotNull(nodeInfo: SelectedNodeInfo | null): SelectedNodeInfo { - expect(nodeInfo).not.toBeNull(); - return nodeInfo!; - } + // function verifyNodeInfoNotNull(nodeInfo: SelectedNodeInfo | null): SelectedNodeInfo { + // expect(nodeInfo).not.toBeNull(); + // return nodeInfo!; + // } }); diff --git a/frontend/src/lib/StaticGraphParser.ts b/frontend/src/lib/StaticGraphParser.ts index 60a3bcf66b5..c45864bc643 100644 --- a/frontend/src/lib/StaticGraphParser.ts +++ b/frontend/src/lib/StaticGraphParser.ts @@ -15,34 +15,142 @@ */ import * as dagre from 'dagre'; -import { Workflow } from '../../third_party/argo-ui/argo_template'; +import { Workflow, Template } from '../../third_party/argo-ui/argo_template'; +import { logger } from './Utils'; interface ConditionalInfo { condition: string; taskName: string; } -export interface SelectedNodeInfo { - nodeType: 'container' | 'dag' | 'unknown'; - containerInfo?: { - args: string[]; - command: string[]; - image: string; - inputs: string[][]; - outputs: string[][]; - }; - dagInfo?: { - conditionalTasks: ConditionalInfo[]; - }; +export type nodeType = 'container' | 'dag' | 'unknown'; + +export class SelectedNodeInfo { + public args: string[]; + public command: string[]; + public condition: string; + public conditionalTasks: ConditionalInfo[]; + public image: string; + public inputs: string[][]; + public nodeType: nodeType; + public outputs: string[][]; + + constructor() { + this.args = []; + this.command = []; + this.condition = ''; + this.conditionalTasks = []; + this.image = ''; + this.inputs = [[]]; + this.nodeType = 'unknown'; + this.outputs = [[]]; + } } -export function createGraph(workflow: Workflow): dagre.graphlib.Graph { - const g = new dagre.graphlib.Graph(); - g.setGraph({}); - g.setDefaultEdgeLabel(() => ({})); - const NODE_WIDTH = 180; - const NODE_HEIGHT = 70; +const NODE_WIDTH = 180; +const NODE_HEIGHT = 70; + +function populateInfoFromTemplate(info: SelectedNodeInfo, template: Template): SelectedNodeInfo { + if (!template.container) { + return info; + } + + info.nodeType = 'container'; + info.args = template.container.args || [], + info.command = template.container.command || [], + info.image = template.container.image || ''; + + if (template.inputs) { + info.inputs = + (template.inputs.parameters || []).map(p => [p.name, p.value || '']); + } + if (template.outputs) { + info.outputs = (template.outputs.parameters || []).map(p => { + let value = ''; + if (p.value) { + value = p.value; + } else if (p.valueFrom) { + value = p.valueFrom.jqFilter || p.valueFrom.jsonPath || p.valueFrom.parameter || p.valueFrom.path || ''; + } + return [p.name, value]; + }); + } + return info; +} + +/** + * Recursively construct the static graph of the Pipeline. + * + * @param graph The dagre graph object + * @param rootTemplateId The current template being explored at this level of recursion + * @param templates An unchanging map of template name to template and type + * @param parent The task that was being examined when the function recursed + */ +function buildDag( + graph: dagre.graphlib.Graph, + rootTemplateId: string, + templates: Map, + parent?: string): void { + + const root = templates.get(rootTemplateId); + + if (root && root.nodeType === 'dag') { + const template = root.template; + + (template.dag.tasks || []).forEach((task) => { + // If the user specifies an exit handler, then the compiler will wrap the entire Pipeline + // within an additional DAG template prefixed with "exit-handler". + // If this is the case, we simply treat it as the root of the graph and work from there + if (task.name.startsWith('exit-handler')) { + buildDag(graph, task.template, templates); + } else { + + // Parent here will be the task that pointed to this DAG template + if (parent) { + graph.setEdge(parent, task.name); + } + + // This object contains information about the node that we display to the user when they + // click on a node in the graph + const info = new SelectedNodeInfo(); + if (task.when) { + info.condition = task.when; + } + + // "Child" here is the template that this task points to. This template should either be a + // DAG, in which case we recurse, or a container which can be thought of as a leaf node. + const child = templates.get(task.template); + if (child) { + if (child.nodeType === 'dag') { + buildDag(graph, task.template, templates, task.name); + } else if (child.nodeType === 'container' ) { + populateInfoFromTemplate(info, child.template); + } else { + logger.error(`Unknown nodetype: ${child.nodeType} on template: ${child.template}`); + } + } + + graph.setNode(task.name, { + bgColor: task.when ? 'cornsilk' : undefined, + height: NODE_HEIGHT, + info, + label: task.name, + width: NODE_WIDTH, + }); + } + + // DAG tasks can indicate dependencies which are graphically shown as parents with edges + // pointing to their children (the task(s)). + (task.dependencies || []).forEach((dep) => graph.setEdge(dep, task.name)); + }); + } +} + +export function createGraph(workflow: Workflow): dagre.graphlib.Graph { + const graph = new dagre.graphlib.Graph(); + graph.setGraph({}); + graph.setDefaultEdgeLabel(() => ({})); if (!workflow.spec || !workflow.spec.templates) { throw new Error('Could not generate graph. Provided Pipeline had no components.'); @@ -50,63 +158,42 @@ export function createGraph(workflow: Workflow): dagre.graphlib.Graph { const workflowTemplates = workflow.spec.templates; - // Iterate through the workflow's templates to construct the graph - for (const template of workflowTemplates) { - // Argo allows specifying a single global exit handler. We also highlight that node. + const templates = new Map(); + + // Iterate through the workflow's templates to construct a map which will be used to traverse and + // construct the graph + for (const template of workflowTemplates.filter(t => !!t && !!t.name)) { + // Argo allows specifying a single global exit handler. We also highlight that node if (template.name === workflow.spec.onExit) { - g.setNode(template.name, { + const info = new SelectedNodeInfo(); + populateInfoFromTemplate(info, template); + graph.setNode(template.name, { bgColor: '#eee', height: NODE_HEIGHT, + info, label: 'onExit - ' + template.name, width: NODE_WIDTH, }); } - // TODO remove - // g.setNode(template.name, { - // height: NODE_HEIGHT, - // label: template.name, - // width: NODE_WIDTH, - // }); - // DAGs are the main component making up a Pipeline, and each DAG is composed of tasks. - if (template.dag && template.dag.tasks) { - - template.dag.tasks.forEach((task) => { - // tslint:disable-next-line:no-console - console.log('task', task); - if (!task.name.startsWith('exit-handler')) { - g.setNode(task.name, { - bgColor: task.when ? 'cornsilk' : undefined, - height: NODE_HEIGHT, - label: task.name, - width: NODE_WIDTH, - }); - } - - // TODO remove - // g.setEdge(task.name, task.template); - - if (template.name !== workflow.spec.entrypoint && !template.name.startsWith('exit-handler')) { - // g.setEdge(template.name, task.name); - } - - // DAG tasks can indicate dependencies which are graphically shown as parents with edges - // pointing to their children (the task(s)). - // (task.dependencies || []).forEach((dep) => g.setEdge(dep, task.name)); - }); + if (template.container) { + templates.set(template.name, { nodeType: 'container', template }); + } else if (template.dag) { + templates.set(template.name, { nodeType: 'dag', template }); + } else { + logger.verbose(`Template: ${template.name} was neither a Container nor a DAG`); } } - // tslint:disable-next-line:no-console - console.log(g); + buildDag(graph, workflow.spec.entrypoint, templates); // DSL-compiled Pipelines will always include a DAG, so they should never reach this point. // It is, however, possible for users to upload manually constructed Pipelines, and extremely // simple ones may have no steps or DAGs, just an entry point container. - if (g.nodeCount() === 0) { + if (graph.nodeCount() === 0) { const entryPointTemplate = workflowTemplates.find((t) => t.name === workflow.spec.entrypoint); if (entryPointTemplate) { - g.setNode(entryPointTemplate.name, { + graph.setNode(entryPointTemplate.name, { height: NODE_HEIGHT, label: entryPointTemplate.name, width: NODE_WIDTH, @@ -114,52 +201,5 @@ export function createGraph(workflow: Workflow): dagre.graphlib.Graph { } } - return g; -} - -export function getNodeInfo(workflow?: Workflow, nodeId?: string): SelectedNodeInfo | null { - if (!nodeId || !workflow || !workflow.spec || !workflow.spec.templates) { - return null; - } - - const info: SelectedNodeInfo = { nodeType: 'unknown' }; - const template = workflow.spec.templates.find((t) => t.name === nodeId); - if (template) { - if (template.container) { - info.nodeType = 'container'; - info.containerInfo = { - args: template.container.args || [], - command: template.container.command || [], - image: template.container.image || '', - inputs: [[]], - outputs: [[]] - }; - if (template.inputs) { - info.containerInfo.inputs = - (template.inputs.parameters || []).map(p => [p.name, p.value || '']); - } - if (template.outputs) { - info.containerInfo.outputs = (template.outputs.parameters || []).map(p => { - let value = ''; - if (p.value) { - value = p.value; - } else if (p.valueFrom) { - value = p.valueFrom.jqFilter || p.valueFrom.jsonPath || p.valueFrom.parameter || p.valueFrom.path || ''; - } - return [p.name, value]; - }); - } - } else if (template.dag && template.dag.tasks) { - // Conditionals are represented by DAG tasks with 'when' properties. - info.nodeType = 'dag'; - const conditionalTasks: ConditionalInfo[] = []; - template.dag.tasks.forEach((task) => { - if (task.when) { - conditionalTasks.push({ condition: task.when, taskName: task.name }); - } - }); - info.dagInfo = { conditionalTasks }; - } - } - return info; + return graph; } diff --git a/frontend/src/pages/PipelineDetails.tsx b/frontend/src/pages/PipelineDetails.tsx index bda1a231e1d..754a706ab03 100644 --- a/frontend/src/pages/PipelineDetails.tsx +++ b/frontend/src/pages/PipelineDetails.tsx @@ -328,58 +328,49 @@ class PipelineDetails extends Page<{}, PipelineDetailsState> { private _selectNode(id: string): void { let nodeInfoJsx: JSX.Element =
Unable to retrieve node info
; - const nodeInfo = StaticGraphParser.getNodeInfo(this.state.template, id); + const nodeInfo = this.state.graph!.node(id).info as StaticGraphParser.SelectedNodeInfo; if (!nodeInfo) { logger.error(`Node with ID: ${id} was not found in the graph`); return; } - switch (nodeInfo.nodeType) { - case 'container': - if (nodeInfo.containerInfo) { - // TODO: The headers for these DetailsTables should just be a part of DetailsTables - nodeInfoJsx = -
-
Input parameters
- - -
Output parameters
- - -
Arguments
- {nodeInfo.containerInfo.args.map((arg, i) => -
{arg}
)} - -
Command
- {nodeInfo.containerInfo.command.map((c, i) =>
{c}
)} - -
Image
-
{nodeInfo.containerInfo.image}
-
; - } - break; - case 'dag': - if (nodeInfo.dagInfo) { - nodeInfoJsx = -
- {nodeInfo.dagInfo.conditionalTasks && !!nodeInfo.dagInfo.conditionalTasks.length && ( -
Conditionals
- )} - {nodeInfo.dagInfo.conditionalTasks.map((conditionalTask, i) => -
-
When: {conditionalTask.condition}
-
Run task: {conditionalTask.taskName}
-
- )} -
; - } - break; - default: - // TODO: display using error banner within side panel. - nodeInfoJsx =
{`Node ${id} has unknown node type.`}
; - logger.error(`Node ${id} has unknown node type.`); - } + // TODO: The headers for these DetailsTables should just be a part of DetailsTables + nodeInfoJsx = +
+
Input parameters
+ + +
Output parameters
+ + +
Arguments
+ {nodeInfo.args.map((arg, i) => +
{arg}
)} + +
Command
+ {nodeInfo.command.map((c, i) =>
{c}
)} + +
Image
+
{nodeInfo.image}
+ + {!!nodeInfo.condition && ( +
+
Condition
+
Run when: {nodeInfo.condition}
+
+ )} + + {nodeInfo.conditionalTasks && !!nodeInfo.conditionalTasks.length && ( +
Conditionals
+ )} + {nodeInfo.conditionalTasks.map((conditionalTask, i) => +
+
When: {conditionalTask.condition}
+
Run task: {conditionalTask.taskName}
+
+ )} +
; this.setState({ selectedNodeId: id, From f28a8081f8ad8b8910562052cf353328bf0f153a Mon Sep 17 00:00:00 2001 From: Riley Bauer Date: Thu, 15 Nov 2018 14:30:10 -0800 Subject: [PATCH 4/5] Adds/updates StaticParser tests --- frontend/src/lib/StaticGraphParser.test.ts | 593 +++++++-------------- frontend/src/lib/StaticGraphParser.ts | 17 +- frontend/src/pages/PipelineDetails.tsx | 10 - 3 files changed, 202 insertions(+), 418 deletions(-) diff --git a/frontend/src/lib/StaticGraphParser.test.ts b/frontend/src/lib/StaticGraphParser.test.ts index b82ef63d76b..995a39bf579 100644 --- a/frontend/src/lib/StaticGraphParser.test.ts +++ b/frontend/src/lib/StaticGraphParser.test.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { createGraph } from './StaticGraphParser'; +import { createGraph, _populateInfoFromTemplate, SelectedNodeInfo } from './StaticGraphParser'; describe('StaticGraphParser', () => { @@ -36,48 +36,51 @@ describe('StaticGraphParser', () => { }; } - // In this pipeline, the conditionals are the templates: 'condition-1' and 'condition-2' - // 'condition-1-child' and 'condition-2-child' are not displayed in the static graph, but they - // are used by the parser to properly connect the nodes. function newConditionalWorkflow(): any { return { spec: { entrypoint: 'pipeline-flip-coin', templates: [ - { - name: 'condition-1', - steps: [[{ - name: 'condition-1-child', - template: 'condition-1-child', - when: '{{inputs.parameters.flip-output}} == heads' - }]] - }, - { dag: { tasks: [{ name: 'heads', template: 'heads' }] }, name: 'condition-1-child' }, - { - name: 'condition-2', - steps: [[{ - name: 'condition-2-child', - template: 'condition-2-child', - when: '{{inputs.parameters.flip-output}} == tails' - }]] - }, - { dag: { tasks: [{ name: 'tails', template: 'tails' }] }, name: 'condition-2-child' }, + // Root/entrypoint DAG { dag: { tasks: [ - { dependencies: ['flip'], name: 'condition-1', template: 'condition-1' }, - { dependencies: ['flip'], name: 'condition-2', template: 'condition-2' }, - { name: 'flip', template: 'flip' } + { name: 'flip-task', template: 'flip' }, + { + dependencies: ['flip-task'], + name: 'condition-1-task', + template: 'condition-1', + when: '{{inputs.parameters.flip-output}} == heads', + }, + { + dependencies: ['flip-task'], + name: 'condition-2-task', + template: 'condition-2', + when: '{{inputs.parameters.flip-output}} == tails' + }, ] }, - name: 'pipeline-flip-coin' + name: 'pipeline-flip-coin', }, + // Container that flips the coin and outputs the result { container: { args: [ /* omitted */], command: ['sh', '-c'], }, name: 'flip', outputs: { parameters: [{ name: 'flip-output', valueFrom: { path: '/tmp/output' } }] } }, + // DAG representing execution if result is 'heads' + { + dag: { tasks: [{ name: 'heads-task', template: 'heads' }] }, + name: 'condition-1', + }, + // Container that is run if result is 'heads' { container: { command: ['echo', '\'heads\''], }, name: 'heads', }, + // DAG representing execution if result is 'tails' + { + dag: { tasks: [{ name: 'tails-task', template: 'tails' }] }, + name: 'condition-2', + }, + // Container that is run if result is 'tails' { container: { command: ['echo', '\'tails\''], }, name: 'tails', }, ] } @@ -85,7 +88,7 @@ describe('StaticGraphParser', () => { } describe('createGraph', () => { - it('Creates a single node with no edges for a workflow with one step.', () => { + it('creates a single node with no edges for a workflow with one step.', () => { const workflow = newWorkflow(); const g = createGraph(workflow); expect(g.nodeCount()).toBe(1); @@ -93,10 +96,10 @@ describe('StaticGraphParser', () => { expect(g.node(workflow.spec.templates[0].dag.tasks[0].name).label).toBe('task-1'); }); - it('Adds an unconnected node for the onExit template, if onExit is specified', () => { + it('adds an unconnected node for the onExit template, if onExit is specified', () => { const workflow = newWorkflow(); workflow.spec.onExit = 'on-exit'; - workflow.spec.templates.push({ container: null, name: 'on-exit' }); + workflow.spec.templates.push({ container: {} as any, name: 'on-exit' }); const g = createGraph(workflow); expect(g.nodeCount()).toBe(2); expect(g.edgeCount()).toBe(0); @@ -104,7 +107,7 @@ describe('StaticGraphParser', () => { expect(g.node('on-exit').label).toBe('onExit - ' + 'on-exit'); }); - it('Adds and connects nodes based on listed dependencies', () => { + it('adds and connects nodes based on listed dependencies', () => { const workflow = newWorkflow(); workflow.spec.templates[0].dag.tasks.push({ name: 'task-2', dependencies: ['task-1'] }); const g = createGraph(workflow); @@ -114,7 +117,7 @@ describe('StaticGraphParser', () => { expect(g.edges()[0].w).toBe('task-2'); }); - it('Allows there to be and connects nodes based on listed dependencies', () => { + it('allows there to be and connects nodes based on listed dependencies', () => { const workflow = newWorkflow(); workflow.spec.templates[0].dag.tasks.push({ name: 'task-2', dependencies: ['task-1'] }); const g = createGraph(workflow); @@ -124,33 +127,18 @@ describe('StaticGraphParser', () => { expect(g.edges()[0].w).toBe('task-2'); }); - it('Shows conditional nodes without adding conditional children as nodes', () => { - const g = createGraph(newConditionalWorkflow()); - expect(g.nodeCount()).toBe(5); - ['flip', 'condition-1', 'condition-2', 'heads', 'tails'].forEach((nodeName) => { - expect(g.nodes()).toContain(nodeName); - }); - }); - - it('Connects conditional graph correctly', () => { + it('connects conditional graph correctly', () => { const g = createGraph(newConditionalWorkflow()); // 'flip' has two possible outcomes: 'condition-1' and 'condition-2' - expect(g.edges()[0].v).toBe('flip'); - expect(g.edges()[0].w).toBe('condition-1'); - expect(g.edges()[1].v).toBe('flip'); - expect(g.edges()[1].w).toBe('condition-2'); - // condition-1 means the 'heads' node will run - expect(g.edges()[2].v).toBe('condition-1'); - expect(g.edges()[2].w).toBe('heads'); - // condition-2 means the 'tails' node will run - expect(g.edges()[3].v).toBe('condition-2'); - expect(g.edges()[3].w).toBe('tails'); - // Confirm there are no other nodes or edges + expect(g.edges()).toContainEqual({ v: 'flip-task', w: 'condition-1-task' }); + expect(g.edges()).toContainEqual({ v: 'flip-task', w: 'condition-2-task' }); + expect(g.edges()).toContainEqual({ v: 'condition-1-task', w: 'heads-task' }); + expect(g.edges()).toContainEqual({ v: 'condition-2-task', w: 'tails-task' }); expect(g.nodeCount()).toBe(5); expect(g.edgeCount()).toBe(4); }); - it('Finds conditionals and colors them', () => { + it('finds conditionals and colors them', () => { const g = createGraph(newConditionalWorkflow()); g.nodes().forEach((nodeName) => { const node = g.node(nodeName); @@ -162,7 +150,19 @@ describe('StaticGraphParser', () => { }); }); - it('Renders extremely simple workflows with no steps or DAGs', () => { + it('includes the conditional itself in the info of conditional nodes', () => { + const g = createGraph(newConditionalWorkflow()); + g.nodes().forEach((nodeName) => { + const node = g.node(nodeName); + if (nodeName.startsWith('condition-1')) { + expect(node.info.condition).toBe('{{inputs.parameters.flip-output}} == heads'); + } else if (nodeName.startsWith('condition-2')) { + expect(node.info.condition).toBe('{{inputs.parameters.flip-output}} == tails'); + } + }); + }); + + it('renders extremely simple workflows with no steps or DAGs', () => { const simpleWorkflow = { spec: { entrypoint: 'template-1', @@ -179,7 +179,7 @@ describe('StaticGraphParser', () => { // The compiled Pipelines will contain 1 DAG for the entry point, that has a single task which // is the exit-handler. The exit-handler will be another DAG that contains all of the Pipeline's // components. - it('Does not show any nodes for the entrypoint DAG if it just points to an another DAG', () => { + it('does not show any nodes for the entrypoint DAG if it just points to an another DAG', () => { const workflow = { spec: { entrypoint: 'entrypoint-dag', @@ -200,367 +200,168 @@ describe('StaticGraphParser', () => { expect(g.edgeCount()).toBe(0); }); - it('Throws an error if the workflow has no spec', () => { + it('throws an error if the workflow has no spec', () => { expect(() => createGraph({} as any)).toThrowError('Could not generate graph.'); }); - it('Throws an error message if the workflow spec has no templates', () => { + it('throws an error message if the workflow spec has no templates', () => { expect(() => createGraph({} as any)).toThrowError('Could not generate graph.'); }); + }); - it('Throws an error message if the workflow spec has a template with multiple workflow steps', () => { - const workflow = { - spec: { - templates: [ - { - steps: [ - [{ name: 'workflow-step-1', template: 'workflow-step-1' }], - [{ name: 'workflow-step-2', template: 'workflow-step-2' }], - ], - }, - ], - }, - } as any; - expect(() => createGraph(workflow)).toThrowError('Pipeline had template with multiple steps.'); + describe('populateInfoFromTemplate', () => { + it('returns default SelectedNodeInfo if template is undefined', () => { + const defaultSelectedNodeInfo = new SelectedNodeInfo(); + const nodeInfo = _populateInfoFromTemplate(defaultSelectedNodeInfo, undefined); + expect(nodeInfo).toEqual(defaultSelectedNodeInfo); }); - it('Throws an error message if the workflow spec has a template with multiple steps', () => { - const workflow = { - spec: { - templates: [ - { - steps: [ - [ - { name: 'step-1', template: 'step-1' }, - { name: 'step-2', template: 'step-2' }, - ], - ], - }, - ], + it('returns default SelectedNodeInfo if template has no container', () => { + const defaultSelectedNodeInfo = new SelectedNodeInfo(); + const nodeInfo = _populateInfoFromTemplate(defaultSelectedNodeInfo, { name: 'template' } as any); + expect(nodeInfo).toEqual(defaultSelectedNodeInfo); + }); + + it('returns nodeInfo with empty values for args, command, and/or image if container does not have them', () => { + const template = { + container: { + // No args + // No command + // No image }, + dag: [], + name: 'template-1', } as any; - expect(() => createGraph(workflow)).toThrowError('Pipeline had template with multiple steps'); + const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template); + expect(nodeInfo.nodeType).toBe('container'); + expect(nodeInfo.args).toEqual([]); + expect(nodeInfo.command).toEqual([]); + expect(nodeInfo.image).toEqual(''); }); - }); - - // describe('getNodeInfo', () => { - // it('Returns null if nodeId is undefined', () => { - // const nodeInfo = getNodeInfo(newWorkflow(), undefined); - // expect(nodeInfo).toBeNull(); - // }); - - // it('Returns null if nodeId is empty', () => { - // const nodeInfo = getNodeInfo(newWorkflow(), ''); - // expect(nodeInfo).toBeNull(); - // }); - - // it('Returns null if workflow is undefined', () => { - // const nodeInfo = getNodeInfo(undefined, 'someId'); - // expect(nodeInfo).toBeNull(); - // }); - - // it('Returns null if workflow.spec is undefined', () => { - // const workflow = newWorkflow(); - // workflow.spec = undefined; - // const nodeInfo = getNodeInfo(workflow, 'someId'); - // expect(nodeInfo).toBeNull(); - // }); - - // it('Returns null if workflow.spec.templates is undefined', () => { - // const workflow = newWorkflow(); - // workflow.spec.templates = undefined; - // const nodeInfo = getNodeInfo(workflow, 'someId'); - // expect(nodeInfo).toBeNull(); - // }); - - // it('Returns nodeInfo containing only \'unknown\' nodeType if no template matches provided ID', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // container: {}, - // name: 'template-1', - // }, - // ], - // }, - // } as any; - // const nodeInfo = getNodeInfo(workflow, 'id-not-in-spec'); - // expect(nodeInfo).toEqual({ nodeType: 'unknown' }); - // }); - - // it('Returns nodeInfo containing only \'unknown\' nodeType if template matching provided ID has no container or steps', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // // No container or steps here - // name: 'template-1', - // }, - // ], - // }, - // } as any; - // const nodeInfo = getNodeInfo(workflow, 'template-1'); - // expect(nodeInfo).toEqual({ nodeType: 'unknown' }); - // }); - - // it('Returns nodeInfo containing only \'unknown\' nodeType if template matching provided ID has no container and empty steps', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // // No container here - // name: 'template-1', - // steps: [], - // }, - // ], - // }, - // } as any; - // const nodeInfo = getNodeInfo(workflow, 'template-1'); - // expect(nodeInfo).toEqual({ nodeType: 'unknown' }); - // }); - // it('Returns nodeInfo containing only \'unknown\' nodeType if template matching provided ID has no container and array of empty steps', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // // No container here - // name: 'template-1', - // steps: [[]], - // }, - // ], - // }, - // } as any; - // const nodeInfo = getNodeInfo(workflow, 'template-1'); - // expect(nodeInfo).toEqual({ nodeType: 'unknown' }); - // }); - // it('Returns nodeInfo containing container args', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // container: { - // args: ['arg1', 'arg2'], - // }, - // name: 'template-1', - // }, - // ], - // }, - // } as any; - // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - // expect(nodeInfo.nodeType).toBe('container'); - // expect(nodeInfo.containerInfo!.args).toEqual(['arg1', 'arg2']); - // }); - - // it('Returns nodeInfo containing container commands', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // container: { - // command: ['command1', 'command2'] - // }, - // name: 'template-1', - // }, - // ], - // }, - // } as any; - // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - // expect(nodeInfo.nodeType).toBe('container'); - // expect(nodeInfo.containerInfo!.command).toEqual(['command1', 'command2']); - // }); - - // it('Returns nodeInfo containing container image name', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // container: { - // image: 'someImageID' - // }, - // name: 'template-1', - // }, - // ], - // }, - // } as any; - // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - // expect(nodeInfo.nodeType).toBe('container'); - // expect(nodeInfo.containerInfo!.image).toBe('someImageID'); - // }); + it('returns nodeInfo containing container args', () => { + const template = { + container: { + args: ['arg1', 'arg2'], + }, + dag: [], + name: 'template-1', + } as any; + const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template); + expect(nodeInfo.nodeType).toBe('container'); + expect(nodeInfo.args).toEqual(['arg1', 'arg2']); + }); - // it('Returns nodeInfo containing container inputs as list of name/value tuples', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // container: {}, - // inputs: { - // parameters: [ - // { name: 'param1', value: 'val1' }, - // { name: 'param2', value: 'val2' }, - // ], - // }, - // name: 'template-1', - // }, - // ], - // }, - // } as any; - // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - // expect(nodeInfo.nodeType).toBe('container'); - // expect(nodeInfo.containerInfo!.inputs).toEqual([['param1', 'val1'], ['param2', 'val2']]); - // }); + it('returns nodeInfo containing container commands', () => { + const template = { + container: { + command: ['echo', 'some-command'], + }, + dag: [], + name: 'template-1', + } as any; + const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template); + expect(nodeInfo.nodeType).toBe('container'); + expect(nodeInfo.command).toEqual(['echo', 'some-command']); + }); - // it('Returns empty strings for inputs with no specified value', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // container: {}, - // inputs: { - // parameters: [ - // { name: 'param1' }, - // { name: 'param2' }, - // ], - // }, - // name: 'template-1', - // }, - // ], - // }, - // } as any; - // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - // expect(nodeInfo.nodeType).toBe('container'); - // expect(nodeInfo.containerInfo!.inputs).toEqual([['param1', ''], ['param2', '']]); - // }); + it('returns nodeInfo containing container image', () => { + const template = { + container: { + image: 'some-image' + }, + dag: [], + name: 'template-1', + } as any; + const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template); + expect(nodeInfo.nodeType).toBe('container'); + expect(nodeInfo.image).toEqual('some-image'); + }); - // it('Returns nodeInfo containing container outputs as list of name/value tuples, pulling from valueFrom if necessary', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // container: {}, - // name: 'template-1', - // outputs: { - // parameters: [ - // { name: 'param1', value: 'val1' }, - // { name: 'param2', valueFrom: { jsonPath: 'jsonPath' } }, - // { name: 'param3', valueFrom: { path: 'path' } }, - // { name: 'param4', valueFrom: { parameter: 'parameterReference' } }, - // { name: 'param5', valueFrom: { jqFilter: 'jqFilter' } }, - // ], - // }, - // }, - // ], - // }, - // } as any; - // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - // expect(nodeInfo.nodeType).toBe('container'); - // expect(nodeInfo.containerInfo!.outputs).toEqual([ - // ['param1', 'val1'], - // ['param2', 'jsonPath'], - // ['param3', 'path'], - // ['param4', 'parameterReference'], - // ['param5', 'jqFilter'], - // ]); - // }); + it('returns nodeInfo with empty values if template does not have inputs and/or outputs', () => { + const template = { + container: {}, + dag: [], + // No inputs + // No outputs + name: 'template-1', + } as any; + const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template); + expect(nodeInfo.nodeType).toBe('container'); + expect(nodeInfo.inputs).toEqual([[]]); + expect(nodeInfo.outputs).toEqual([[]]); + }); - // it('Returns empty strings for outputs with no specified value', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // container: {}, - // name: 'template-1', - // outputs: { - // parameters: [ - // { name: 'param1' }, - // { name: 'param2' }, - // ], - // }, - // }, - // ], - // }, - // } as any; - // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - // expect(nodeInfo.nodeType).toBe('container'); - // expect(nodeInfo.containerInfo!.outputs).toEqual([['param1', ''], ['param2', '']]); - // }); + it('returns nodeInfo containing template inputs params as list of name/value tuples', () => { + const template = { + container: {}, + dag: [], + inputs: { + parameters: [{ name: 'param1', value: 'val1' }, { name: 'param2', value: 'val2' }] + }, + name: 'template-1', + } as any; + const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template); + expect(nodeInfo.nodeType).toBe('container'); + expect(nodeInfo.inputs).toEqual([['param1', 'val1'], ['param2', 'val2']]); + }); - // it('Returns nodeType \'steps\' when node template has steps', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // name: 'template-1', - // steps: [[ - // 'something', - // ]] - // }, - // ], - // }, - // } as any; - // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - // expect(nodeInfo.nodeType).toBe('steps'); - // expect(nodeInfo.stepsInfo).toEqual({ conditional: '', parameters: [[]] }); - // }); + it('returns empty strings for inputs with no specified value', () => { + const template = { + container: {}, + dag: [], + inputs: { + parameters: [{ name: 'param1' }, { name: 'param2' }] + }, + name: 'template-1', + } as any; + const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template); + expect(nodeInfo.nodeType).toBe('container'); + expect(nodeInfo.inputs).toEqual([['param1', ''], ['param2', '']]); + }); - // it('Returns nodeInfo with step template\'s conditional when node template has \'when\' property', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // name: 'template-1', - // steps: [[ { when: '{{someVar}} == something' } ]] - // }, - // ], - // }, - // } as any; - // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - // expect(nodeInfo.nodeType).toBe('steps'); - // expect(nodeInfo.stepsInfo).toEqual({ conditional: '{{someVar}} == something', parameters: [[]] }); - // }); + it('returns nodeInfo containing container outputs as list of name/value tuples, pulling from valueFrom if necessary', () => { + const template = { + container: {}, + name: 'template-1', + outputs: { + parameters: [ + { name: 'param1', value: 'val1' }, + { name: 'param2', valueFrom: { jsonPath: 'jsonPath' } }, + { name: 'param3', valueFrom: { path: 'path' } }, + { name: 'param4', valueFrom: { parameter: 'parameterReference' } }, + { name: 'param5', valueFrom: { jqFilter: 'jqFilter' } }, + ], + }, + } as any; + const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template); + expect(nodeInfo.nodeType).toBe('container'); + expect(nodeInfo.outputs).toEqual([ + ['param1', 'val1'], + ['param2', 'jsonPath'], + ['param3', 'path'], + ['param4', 'parameterReference'], + ['param5', 'jqFilter'], + ]); + }); - // it('Returns nodeInfo with step template\'s arguments', () => { - // const workflow = { - // spec: { - // entrypoint: 'template-1', - // templates: [ - // { - // name: 'template-1', - // steps: [[{ - // arguments: { - // parameters: [ - // { name: 'param1', value: 'val1' }, - // { name: 'param2', value: 'val2' }, - // ], - // }, - // }]] - // }, - // ], - // }, - // } as any; - // const nodeInfo = verifyNodeInfoNotNull(getNodeInfo(workflow, 'template-1')); - // expect(nodeInfo.nodeType).toBe('steps'); - // expect(nodeInfo.stepsInfo) - // .toEqual({ conditional: '', parameters: [['param1', 'val1'], ['param2', 'val2']] }); - // }); - // }); + it('returns empty strings for outputs with no specified value', () => { + const template = { + container: {}, + name: 'template-1', + outputs: { + parameters: [ + { name: 'param1' }, + { name: 'param2' }, + ], + }, + } as any; + const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template); + expect(nodeInfo.nodeType).toBe('container'); + expect(nodeInfo.outputs).toEqual([['param1', ''], ['param2', '']]); + }); - // function verifyNodeInfoNotNull(nodeInfo: SelectedNodeInfo | null): SelectedNodeInfo { - // expect(nodeInfo).not.toBeNull(); - // return nodeInfo!; - // } + }); }); + diff --git a/frontend/src/lib/StaticGraphParser.ts b/frontend/src/lib/StaticGraphParser.ts index c45864bc643..b7b696a3f3b 100644 --- a/frontend/src/lib/StaticGraphParser.ts +++ b/frontend/src/lib/StaticGraphParser.ts @@ -18,18 +18,12 @@ import * as dagre from 'dagre'; import { Workflow, Template } from '../../third_party/argo-ui/argo_template'; import { logger } from './Utils'; -interface ConditionalInfo { - condition: string; - taskName: string; -} - export type nodeType = 'container' | 'dag' | 'unknown'; export class SelectedNodeInfo { public args: string[]; public command: string[]; public condition: string; - public conditionalTasks: ConditionalInfo[]; public image: string; public inputs: string[][]; public nodeType: nodeType; @@ -39,7 +33,6 @@ export class SelectedNodeInfo { this.args = []; this.command = []; this.condition = ''; - this.conditionalTasks = []; this.image = ''; this.inputs = [[]]; this.nodeType = 'unknown'; @@ -51,8 +44,8 @@ export class SelectedNodeInfo { const NODE_WIDTH = 180; const NODE_HEIGHT = 70; -function populateInfoFromTemplate(info: SelectedNodeInfo, template: Template): SelectedNodeInfo { - if (!template.container) { +export function _populateInfoFromTemplate(info: SelectedNodeInfo, template?: Template): SelectedNodeInfo { + if (!template || !template.container) { return info; } @@ -119,13 +112,13 @@ function buildDag( } // "Child" here is the template that this task points to. This template should either be a - // DAG, in which case we recurse, or a container which can be thought of as a leaf node. + // DAG, in which case we recurse, or a container which can be thought of as a leaf node const child = templates.get(task.template); if (child) { if (child.nodeType === 'dag') { buildDag(graph, task.template, templates, task.name); } else if (child.nodeType === 'container' ) { - populateInfoFromTemplate(info, child.template); + _populateInfoFromTemplate(info, child.template); } else { logger.error(`Unknown nodetype: ${child.nodeType} on template: ${child.template}`); } @@ -166,7 +159,7 @@ export function createGraph(workflow: Workflow): dagre.graphlib.Graph { // Argo allows specifying a single global exit handler. We also highlight that node if (template.name === workflow.spec.onExit) { const info = new SelectedNodeInfo(); - populateInfoFromTemplate(info, template); + _populateInfoFromTemplate(info, template); graph.setNode(template.name, { bgColor: '#eee', height: NODE_HEIGHT, diff --git a/frontend/src/pages/PipelineDetails.tsx b/frontend/src/pages/PipelineDetails.tsx index 754a706ab03..4f9eec7f4ce 100644 --- a/frontend/src/pages/PipelineDetails.tsx +++ b/frontend/src/pages/PipelineDetails.tsx @@ -360,16 +360,6 @@ class PipelineDetails extends Page<{}, PipelineDetailsState> {
Run when: {nodeInfo.condition}
)} - - {nodeInfo.conditionalTasks && !!nodeInfo.conditionalTasks.length && ( -
Conditionals
- )} - {nodeInfo.conditionalTasks.map((conditionalTask, i) => -
-
When: {conditionalTask.condition}
-
Run task: {conditionalTask.taskName}
-
- )} ; this.setState({ From 2357b13f67d3f6c8c583db3daea5a8ed59deeb61 Mon Sep 17 00:00:00 2001 From: Riley Bauer Date: Fri, 16 Nov 2018 13:49:19 -0800 Subject: [PATCH 5/5] Give nodes unique names --- frontend/src/lib/StaticGraphParser.test.ts | 100 ++++++++++++++------- frontend/src/lib/StaticGraphParser.ts | 16 ++-- 2 files changed, 77 insertions(+), 39 deletions(-) diff --git a/frontend/src/lib/StaticGraphParser.test.ts b/frontend/src/lib/StaticGraphParser.test.ts index 995a39bf579..bd5c2e41747 100644 --- a/frontend/src/lib/StaticGraphParser.test.ts +++ b/frontend/src/lib/StaticGraphParser.test.ts @@ -24,7 +24,7 @@ describe('StaticGraphParser', () => { entrypoint: 'template-1', templates: [ { - dag: { tasks: [{ name: 'task-1', template: 'task-1', },], }, + dag: { tasks: [{ name: 'task-1', template: 'container-1' }] }, name: 'template-1', }, { @@ -62,26 +62,16 @@ describe('StaticGraphParser', () => { }, name: 'pipeline-flip-coin', }, - // Container that flips the coin and outputs the result - { - container: { args: [ /* omitted */], command: ['sh', '-c'], }, - name: 'flip', - outputs: { parameters: [{ name: 'flip-output', valueFrom: { path: '/tmp/output' } }] } - }, // DAG representing execution if result is 'heads' - { - dag: { tasks: [{ name: 'heads-task', template: 'heads' }] }, - name: 'condition-1', - }, - // Container that is run if result is 'heads' - { container: { command: ['echo', '\'heads\''], }, name: 'heads', }, + { dag: { tasks: [{ name: 'heads-task', template: 'heads' }] }, name: 'condition-1' }, // DAG representing execution if result is 'tails' - { - dag: { tasks: [{ name: 'tails-task', template: 'tails' }] }, - name: 'condition-2', - }, + { dag: { tasks: [{ name: 'tails-task', template: 'tails' }] }, name: 'condition-2' }, + // Container that flips the coin and outputs the result + { container: {}, name: 'flip' }, + // Container that is run if result is 'heads' + { container: {}, name: 'heads' }, // Container that is run if result is 'tails' - { container: { command: ['echo', '\'tails\''], }, name: 'tails', }, + { container: {}, name: 'tails' }, ] } }; @@ -93,7 +83,8 @@ describe('StaticGraphParser', () => { const g = createGraph(workflow); expect(g.nodeCount()).toBe(1); expect(g.edgeCount()).toBe(0); - expect(g.node(workflow.spec.templates[0].dag.tasks[0].name).label).toBe('task-1'); + expect(g.nodes()[0]).toBe('/task-1'); + expect(g.node('/task-1').label).toBe('container-1'); }); it('adds an unconnected node for the onExit template, if onExit is specified', () => { @@ -113,27 +104,68 @@ describe('StaticGraphParser', () => { const g = createGraph(workflow); expect(g.nodeCount()).toBe(2); expect(g.edgeCount()).toBe(1); - expect(g.edges()[0].v).toBe('task-1'); - expect(g.edges()[0].w).toBe('task-2'); + expect(g.edges()[0].v).toBe('/task-1'); + expect(g.edges()[0].w).toBe('/task-2'); }); - it('allows there to be and connects nodes based on listed dependencies', () => { - const workflow = newWorkflow(); - workflow.spec.templates[0].dag.tasks.push({ name: 'task-2', dependencies: ['task-1'] }); - const g = createGraph(workflow); - expect(g.nodeCount()).toBe(2); - expect(g.edgeCount()).toBe(1); - expect(g.edges()[0].v).toBe('task-1'); - expect(g.edges()[0].w).toBe('task-2'); + const nestedWorkflow = { + spec: { + entrypoint: 'dag-1', + templates: [ + { + dag: { tasks: [{ name: 'task-1-1', template: 'dag-2' }] }, + name: 'dag-1', + }, + { + dag: { + tasks: [ + { name: 'task-2-1', template: 'container-2-1' }, + { name: 'task-2-2', template: 'dag-3' }, + ] + }, + name: 'dag-2', + }, + { + dag: { tasks: [{ name: 'task-3-1', template: 'container-3-1' }] }, + name: 'dag-3', + }, + { container: {}, name: 'container-2-1' }, + { container: {}, name: 'container-3-1' }, + ], + }, + } as any; + + it('uses path to task as node ID', () => { + const g = createGraph(nestedWorkflow); + expect(g.nodeCount()).toBe(4); + expect(g.edgeCount()).toBe(3); + [ + '/task-1-1', + '/task-1-1/task-2-1', + '/task-1-1/task-2-2', + '/task-1-1/task-2-2/task-3-1', + ].forEach((nodeId) => expect(g.nodes()).toContain(nodeId)); + }); + + it('uses task\'s template as node label', () => { + const g = createGraph(nestedWorkflow); + expect(g.nodeCount()).toBe(4); + expect(g.edgeCount()).toBe(3); + [ + { id: '/task-1-1', label: 'dag-2' }, + { id: '/task-1-1/task-2-1', label: 'container-2-1' }, + { id: '/task-1-1/task-2-2', label: 'dag-3' }, + { id: '/task-1-1/task-2-2/task-3-1', label: 'container-3-1' }, + ].forEach((idAndLabel) => expect(g.node(idAndLabel.id).label).toBe(idAndLabel.label)); }); it('connects conditional graph correctly', () => { const g = createGraph(newConditionalWorkflow()); // 'flip' has two possible outcomes: 'condition-1' and 'condition-2' - expect(g.edges()).toContainEqual({ v: 'flip-task', w: 'condition-1-task' }); - expect(g.edges()).toContainEqual({ v: 'flip-task', w: 'condition-2-task' }); - expect(g.edges()).toContainEqual({ v: 'condition-1-task', w: 'heads-task' }); - expect(g.edges()).toContainEqual({ v: 'condition-2-task', w: 'tails-task' }); + expect(g.edges()).toContainEqual({ v: '/flip-task', w: '/condition-1-task' }); + expect(g.edges()).toContainEqual({ v: '/flip-task', w: '/condition-2-task' }); + expect(g.edges()).toContainEqual({ v: '/condition-1-task', w: '/condition-1-task/heads-task' }); + expect(g.edges()).toContainEqual({ v: '/condition-2-task', w: '/condition-2-task/tails-task' }); expect(g.nodeCount()).toBe(5); expect(g.edgeCount()).toBe(4); }); @@ -142,7 +174,7 @@ describe('StaticGraphParser', () => { const g = createGraph(newConditionalWorkflow()); g.nodes().forEach((nodeName) => { const node = g.node(nodeName); - if (nodeName.startsWith('condition')) { + if (nodeName === '/condition-1-task' || nodeName === '/condition-2-task') { expect(node.bgColor).toBe('cornsilk'); } else { expect(node.bgColor).toBeUndefined(); diff --git a/frontend/src/lib/StaticGraphParser.ts b/frontend/src/lib/StaticGraphParser.ts index b7b696a3f3b..4601ceaeefa 100644 --- a/frontend/src/lib/StaticGraphParser.ts +++ b/frontend/src/lib/StaticGraphParser.ts @@ -92,6 +92,9 @@ function buildDag( const template = root.template; (template.dag.tasks || []).forEach((task) => { + + const nodeId = (parent || '') + '/' + task.name; + // If the user specifies an exit handler, then the compiler will wrap the entire Pipeline // within an additional DAG template prefixed with "exit-handler". // If this is the case, we simply treat it as the root of the graph and work from there @@ -101,7 +104,7 @@ function buildDag( // Parent here will be the task that pointed to this DAG template if (parent) { - graph.setEdge(parent, task.name); + graph.setEdge(parent, nodeId); } // This object contains information about the node that we display to the user when they @@ -116,7 +119,7 @@ function buildDag( const child = templates.get(task.template); if (child) { if (child.nodeType === 'dag') { - buildDag(graph, task.template, templates, task.name); + buildDag(graph, task.template, templates, nodeId); } else if (child.nodeType === 'container' ) { _populateInfoFromTemplate(info, child.template); } else { @@ -124,18 +127,21 @@ function buildDag( } } - graph.setNode(task.name, { + graph.setNode(nodeId, { bgColor: task.when ? 'cornsilk' : undefined, height: NODE_HEIGHT, info, - label: task.name, + label: task.template, width: NODE_WIDTH, }); } // DAG tasks can indicate dependencies which are graphically shown as parents with edges // pointing to their children (the task(s)). - (task.dependencies || []).forEach((dep) => graph.setEdge(dep, task.name)); + // TODO: The addition of the parent prefix to the dependency here is only valid if nodes only + // ever directly depend on their siblings. This is true now but may change in the future, and + // this will need to be updated. + (task.dependencies || []).forEach((dep) => graph.setEdge((parent || '') + '/' + dep, nodeId)); }); } }